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).
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. 🎊 🎉