Fix flakiness in the eval integration test (#10008)
I looked at three different logs of devtools tests flaking out on Linux bots on LUCI,
All three logs failed in eval_integration_test.dart:
* Log 1 & Log 2: EvalOnDartLibrary asyncEval supports expressions that do not start with the await keyword timed out after 1 minute (TimeoutException: Test timed out after 1 minutes).
* Log 3: EvalOnDartLibrary asyncEval returns the result of the future completion timed out after 1 minute.
* Cascading failures: On all three runs, following the timeout, the test runner attempted retries (due to retry: 3), but every retry and subsequent test failed with evaluate: (-32000) Service connection disposed.
1. Garbage Collection of reader before evaluation began:
In eval_on_dart_library.dart:390-490, the reader list was previously created in a standalone eval (widgetInspectorService.toId(<dynamic>[], "$readerGroup")). Because WidgetInspectorService only holds objects using
WeakReference, nothing held a strong reference to <dynamic>[] in the target isolate during the network round-trip between DevTools and the target app. If garbage collection occurred in that window, widgetInspectorService.
toObject in the subsequent eval returned null, throwing:
Unhandled exception: type 'Null' is not a subtype of type 'List<dynamic>' in type cast
Because this exception occurred before the try/finally block inside the closure, postEvent("future_completed", ...) was never posted, causing DevTools to hang waiting on future_completed until the test timed out after 1
minute.
2. Pinning loop stopped prematurely:
The pinning loop (while (!isDone && ++bufferTicks <= 20)) intended to pin reader until retrieved. However:
* As soon as isDone was set to true, !isDone became false and the loop terminated immediately (providing zero buffer while postEvent traveled to DevTools and DevTools issued evalInstance). If a GC occurred during that
window, toObject returned null.
* For evaluations taking longer than 1 second (20 ticks of 50ms), ++bufferTicks <= 20 became false while the future was still pending, causing reader to be unpinned before completion.
3. Unhandled error in target isolate:
Any exception occurring during reader initialization or eval setup in the target app closure was unhandled, preventing future_completed from ever firing and causing the target isolate to crash/disconnect.
4. Stale environment reuse on connection drop:
In flutter_test_environment.dart:100-125, _needsSetup was not re-evaluated if the VM service connection dropped (!connectedState.value.connected). When a test timed out and the process disconnected, retries attempted to
reuse the disposed connection, resulting in evaluate: (-32000) Service connection disposed.
1. eval_on_dart_library.dart
* Allocated `final reader = <dynamic>[];` directly inside the evaluated async function and registered it with `widgetInspectorService.toId(reader, "$readerGroup") as String`, eliminating the preliminary eval and ensuring
reader is strongly referenced from the moment of allocation.
* Transmitted reader_id (and any initialization error) directly in postEvent("future_completed", ...).
* Wrapped the entire async function in an outer try/catch that reports errors back via postEvent rather than hanging DevTools.
* Kept reader strongly pinned in the target isolate in the finally block by awaiting in a loop and accessing reader.length until evalInstance calls disposeGroup (or up to 10 seconds timeout).
2. flutter_test_environment.dart
* Added !serviceConnection.serviceManager.connectedState.value.connected to setupEnvironment's re-initialization condition so that if the VM service connection is disposed or dropped, the test environment is
automatically re-created for subsequent tests/retries.
3. eval_integration_test.dart
* Added an explicit test survives garbage collection while the future is pending that triggers full garbage collections in the target isolate via getAllocationProfile(..., gc: true) while the future is pending.
* Removed tags: skipForCustomerTestsTag and retry: 3 now that the flakiness is resolved.
• Ran dart analyze on modified files: No issues found.
• Ran dart format: All modified files formatted.
• Ran 5 consecutive test suites of packages/devtools_app/test/shared/eval_integration_test.dart: All 5 runs passed (5/5 tests passing per run).diff --git a/packages/devtools_app/test/shared/eval_integration_test.dart b/packages/devtools_app/test/shared/eval_integration_test.dart
index 3dea886..ccc6c7c 100644
--- a/packages/devtools_app/test/shared/eval_integration_test.dart
+++ b/packages/devtools_app/test/shared/eval_integration_test.dart
@@ -5,7 +5,6 @@
import 'package:devtools_app/src/shared/globals.dart';
import 'package:devtools_app_shared/service.dart';
import 'package:devtools_app_shared/utils.dart';
-import 'package:devtools_test/helpers.dart';
import 'package:flutter_test/flutter_test.dart';
import '../test_infra/flutter_test_driver.dart';
@@ -71,39 +70,58 @@
expect(instance2.classRef!.name, '_Future');
},
timeout: const Timeout.factor(2),
- // TODO(https://github.com/flutter/devtools/issues/9484): if this flake
- // is addressed, we can unskip this for the Flutter customer tests.
- tags: skipForCustomerTestsTag,
);
- test(
- 'returns the result of the future completion',
- () async {
- await env.setupEnvironment();
- final mainIsolate =
- serviceConnection.serviceManager.isolateManager.mainIsolate;
- expect(mainIsolate, isNotNull);
+ test('returns the result of the future completion', () async {
+ await env.setupEnvironment();
+ final mainIsolate =
+ serviceConnection.serviceManager.isolateManager.mainIsolate;
- final eval = EvalOnDartLibrary(
- 'dart:core',
- serviceConnection.serviceManager.service!,
- serviceManager: serviceConnection.serviceManager,
- isolate: mainIsolate,
+ final eval = EvalOnDartLibrary(
+ 'dart:core',
+ serviceConnection.serviceManager.service!,
+ serviceManager: serviceConnection.serviceManager,
+ isolate: mainIsolate,
+ );
+
+ final instance = (await eval.asyncEval(
+ // The delay asserts that there is no issue with garbage collection
+ 'await Future<int>.delayed(const Duration(milliseconds: 500), () => 42)',
+ isAlive: isAlive,
+ ))!;
+
+ expect(instance.valueAsString, '42');
+ }, timeout: const Timeout.factor(2));
+
+ test('survives garbage collection while the future is pending', () async {
+ await env.setupEnvironment();
+ final mainIsolate =
+ serviceConnection.serviceManager.isolateManager.mainIsolate;
+
+ final eval = EvalOnDartLibrary(
+ 'dart:core',
+ serviceConnection.serviceManager.service!,
+ serviceManager: serviceConnection.serviceManager,
+ isolate: mainIsolate,
+ );
+
+ final evalFuture = eval.asyncEval(
+ 'await Future<int>.delayed(const Duration(milliseconds: 500), () => 42)',
+ isAlive: isAlive,
+ );
+
+ // Force garbage collection in the target isolate while the future is pending.
+ for (var i = 0; i < 3; i++) {
+ await Future<void>.delayed(const Duration(milliseconds: 100));
+ await serviceConnection.serviceManager.service!.getAllocationProfile(
+ mainIsolate.value!.id!,
+ gc: true,
);
+ }
- final instance = (await eval.asyncEval(
- // The delay asserts that there is no issue with garbage collection
- 'await Future<int>.delayed(const Duration(milliseconds: 500), () => 42)',
- isAlive: isAlive,
- ))!;
-
- expect(instance.valueAsString, '42');
- },
- timeout: const Timeout.factor(2),
- // TODO(https://github.com/flutter/devtools/issues/9484): if this flake
- // is addressed, we can unskip this for the Flutter customer tests.
- tags: skipForCustomerTestsTag,
- );
+ final instance = (await evalFuture)!;
+ expect(instance.valueAsString, '42');
+ }, timeout: const Timeout.factor(2));
test(
'throws FutureFailedException when the future is rejected',
@@ -152,12 +170,7 @@
expect(error.valueAsString, 'foo');
},
timeout: const Timeout.factor(2),
- // TODO(https://github.com/flutter/devtools/issues/9484): if this flake
- // is addressed, we can unskip this for the Flutter customer tests.
- tags: skipForCustomerTestsTag,
);
- // TODO(https://github.com/flutter/devtools/issues/9484): if this flake
- // is addressed, we can remove the retry.
- }, retry: 3);
+ });
});
}
diff --git a/packages/devtools_app/test/test_infra/flutter_test_environment.dart b/packages/devtools_app/test/test_infra/flutter_test_environment.dart
index d85b328..71906b7 100644
--- a/packages/devtools_app/test/test_infra/flutter_test_environment.dart
+++ b/packages/devtools_app/test/test_infra/flutter_test_environment.dart
@@ -107,7 +107,8 @@
if (force ||
_needsSetup ||
!reuseTestEnvironment ||
- _isNewRunConfig(config)) {
+ _isNewRunConfig(config) ||
+ !serviceConnection.serviceManager.connectedState.value.connected) {
_setupInProgress = Completer();
try {
// If we already have a running test device, stop it before setting up a
diff --git a/packages/devtools_app_shared/CHANGELOG.md b/packages/devtools_app_shared/CHANGELOG.md
index 9115a73..4965452 100644
--- a/packages/devtools_app_shared/CHANGELOG.md
+++ b/packages/devtools_app_shared/CHANGELOG.md
@@ -6,7 +6,7 @@
## 0.5.2-wip
* Fix a `RangeError` thrown by `SplitPane` when the number of children
changes between rebuilds.
-* Fix garbage collection issues with the result list in `asyncEval` on both native VM and web.
+* Fix garbage collection and timeout issues with the result list in `asyncEval` on both native VM and web.
* Safely handle RPC errors and unexpected exceptions when calling service extensions in `ServiceExtensionManager`.
* The minimum Dart SDK version is bumped to 3.11.0.
* The minimum Flutter SDK version is bumped to 3.41.0.
diff --git a/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart b/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart
index 8df2f92..e831211 100644
--- a/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart
+++ b/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart
@@ -418,43 +418,50 @@
isAlive: isAlive,
);
- final readerId = await safeEval(
- // since we are awaiting the Future, we need to make sure that during the awaiting,
- // the "reader" is not GCed
- 'widgetInspectorService.toId(<dynamic>[], "$readerGroup")',
- isAlive: isAlive,
- scope: {'widgetInspectorService': widgetInspectorServiceRef.id!},
- ).then((ref) => ref.valueAsString!);
-
await safeEval(
'() async {'
- ' final reader = widgetInspectorService.toObject("$readerId", "$readerGroup") as List;'
- ' /* Keep a strong reference to `reader` in the target app to prevent it'
- ' from being garbage collected by Chrome/VM before the future resolves.'
- ' Without this, the reader is only weakly referenced by the inspector'
- ' service and is aggressively GCed, causing a TypeError/TimeoutException'
- ' or failing the assertion that the retrieved result length is 1 or 2.'
- ' We use Future.delayed in a loop instead of Timer because Future is in'
- ' dart:core and guaranteed to be resolved without requiring dart:async. */'
- ' bool isDone = false;'
- ' () async {'
- ' int bufferTicks = 0;'
- ' /* Stop pinning after a 1-second buffer when the future has completed. */'
- ' while (!isDone && ++bufferTicks <=20) {'
- ' final _ = reader;'
- ' await Future.delayed(const Duration(milliseconds: 50));'
- ' }'
- ' }();'
+ ' String? readerId;'
' try {'
+ ' final reader = <dynamic>[];'
+ ' readerId = widgetInspectorService.toId(reader, "$readerGroup") as String;'
+ ' try {'
// Cast as dynamic so that it is possible to await Future<void>
- ' dynamic result = ($expression) as dynamic;'
- ' reader.add(result);'
- ' } catch (err, stack) {'
- ' reader.add(err);'
- ' reader.add(stack);'
- ' } finally {'
- ' isDone = true;'
- ' postEvent("future_completed", {"future_id": $futureId, "client_id": $_clientId});'
+ ' dynamic result = ($expression) as dynamic;'
+ ' reader.add(result);'
+ ' } catch (err, stack) {'
+ ' reader.add(err);'
+ ' reader.add(stack);'
+ ' } finally {'
+ ' postEvent("future_completed", {'
+ ' "future_id": $futureId,'
+ ' "client_id": $_clientId,'
+ ' "reader_id": readerId,'
+ ' });'
+ ' /* Keep a strong reference to `reader` in the target app to prevent it'
+ ' from being garbage collected by Chrome/VM before DevTools retrieves it.'
+ ' Without this, the reader is only weakly referenced by the inspector'
+ ' service and is aggressively GCed, causing a TypeError/TimeoutException'
+ ' or failing the assertion that the retrieved result length is 1 or 2.'
+ ' We use Future.delayed in a loop instead of Timer because Future is in'
+ ' dart:core and guaranteed to be resolved without requiring dart:async. */'
+ ' for (int i = 0; i < 200; i++) {'
+ ' await Future.delayed(const Duration(milliseconds: 50));'
+ ' try {'
+ ' if (widgetInspectorService.toObject(readerId!, "$readerGroup") == null) {'
+ ' break;'
+ ' }'
+ ' } catch (_) {'
+ ' break;'
+ ' }'
+ ' final _ = reader.length;'
+ ' }'
+ ' }'
+ ' } catch (err) {'
+ ' postEvent("future_completed", {'
+ ' "future_id": $futureId,'
+ ' "client_id": $_clientId,'
+ ' "error": err.toString(),'
+ ' });'
' }'
'}()',
isAlive: isAlive,
@@ -465,7 +472,15 @@
},
);
- await future;
+ final event = await future;
+ if (event.extensionData?.data['error'] != null) {
+ throw UnknownEvalException(
+ expression: expression,
+ scope: scope,
+ exception: event.extensionData!.data['error'],
+ );
+ }
+ final readerId = event.extensionData!.data['reader_id'] as String;
final resultRef = await evalInstance(
'() {'
