Conversation
DanTup
added
is enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5948 +/- ##
==========================================
+ Coverage 67.35% 67.36% +0.01%
==========================================
Files 169 169
Lines 13014 13018 +4
Branches 2577 2577
==========================================
+ Hits 8765 8769 +4
- Misses 3787 3788 +1
+ Partials 462 461 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DanTup
force-pushed
the
add-daemon-startup-delay
branch
from
ab4ab82 to
f787521
Compare
…ce.enable has completed I noticed in the logs that we can call functions before `device.enable` completes. It's a long shot, but I wonder if it might be what leads to the timeouts in #5793. We've had issues in the past where sending requests too early would cause issues, which lead to the `daemonStarted` promise, however it only awaited for _any_ output message and not for `device.enable` to complete. This adds a new completer for `device.enable` completing, and then an additional 2s delay. If the timeouts reduce with this change, it might help narrow down the ultimate cause in the `flutter` tool.
DanTup
force-pushed
the
add-daemon-startup-delay
branch
from
f787521 to
8ff7ab4
Compare
|
@codex review /gemini review |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to prevent race conditions by ensuring getEmulators and getSupportedPlatforms are not called before device.enable has completed. It achieves this by introducing a new daemonInitialized state that completes after device.enable and an additional delay. The changes are logical and well-structured. My review includes a suggestion to simplify the new delay logic for better clarity and to clean up some related test code.
![chatgpt-codex-connector[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f6d5d366d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce Flutter daemon timeout/hang scenarios by preventing early getEmulators/getSupportedPlatforms requests from being sent before device.enable has completed (plus an additional 2s delay), to help diagnose/mitigate timeouts like those tracked in #5793.
Changes:
- Remove the public
daemonStartedpromise fromIFlutterDaemonand stop awaiting it inFlutterDeviceManager. - Add internal gating in
FlutterDaemonsogetEmulators()/getSupportedPlatforms()wait until afterdevice.enablecompletes and a fixed 2s delay elapses. - Update the device manager tests’ fake daemon to reflect the interface change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/extension/flutter/flutter_daemon.ts | Introduces daemon “responsive” vs “initialized” gating; delays emulator/platform requests until after device.enable + 2s. |
| src/shared/vscode/device_manager.ts | Removes explicit waiting for daemon “started” before calling daemon APIs. |
| src/shared/interfaces.ts | Removes daemonStarted from IFlutterDaemon. |
| src/test/flutter/device_manager.test.ts | Updates test fake daemon to align with the daemon interface change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public getEmulators(): Thenable<f.FlutterEmulator[]> { | ||
| public async getEmulators(): Promise<f.FlutterEmulator[]> { | ||
| await this.daemonInitialized; |
There was a problem hiding this comment.
getEmulators() awaits this.daemonInitialized with no fallback/timeout. If daemon.connected never arrives or device.enable never settles (or the daemon exits early), this promise never resolves and callers (eg. FlutterDeviceManager constructor) can hang indefinitely. Consider racing initialization with a timeout or process-exit/dispose signal (resolving/rejecting the completer) so emulator requests can fail fast and still be covered by existing timeout/error reporting.
| await this.daemonInitialized; | |
| await withTimeout( | |
| this.daemonInitialized, | |
| "Flutter daemon did not initialize in time to retrieve emulators", | |
| twentySecondsInMs, | |
| ); |
| public async getSupportedPlatforms(projectRoot: string): Promise<f.SupportedPlatformsResponse> { | ||
| await this.daemonInitialized; | ||
| return this.withRecordedTimeout("daemon.getSupportedPlatforms", this.sendRequest("daemon.getSupportedPlatforms", { projectRoot })); | ||
| } |
There was a problem hiding this comment.
getSupportedPlatforms() also blocks on this.daemonInitialized before entering withRecordedTimeout(...), so if initialization never completes these calls will never time out/log (and any upstream withTimeout(...) only times out the caller, not the underlying work). Consider ensuring daemonInitialized is always completed (resolve/reject) on daemon exit/error and/or wrapping the initialization wait in a bounded timeout.
| // Delay marking as initialized for a few seconds to see whether it helps prevent | ||
| // the first emulator requests from hanging. | ||
| // https://github.com/Dart-Code/Dart-Code/issues/5793 | ||
| const completer = this.daemonInitializedCompleter; | ||
| async function markInitializedAfterDelay() { | ||
| setTimeout(() => completer.resolve(), 2000); | ||
| } | ||
|
|
||
| void this.deviceEnable().then(markInitializedAfterDelay, markInitializedAfterDelay); |
There was a problem hiding this comment.
daemonInitializedCompleter is only resolved after deviceEnable() settles, then an additional setTimeout delay. If device.enable hangs (or the daemon fails before sending daemon.connected), daemonInitialized will never complete and can permanently block emulator/platform requests. Consider adding a maximum wait (timeout) and completing the completer on daemon exit/dispose, and using the existing delay(...) helper instead of an async function that schedules a raw setTimeout for clarity.
| public supportedPlatforms: f.PlatformType[] | undefined; | ||
| public supportedPlatformsDelaySeconds: number | undefined; | ||
| public daemonStarted = Promise.resolve(); | ||
| public daemonInitialized = Promise.resolve(); |
There was a problem hiding this comment.
daemonInitialized on FakeFlutterDaemon is not referenced anywhere in this test file and is no longer required to satisfy IFlutterDaemon (since daemonStarted was removed). Consider removing it to avoid confusion about how initialization is supposed to work in tests.
| public daemonInitialized = Promise.resolve(); |
Some tweaks from review on #5948 from after it merged.
I noticed in the logs that we can call functions before
device.enablecompletes. It's a long shot, but I wonder if it might be what leads to the timeouts in #5793. We've had issues in the past where sending requests too early would cause issues, which lead to thedaemonStartedpromise, however it only awaited for any output message and not fordevice.enableto complete.This adds a new completer for
device.enablecompleting, and then an additional 2s delay. If the timeouts reduce with this change, it might help narrow down the ultimate cause in thefluttertool.