[flutter_tools] Fix deadlock in debug adapters when process exits early - #190931
[flutter_tools] Fix deadlock in debug adapters when process exits early#190931bkonyi wants to merge 4 commits into
Conversation
When launching a debug session, if the process exits before the debugger connects, the debug adapter would deadlock waiting for the debugger to initialize. This change sets `waitingForDebugger = true` before launching the process, and uses `Future.any` to wait for either `debuggerInitialized` or `debuggerInitializationFailedCompleter.future`. The completer is completed with an error when the session terminates, breaking the deadlock. Fixes flutter#190721
There was a problem hiding this comment.
Code Review
This pull request updates the Flutter debug adapters to handle early session termination during debugger initialization by introducing a debuggerInitializationFailedCompleter and a waitingForDebugger flag, allowing the adapter to fail fast if the process exits early. The review feedback identifies a potential unhandled asynchronous exception if the completer is completed with an error before listeners are attached, and points out that the new public fields lack required documentation.
| bool waitingForDebugger = false; | ||
| final Completer<void> debuggerInitializationFailedCompleter = Completer<void>(); |
There was a problem hiding this comment.
Issue: Potential Unhandled Asynchronous Exception & Missing Documentation
-
Unhandled Exception Risk: If
handleSessionTerminateis called whilelaunchAsProcessis running (wherewaitingForDebuggeristruebutFuture.anyhas not yet been awaited),debuggerInitializationFailedCompleter.completeErrorwill be called. Since there are no listeners ondebuggerInitializationFailedCompleter.futureat that moment, this will result in an unhandled asynchronous exception in Dart, which can crash the process or cause test flakiness.Adding a dummy error handler using
..future.catchError((Object _) {})at creation time safely prevents unhandled exceptions while still allowingFuture.anyto receive the error when it eventually listens to it. -
Missing Documentation: According to the Repository Style Guide (Line 62: "All public members should have documentation."), these new public fields should have
///doc comments explaining their purpose.
Here is the suggested improvement:
/// Whether the adapter is currently waiting for the debugger to initialize.
bool waitingForDebugger = false;
/// A completer that completes with an error if debugger initialization fails
/// (for example, if the session terminates early).
///
/// A dummy error handler is attached to the future to prevent unhandled
/// asynchronous exceptions if it completes before any listeners are active.
final Completer<void> debuggerInitializationFailedCompleter = Completer<void>()
..future.catchError((Object _) {});References
- All public members should have documentation. (link)
There was a problem hiding this comment.
Did the bot get smart?
I think it's right - flutter_test_adapter sets waitingForDebugger = true before an await, so if it happens before reading the Future.any(), there will be no listeners and this might be an unhandled error?
…initialization failure and document public members
|
autosubmit label was removed for flutter/flutter/190931, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/190931, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
When launching a debug session, if the target process exited early (e.g., due to a startup error, non-existent target, or failure before the VM service connects), the debug adapter would deadlock waiting indefinitely for
debuggerInitializedto complete.This change:
waitingForDebuggerand adds adebuggerInitializationFailedCompleterinFlutterBaseDebugAdapter.debuggerInitializationFailedCompleterwith aDebugAdapterExceptioninhandleSessionTerminateifwaitingForDebuggeris active.FlutterTestDebugAdapter.launchImplandFlutterDebugAdapter._handleAppStarted, racesdebuggerInitializedagainstdebuggerInitializationFailedCompleter.futureviaFuture.any, allowing the adapter to fail fast and terminate cleanly without hanging.Fixes #190721
Pre-launch Checklist
///).