Conversation
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).
There was a problem hiding this comment.
Code Review
This pull request refactors asyncEval in EvalOnDartLibrary to improve garbage collection and timeout handling by declaring the reader within the evaluated block and returning its ID via the completion event. It also adds a new integration test to verify survival during garbage collection and updates the test environment setup conditions. The review feedback highlights a potential compilation error in the target isolate due to passing a nullable readerId to toObject, and notes that integration tests should assert mainIsolate.value rather than the ValueListenable container itself.
| ' for (int i = 0; i < 200; i++) {' | ||
| ' await Future.delayed(const Duration(milliseconds: 50));' | ||
| ' try {' | ||
| ' if (widgetInspectorService.toObject(readerId, "$readerGroup") == null) {' |
There was a problem hiding this comment.
[MUST-FIX] Since readerId is declared as a nullable String? on line 423, passing it directly to widgetInspectorService.toObject (which expects a non-nullable String) will cause a static type error / compilation failure in the target isolate under sound null safety. Use the null-assertion operator readerId! to ensure it compiles correctly.
' if (widgetInspectorService.toObject(readerId!, "$readerGroup") == null) {'| await env.setupEnvironment(); | ||
| final mainIsolate = | ||
| serviceConnection.serviceManager.isolateManager.mainIsolate; | ||
| expect(mainIsolate, isNotNull); |
There was a problem hiding this comment.
[CONCERN] mainIsolate is a ValueListenable<IsolateRef?>, which is a container that is never null. To assert that the main isolate has been successfully populated and is not null, you should check mainIsolate.value instead.
| expect(mainIsolate, isNotNull); | |
| expect(mainIsolate.value, isNotNull); |
| await env.setupEnvironment(); | ||
| final mainIsolate = | ||
| serviceConnection.serviceManager.isolateManager.mainIsolate; | ||
| expect(mainIsolate, isNotNull); |
There was a problem hiding this comment.
[CONCERN] mainIsolate is a ValueListenable<IsolateRef?>, which is a container that is never null. To assert that the main isolate has been successfully populated and is not null, you should check mainIsolate.value instead.
| expect(mainIsolate, isNotNull); | |
| expect(mainIsolate.value, isNotNull); |
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.
asyncEvalsupports expressions that do not start with theawaitkeyword timed out after 1 minute ("TimeoutException: Test timed out after 1 minutes").asyncEvalreturns the result of the future completion timed out after 1 minute.retry: 3), but every retry and subsequent test failed with "evaluate: (-32000) Service connection disposed."So here are the root causes:
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.toObjectin the subsequent eval returnednull, 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 onfuture_completeduntil the test timed out after 1 minute.while (!isDone && ++bufferTicks <= 20)) intended to pin reader until retrieved. However:isDonewas set totrue,!isDonebecamefalseand the loop terminated immediately (providing zero buffer whilepostEventtraveled to DevTools and DevTools issued evalInstance). If a GC occurred during that window,toObjectreturnednull.++bufferTicks <= 20becamefalsewhile the future was still pending, causing reader to be unpinned before completion.future_completedfrom ever firing and causing the target isolate to crash/disconnect.flutter_test_environment.dart:100-125,_needsSetupwas 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."Here's the fixes:
eval_on_dart_library.dartfinal reader = <dynamic>[];directly inside the evaluated async function and registered it withwidgetInspectorService.toId(reader, "$readerGroup") as String, eliminating the preliminary eval and ensuring reader is strongly referenced from the moment of allocation.reader_id(and any initialization error) directly inpostEvent("future_completed", ...).postEventrather than hanging DevTools.reader.lengthuntilevalInstancecallsdisposeGroup(or up to 10 seconds timeout).flutter_test_environment.dart!serviceConnection.serviceManager.connectedState.value.connectedtosetupEnvironment'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.eval_integration_test.dartgetAllocationProfile(..., gc: true)while the future is pending.skipForCustomerTestsTagandretry: 3now that the flakiness is resolved. 🎊 🎉