Implemented hot restart over websocket #2666
Conversation
| /// Grace period before destroying isolate when no clients are detected. | ||
| /// This handles the race condition during page refresh where the old connection | ||
| /// closes before the new connection is established, preventing premature isolate destruction. | ||
| const _isolateDestructionGracePeriod = Duration(seconds: 2); |
There was a problem hiding this comment.
Where is the client connection coming from and when does it get sent? Is it sent from the injected client.js running in the browser? I'm concerned about the potential for 2 seconds being too short compared to the 15 second timeout I see in _performWebSocketHotRestart().
There was a problem hiding this comment.
The client connection is sent from the injected client running in the browser. This actually doesn’t impact hot restart, but rather a page refresh operation. I found that in my previous implementation, when there’s only a single window open and we trigger a page refresh, the server might not recognize it as a refresh and could start destroying the isolate immediately since there are no connections for a short period. This change just tells the server to wait an extra 2 seconds, then check if any new connections appear before proceeding to destroy the isolate. From my local testing, 2 seconds seems to work fine, but I can increase it if you think it would make more sense.
There was a problem hiding this comment.
I increased it to 10 seconds instead
@nshahan We do not have a test here but I can add a test in a flutter repo where I use a headless chrome browser similar to what I did for hot reload. we can't really test the websocket channel here without establishing the connection between the injected client and DWDS. |
@jyameo We have these tests already but it is getting harder to understand what scenarios they cover.
@srujzs I'm getting lost in the details of
|
|
My rough thoughts:
Also note that that whole group is skipped when using the frontend server as those tests only make sense for the build daemon. It may be less confusing to pull those out into a separate test that's only run with the build daemon. |
| /// Default implementation throws UnimplementedError. | ||
| /// Override in subclasses that support hot restart completion. | ||
| void completeHotRestart(HotRestartResponse response) { | ||
| throw UnimplementedError('completeHotRestart not supported'); |
There was a problem hiding this comment.
Why not make this method abstract instead?
There was a problem hiding this comment.
Because only WebSocketProxyService implements hot restart completion. The chrome path handles hot restart differently therefore the method is an optional, protocol-specific behavior, not core functionality for all proxy services.
| /// Grace period before destroying isolate when no clients are detected. | ||
| /// This handles the race condition during page refresh where the old connection | ||
| /// closes before the new connection is established, preventing premature isolate destruction. | ||
| const _isolateDestructionGracePeriod = Duration(seconds: 10); |
There was a problem hiding this comment.
Not sure I understand this change correctly but wanted to check;
Is the duration here dependant on how long the client takes to load all JS bundles again? In some of our products we have a remote server of flutter run -d web-server.
Loading from a remote server serveral hundred Flutter debug JS bundles unfortunately takes a long time - consistently more than 10s on average, sometimes minutes on a poor connection - so wanted to check if this change affects this in anyway
There was a problem hiding this comment.
This is NOT related to how long it takes to load JS bundles. Instead, it handles a specific race condition during page refresh so it should NOT negatively affect your remote server setup
@nshahan Will land this PR for now and look into adding a test in a follow up PR |
related to #2605