Skip to content
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

watch: Debounce all files updates into a single event. #51988

Closed
wants to merge 5 commits into from

Conversation

matthieusieben
Copy link
Contributor

@matthieusieben matthieusieben commented Mar 6, 2024

Partially fixes #51954 by reducing the amount of changed events in watch mode.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 6, 2024
@matthieusieben
Copy link
Contributor Author

@MoLow what do you think about this one ?

@MoLow
Copy link
Member

MoLow commented Mar 7, 2024

As to my understanding, the issue described in #51954 (comment) isn't caused by the lack of debounce, but from a race condition where change events are fired between killing the process and restarting it.
adding another debounce mechanism might make this condition more rare than it currently is, but it won't solve it

@matthieusieben
Copy link
Contributor Author

matthieusieben commented Mar 8, 2024

There are two problems described (although admittedly not very clearly) in issue #51954:

The first one is tackled by your PR #51992

The second problem, which also causes too many un-necessary restarts, is due to the fact that the current behavior does not act as a "debounce" but actually acts more as a "delayed throttle".

Here is the definition of Debounce and Throttle

  • Debounce will wait that a full period (X ms) without any update (for a particular file) before firing the event.
  • Throttle will emit changed events (for a particular file) as soon as they occur, but not more often than every X ms

The current behavior is a mix between these two modes: The File watcher will wait X ms before emitting the first event, and will then continue emitting events (if the same file keep being updated) every X ms.

In practice, this means that if the same file gets updated every 100ms, the changed event will be fired every 200ms (1 out of 2 events will be dropped). So the watcher will try and restart the app every 200ms (except for events occurring during restart).

The issue is even worse when multiple files are concerned. Indeed, consider the following scenario (e.g. during a build or git checkout):

  1. file A gets updated ( + 0ms) -- event debounced
  2. file B gets updated ( + 100ms ) -- event debounced
  3. file C gets updated ( + 200ms ) -- event debounced
  4. changed A emitted ( + 201ms )
  5. App is killed
  6. file D gets updated ( + 300ms ) -- event debounced
  7. changed B emitted ( + 301ms ) -- now ignored thanks to watch: batch file restarts #51992
  8. App gracefully exitted
  9. App starts (at this point all files are up-to-date)
  10. changed C emitted ( + 401ms )
  11. App restarts -- due to changed event from C
  12. changed D emitted ( + 501ms ) -- might get ignored if happens during restart
  13. App restarts

As you can see in this example, steps 10 to 13 yield un-necessary restarts. How many un-necessary restarts will depend on the number of file updated, the frequence at which they are updated, and the time the app takes to gracefully exit. But it is safe to say that updating files faster than the expected debounce rate (200ms) causes more restarts than needed. I would expect that only files updated slower than every 200 ms trigger multiple updates, not when updates are fasters than 200 ms.

This is probably the main reason why tests are "flacky". Part of the flackyness is due to the behavior of the watcher and not to IO / performance reasons.

// Unfortunately testing that changesCount === 2 is flaky
assert.ok(changesCount < 5);

This PR fixes that by:

  1. actually implementing a debounce instead of a trickle
  2. group (and debounce) all updated together in a single event (instead of one event per file)

@matthieusieben
Copy link
Contributor Author

matthieusieben commented Mar 8, 2024

Grouping all updated in a single event is something that was also suggested by @atlowChemi here

@MoLow
Copy link
Member

MoLow commented Mar 9, 2024

Wouldn't this be a simpler fix with the same effect? #51986

this.emit('changed', { owners });
this.#debounceOwners = new SafeSet();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.#debounceOwners = new SafeSet();
this.#debounceOwners.clear();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I did that is to allow async processing of the owners by the listeners. I figured it would be more efficient to swap the Set rather than doing:

      const owners = new Set(this.#debounceOwners);
      this.emit('changed', { owners });
      this.#debounceOwners.clear();

What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think that async processing of the event is not something we should care about here, then I can just implement your change.

@matthieusieben
Copy link
Contributor Author

matthieusieben commented Mar 11, 2024

Hello @MoLow,

The changes you suggested in #51986 have several issues:

  1. The set for individual changes (this.#debouncing) is no longer needed at all in that implementation, as noted by @atlowChemi here
  2. This implementation will not emit the event with all the right owners since some events (e.g. file A updated) are completely ignored if a timer was already started (e.g. by file B).
  3. That implementation still "trickles" instead of debouncing.

@matthieusieben
Copy link
Contributor Author

Hey guys, any feedback you'd like to share on this ?
cc: @MoLow @atlowChemi @anonrig

@matthieusieben
Copy link
Contributor Author

matthieusieben commented Mar 19, 2024

It might be worth noting that one of the reason why it may have appeared as a single issue before, is that when trying to provide a solution (eg. in main...matthieusieben:node:patch-2), I provided an implementation that fixed both underlying causes by wrapping the changed event using a single "debouncer". But there were indeed two issues (an there is still this one) as explained (here and detailed here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

watch should debounce the restart
4 participants