@MoLow what do you think about this one ?

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

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

• changes during restart watch should debounce the restart #51954 (comment)
• incorrect debounce behaviour

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.

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)

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

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

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.

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

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)