-
Notifications
You must be signed in to change notification settings - Fork 86
[DWDS] Fixes hot reload/restart crashes after closing browser tab on web-server device #2699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| _logger.info('No clients available for hot restart'); | ||
| return { | ||
| 'result': { | ||
| 'type': 'Success', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel right as this isn't a Success.
Realistically, we should be doing something like throwing an RPCError with an error code that package:vm_service forwards without modification. Right now, package:vm_service catches all exceptions and wraps them in RPCErrors with a kServerError code, but maybe we should update it to rethrow RPCError instances.
Another option would be to update the hotRestart extension to return ReloadReport as well so we can be consistent between hotReload and hotRestart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! This shouldn’t be treated as a success. I think returning a ReloadReport would be the better approach to keep things consistent and also add the field noClientsAvailable. For now, I’ve removed the type field since it’s not being used for this operation.
In another PR, we can refactor this to return a ReloadReport instead, that would also involve updating the Chrome path and Flutter tools as well, as they currently expect the existing format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that sound? @bkonyi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think we can remove the type property in the result without consequence. package:vm_service checks for this and, if it can't find the type, returns a Response by default. While I don't think anyone is checking for the Success object explicitly since there's no useful information attached to it, this could break things.
I think it's just worth updating the return type to be ReloadReport now and then updating Flutter to return the same when we roll this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I feel like this resurrects the original issue we had where we can't distinguish what the kServerError corresponds to, whereas if we had hot restart return a ReloadReport, we have that metadata in the report.
(also I'll be OOO tomorrow, so if you decide this is the right approach anyways, feel free to land it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Srujan, Ben sand I discussed this and decided that this approach to throw an RPCError would work better since changing hot restart to return a ReloadReport would also require modifying the Chrome path, which could introduce some breakage.
In Flutter tools, I check for the calling method and kServerError to distinguish that this is the specific scenario. We could technically add a new RPCErrorKind for this, but I’m not sure it’s really necessary.
Thoughts? @bkonyi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we'd like to avoid using the generic kServerError, I think we can go ahead and use kIsolateCannotReload to indicate that the reload has failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: dart-lang/sdk#61757
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks Jessy! I'm guessing we can't add a test in DWDS as it requires closing the browser or somehow disrupting the connection which is going to be manual and therefore requires something like the Flutter test runner.
yes that's correct I have added a test in flutter tools for this scenario |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one nit.
…ver device (#177026) This PR fixes crashes during hot reload/restart operations on web-server devices that occur when browser tabs are closed. Previously, the VM service would throw "Bad state: No element" errors when attempting operations with no connected clients. The fix introduces graceful handling on both sides: DWDS now catches NoClientsAvailableException in its hot reload/restart operations and returns structured JSON responses with a noClientsAvailable boolean flag instead of throwing exceptions. Flutter Tools reads this flag via a helper method _checkNoClientsAvailable() and handles the scenario gracefully by displaying "Recompile complete. No client connected." while preserving the DWDS connection to automatically support browser reconnections without requiring a full restart of the Flutter tools. Fixes: #174791 Changes in DWDS (Parent PR): dart-lang/webdev#2699 Follow up bug: dart-lang/sdk#61757
Revisions updated by `dart tools/rev_sdk_deps.dart`. protobuf (https://github.com/dart-lang/protobuf/compare/14bbd0b..78cf743): 78cf743 2025-10-20 Ömer Sinan Ağacan Fix a few extension field bugs (google/protobuf.dart#1062) tools (https://github.com/dart-lang/tools/compare/f5920a2..d0941a3): d0941a35 2025-10-17 Morgan :) Unify watcher tests (dart-lang/tools#2211) fa978cd2 2025-10-17 Morgan :) Remove backup expectations (dart-lang/tools#2209) web (https://github.com/dart-lang/web/compare/816abcc..5a7d0be): 5a7d0be 2025-10-16 Srujan Gaddam Initial commit of package:js_interop 0.1.0-beta (dart-lang/web#476) webdev (https://github.com/dart-lang/webdev/compare/2517aa9..82b3855): 82b38557 2025-10-20 Jessy Yameogo [DWDS] Fixes hot reload/restart crashes after closing browser tab on web-server device (dart-lang/webdev#2699) Change-Id: I98c840892d243f3dc34f9409956d8726ff84238a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/456040 Commit-Queue: Konstantin Shcheglov <scheglov@google.com> Auto-Submit: Devon Carew <devoncarew@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
This PR introduces graceful handling for scenarios where no browser clients are connected to DWDS during hot reload and hot restart operations. Previously, DWDS threw generic
StateErrorexceptions that required Flutter Tools to parse error messages using brittle string matching. Now, a customNoClientsAvailableExceptionis thrown and caught, with structured error responses that include anoClientsAvailableboolean field in the JSON output. This enables type-safe error handling and eliminates the need for error string parsing. The changes affect thereloadSourcesandhotRestartmethods inWebSocketProxyService, which now return success responses with metadata when no clients are available, treating this as a recoverable scenario rather than a fatal error.Related to flutter/flutter#174791
Changes in Flutter tools (Child PR): flutter/flutter#177026
Follow up bug: dart-lang/sdk#61757