Support hot reload over websocket

[jyameo]

• Added WebSocket-based hot reload support: reloadSources in ChromeProxyService and DevHandler can now handle hot reload requests and responses over WebSockets (protected by bool flag useWebSocket).
• Refactored the injected client to use a reusable function for handling hot reload requests and responses over WebSockets.

Related to #2605

[srujzs]

Thanks Jessy! This generally looks good % my comment about requests coming in too quick.

[srujzs]

The way the hot reload works in the client is it looks at the hotReloadSourcesUri which contains all the changed files and their libraries and then does an XHR. If whatever app that calls DWDS sends requests too quickly, it may be a possible that a hot reload request reads a version of the file that is no longer the correct version.

For example, the app sends a request and then sets the data in that URI. The app sends a second request with new data in that URI before the first request finishes. Then the first request is reading the new data instead.

[jyameo]

I see. This makes sense. If I add a request/response mechanism for fetchLibrariesForHotReload, similar to how I handle HotReloadRequest with a Completer, would that help resolve the possible race condition you described? Essentially by sending a FetchLibrariesForHotReloadRequest and waiting for its response (using a Completer), we can ensure that the client fetches the correct set of changed libraries before proceeding to the hot reload step. What do you think about this approach?

[srujzs]

I think we want to have the completer wait for both fetchLibrariesForHotReload and hotReload. The former may return the right paths now, but those paths may be replaced with new compiled files before hotReload gets to execute if we don't wait for hotReload as well.

But I believe that should work. This more or less "blocks" the request queue until the previous request is fully finished.

[srujzs]

Sorry Jessy, I may have led you astray here and we should discuss this a bit more before we settle on an implementation.

I don't think the current code makes much of a difference as we just split the requests into two.

Going back to the original code you had, I'm realizing we just await the completer we created to finish before we return from reloadSources. This is a little confusing to me, as that means we're waiting for the hot reload to finish for every request. In that case, we don't really need a map of completers, we could just have one pending completer. There's also no race condition here because there's only one request ever being processed.

I'm assuming this isn't what we want, though, and we do want DWDS to handle multiple requests coming in without blocking (but maybe not, in which case the original code works!). In the Flutter tools case, we always await reloadSources to finish when a reload request comes in. How does the workflow look like for Firebase Studio?

[jyameo]

No worries. Using a single completer instead of a map does make sense here.

In DWDS, the VmServerConnection is the component that receives VM service protocol requests and delegates them to the vmServiceInterface implementation, which in our case is the ChromeProxyService. There, we also await the reloadSources call to complete before moving on. Additionally, the ext.flutter.reassemble service extension is invoked after reloadSources has finished. All the requests are coming from Flutter Tools.

Regarding whether we want to support multiple requests without blocking. I’m not entirely sure. So far, I haven’t run into any race conditions, so it’s unclear if we actually need to handle concurrent requests. That said, it’s something we can keep an eye on as we flesh things out further.

I'm assuming Firebase Studio will behave similarly, but we’re currently unable to test that flow until we complete the implementation of the WebSocketProxyService, which will act as the alternative vmServiceInterface in that environment.

Let me know if you’d like me to revert to the previous code and apply the change to use a single completer instead or we could setup a call next week to discuss this further. Happy to update accordingly 🙂

[srujzs]

So far, any hot reload requests have been blocking with Flutter tools, so we don't really need to worry about multiple requests overlapping (at least within DWDS).

In the future, for Firebase Studio, if we do see that we'd like to be able to handle multiple calls to hot reload within DWDS, then adding back the map makes sense, and we'll have to make sure that the previous request completed before processing a new one by awaiting the previous request's completer before sending a new request over the socket. It looks like that isn't the current case though, so my preference would be to just revert to the code you had earlier as it's simpler and we don't need to separate the two requests. The only change we should have is a single completer instead of a map.

Sorry again for the churn, I saw the map and thought we wanted to have multiple concurrent requests within DWDS.

[jyameo]

No worries at all, this makes sense. I’ll go ahead and make those changes and simplify things as suggested. Thanks for the clarification!

[biggs0125]

Based on the comments above it seems like these both need to happen before we 'ack' the hot reload request. I'm not clear on a couple things:

1. Why do these need to be 2 separate functions if we need to make sure they're called together anyway?
2. Once the future from fetchLibrariesForHotReload completes the files are loaded into the page and we have an ID associated with the request so we can avoid mixing up files. Why do we need to wait for the client to finish applying the hot reload patch (i.e. calling hotReload) to 'ack' the server and unblock it to process further requests?

cc @srujzs

[srujzs]

1. In the non-websocket case, we use the list of libraries to disable breakpoints before we continue the hot reload. Doing this after the full hot reload means we may come across a previous breakpoint (maybe in a captured closure?) before we have the chance to disable all the breakpoints.

Once breakpoints work for hot reload, we'll definitely need this to be two methods so that the debugger can place breakpoints in the newly loaded libraries before they're added to the runtime (between hotReloadStart and hotReloadEnd).

2. the files are loaded into the page

Not yet. This just fetches the list, but doesn't load the files into the page until a later call to hotReload. Once breakpoints work, that will be true though.

and we have an ID associated with the request so we can avoid mixing up files. Why do we need to wait for the client to finish applying the hot reload patch (i.e. calling hotReload) to 'ack' the server and unblock it to process further requests?

We don't pipe that ID to the DDC embedder though. I'm imagining a scenario where the first request fetches the list, maybe even loads the files onto the page, but before we call hotReloadEnd, the second request also fetches its list and loads the files onto the page, essentially clobbering the information in the embedder that we need for the first hotReloadEnd. I believe that's possible due to the async boundaries. We could have an ID associated with the request within the embedder and it could do some of that state management, but that doesn't exist today. We rate-limited hot restart requests in the past (_hotRestartRunIdCache), so we may be able to do something similar.

[biggs0125]

Okay, from experience so far it seems like hot reloads tend to occur fast enough. So sequentializing the hot reloads like this is probably okay. I'm curious if that'll still be the case with more network heavy workflows like those involving Firebase Studio. We should keep an eye on this to make sure the experience remains smooth for developers.

[srujzs]

LGTM, thanks Jessy!

[biggs0125]

LGTM!