Race condition hot restarting on-save

[DanTup]

I don't know if this affects Flutter web or not, I see it using the daemon directly. When I modify a HTML file in VS Code we trigger hot-reload-on-save. We wait for 200ms to ensure there are no other saves (so that Save All doesn't trigger lots of reloads) and then send an app.restart to the daemon.

Despite the 200ms delay, it seems that the hot restart is processed before the rebuild, and this results in the browser reloading with the old contents of the HTML file (I guess it's not read directly from the disk, but from an intermediate built location?).

The output looks like this:

Restarted application in 88ms
[INFO] About to build [web]...
[INFO] Updating asset graph...
[INFO] Updating asset graph completed, took 3ms
[INFO] Running build...
[INFO] 1.0s elapsed, 5/5 actions completed.
[INFO] Running build completed, took 1.0s
[INFO] Caching finalized dependency graph...
[INFO] Caching finalized dependency graph completed, took 324ms
[INFO] Succeeded after 1.3s with 8 outputs (7 actions)
[INFO] ------------------------------------------------------------------------

This is running on macOS. I'm not sure where the delay comes from - does webdev do its own debounce/delay on its watcher to also avoid triggering too much? If so, once the first one triggers could it stall processing things like hot reloads (eg. just make it seem like the hot reload took longer, because it waited for the build, then reloaded afterwards)?

[jakemac53]

We do our own debouncing to handle swap files and save all scenarios - so that might be the cause here.

The files are generally cached and served from memory (we would update that cache when they change - but only after the debouncing).

Flutter solves this by not using auto build mode - they use "manual" build mode where they explicitly trigger builds and we don't do any file watching at all.

I don't think we support manual build mode through the daemon protocol - but we likely could.

[DanTup]

I don't think we support manual build mode through the daemon protocol - but we likely could.

If you did this, could you just do the build when we ask for a hot restart (eg. don't introduce a new command we need to send to rebuild, just do include it in the restart/reload)? I think that would match Flutter's behaviour and avoid deviating the daemon protocol (it would also avoid additional builds if a user is not using hot-reload-on-save and makes multiple edits/saves prior to clicking restart).

[jakemac53]

That could be a valid option yes - my main concern would be if the user doesn't have restart on save enabled and they refresh their browser manually expecting changes to have happened, we would still be serving the old state.

[DanTup]

Isn't that a consequence of using manual build rather than whether the build is triggered by the IDE or automatically? That said, auto rebuilds are fine by me as long as the hot restart isn't processed during the debounce (and end up refreshing before the rebuild) :-)

[jakemac53]

Isn't that a consequence of using manual build rather than whether the build is triggered by the IDE or automatically?

Right but you are essentially asking that manual build be the default for daemon mode - which would mean if the IDE launches webdev but the user doesn't have reload on save enabled they would have to manually click reload in the IDE to see any changes - where today a simple browser refresh would also work.

[DanTup]

Right but you are essentially asking that manual build be the default for daemon mode

I don't think that's necessary. You can still auto-build on file saves, as long as when a hot restart comes in if you're in the "debouncing phase" of a build/fs-watcher-triggered-build, you delay processing the hot restart until the build completes (eg. something like put it in a queue if a build is pending, then process the queue after the build).

Manual mode would be more consistent with Flutter - but since this isn't Flutter, I don't know if that's an important consideration.

My only desire is that if you have hot-reload-on-save enabled, you don't see a refresh that shows the same contents, and then a build complete so you need to reload again (that's what currently happens, and it seems like a big bug). I could increase the debounce timer in VS Code to be higher than yours, however that would artificially increase the time for hot restarts and ofcourse the faster we can make them the better.

[jakemac53]

Ah ok - if you don't want to mirror the flutter behavior and still allow eager builds then yes we can block this in a much more general fashion.

We could likely change serve/daemon mode generally so that during the debounce the server starts blocking instead of waiting for a build start like it currently does.

[DanTup]

instead of waiting for a build start like it currently does

I'm not totally sure what that means, but having the server not process incoming messages during the debounce I think should do the job. My 200ms debounce (and going across processes) should ensure your FS watcher has fired well before the hot restart is sent, I think the current issue is just that you process it immediately (while the build is being debounced).

If it's a simple change I can make (or if you can make it on a branch or something), I could do some testing of it locally.

[jakemac53]

The blocking for the daemon happens here https://github.com/dart-lang/build/blob/master/build_runner/lib/src/daemon/asset_server.dart#L30.

Ultimately that future comes from a completer that gets created by by calling_signalStart here.

We would probably want to make that method public, and call it earlier. But it gets complicated because we have to handle the case of an already ongoing build as well.

The actual debouncing also happens here which also makes it a bit more challenging because it is pretty much hiding the debouncing from everything else. It makes sense that it lives there but we would probably need to expose another stream or something which gives an events prior to the debouncing. It could likely just be a Stream<void> since we don't care about the values.