Sorry, it took me a while to get to this.

Overall I love the idea of removing delays around hot reload. In terms of the implementation, doing in the daemon seems like the most reasonable approach, since ultimately this is only required for IDE consumption.

In terms of the debounce mechanics - why exactly do we need to debounce instead of say, queue? Is it due to multiple events created from saveAll actions?

In terms of the debounce mechanics - why exactly do we need to debounce instead of say, queue? Is it due to multiple events created from saveAll actions?

Yep! Unfortunately in VS Code we don't have any "Save All" event (request is at microsoft/vscode#86087) so our only options are to either debounce on the client (which we do now) or sent multiple events to Flutter (which we could do if we move this here).

My thinking is that by moving it here:

• it simplifies things on the client (including if multiple reload events come from other sources than save-all, like the user triggering the command, double-clicking buttons, etc.)
• allows Flutter to control the timeout
• if Flutter wanted to do something different (eg. don't debounce (or have a smaller timeout for) the initial request in a batch to reduce latency in the common case where there's only a single event - at the expense of a Save All maybe doing two reloads), that's simpler here

@jonahwilliams discussing reload times reminded me of this - having the debounce here would allow removing it from the editors so you have more control (though this does make Flutter responsible for merging multiple saves from a Save All from an editor like VS code that doesn't have an event for this).

Lets do it!

@jonahwilliams Ok cool! Do you want to scan over the approach here and see if it seems reasonable? It needs some more tests, though I'm not sure how to handle them without adding these delays in the test (since what we're testing is timebased, and an integration tests so we can't easily interfere with the passing of time).

Yeah, I'll set aside some time tomorrow to do a deep dive on this. For testing with timeouts, probably a combination of real integration tests as well as fake async.

Hey, from the IntelliJ perspective:

• we have an existing 125ms delay from when we save all modified files to when we issue a hot reload request to flutter tools; https://github.com/flutter/flutter-intellij/blob/a98fd2526203b3cc89d35d1789186fa489877d7c/src/io/flutter/run/FlutterReloadManager.java#L161
• it turns out that's legacy for when we used to block hot reload based on analysis issues; we no longer do that. we only block on structural issues in the current file, and we don't need an artificial delay in order to do that. I'll remove the IntelliJ 125ms delay

With that change, IntelliJ has two usage patterns:

• listen for save all actions from the user and issue a hit reload request
• react to a 'hot reload' action performed by the user; here, we programmatically save all modified files and issue a save all request

For both usage patterns, IntelliJ will be modifying source files on disk and issuing an immediate hot reload request, w/ no artificial delay. If flutter tools relies solely on file watching to know about changed files, we'll likely have a race condition between when it gets a reload request and when it knows about changed files.

We may want to either 1) have flutter tools poll for file changes before performing a hot reload 2) have flutter tools be in charge of an (100ms?) artificial delay before it performs a hot reload, in order to collect any file watching events.

If flutter tools relies solely on file watching to know about changed files, we'll likely have a race condition between when it gets a reload request and when it knows about changed files.

From some brief trial and error locally, we may not actually have a race condition, or if we do, we're always wining it. I always see my changes active on reload even without an artificial delay.

We don't use file watching in the tool, instead preferring to manually check timestamps after a request. Timestamp granuality isn't great on windows, but as long as the is a ms or two of delay between saving and the request it should be fine

Timestamp granuality isn't great on windows, but as long as the is a ms or two of delay between saving and the request it should be fine

Ooooooh.... slightly off-topic but I've seen a few issues in the past where people said they were seeing stale content after a reload (like the n-1 content was reloaded) and if just occurred to me that this might be related to FAT file times..

The granularity for FAT modified time is a whole two seconds (!). Presumably Flutter is looking at files "modified since the last reload" so it seems like this should work fine as long as the change was flushed to disk before Flutter read it.

However, on a second reload - depending on how the FAT write time is rounded, maybe we might think a save that happened just after the last reload actually happened before it (and therefore was already included)?

So would it makes sense for Flutter to reload any changes made "since two seconds before the last reload"? There's a chance that will include changes that were already included in the previous reload, but it seems better to overlap and include slightly too much than too little? (it's possible this is already done and it's unrelated to the issue, but it felt plausible!).

Well, it seemed plausible (the original case was #32573 btw), but apparently FAT is cleverer(?) than I, as it always rounds up:

https://devblogs.microsoft.com/oldnewthing/20140903-00/?p=83

So probably not the source of that issue!

as long as the change was flushed to disk before Flutter read it

That's probably also not the current source, since both editors have had fairly large (> 100ms) built-in delays up until this point.

Edit: Though this commenter has it only at the terminal (no artificial delay) and not from the editor, so maybe related? 🤷‍♂️

We don't use file watching in the tool, instead preferring to manually check timestamps after a request. Timestamp granuality isn't great on windows, but as long as the is a ms or two of delay between saving and the request it should be fine

OK, good to know, and comforting that there aren't any race conditions.

It looks like IntelliJ won't need debouncing for hot reload requests, so hopefully the debouncing can be an opt-in parameter for clients (or, the debounce period can be a parameter, and will default to 0ms).

@devoncarew

hopefully the debouncing can be an opt-in parameter for clients (or, the debounce period can be a parameter, and will default to 0ms).

It's currently opt-in here. You have to pass debounce: true to get this (otherwise it'll use 0ms). This is to avoid changing anything for clients already doing their own.

@jonahwilliams

It would be nice if debounce logic was structured so that we could get unit test coverage of it.

Yeah, I'll take a look and see what I can come up with. Sorry for the delay!

@jonahwilliams to make it testable, I pulled the implementation out into its own class:

DanTup@8a2e87f

I've written some tests and they pass when running real-time, but I'm stuck on making them work with FakeAsync (I think it's because of this behaviour -> https://stackoverflow.com/questions/62656200/why-does-this-test-using-fakeasync-just-hang-on-the-await-even-though-the-futu). If I can get that working properly, does it seem like a reasonable solution?

Pushed my changes into this PR, including a fix for the unit tests (unsure if there's a better one though). PTAL!

I'm not sure what's up with the analyze bot:

▌10:02:44▐ RUNNING: cd .; bin/flutter analyze --dartdocs --flutter-repo
Package "path_provider_linux" has conflicts:
  1 source wants "/root/.pub-cache/hosted/pub.dartlang.org/path_provider_linux-0.0.1+2/lib":
    /tmp/flutter sdk/dev/benchmarks/macrobenchmarks/.packages
  1 source wants "/root/.pub-cache/hosted/pub.dartlang.org/path_provider_linux-0.0.1+1/lib":
    /tmp/flutter sdk/dev/integration_tests/android_views/.packages
Package "url_launcher_web" has conflicts:
  1 source wants "/root/.pub-cache/hosted/pub.dartlang.org/url_launcher_web-0.1.2/lib":
    /tmp/flutter sdk/dev/benchmarks/macrobenchmarks/.packages
  1 source wants "/root/.pub-cache/hosted/pub.dartlang.org/url_launcher_web-0.1.1+6/lib":
    /tmp/flutter sdk/dev/integration_tests/flutter_gallery/.packages

The path_provider_linux bug was fixed a few days ago, there was a bug in update_packages

I'm temporarily committing with logging turned on to try and debug the failing test on Windows... It's failing in the opposite way to I'd expect from flakes... < 2 hot reloads rather than > 2! May be that + 10ms isn't enough to separate the two sets of requests (in which case I might do * 2!)

I've removed the debug logging and increased the time between the reloads we want to be grouped together, and those that don't. Let me know if there's anything else, or if you're happy for me to land. Thanks!