Conversation
Collaborator
Author
|
NB: This is stacked on #1906. I assume shipit doesn't support stacks (?), but this should automatically rebase against main and be importable once D119076649 lands. |
Summary: `FallbackWatcher` starts an `fs.watch` on each directory from `recReaddir`'s `dirCallback`, and the crawl calls that after `readdir` has returned. Anything written into a directory *between the two* appears in neither the listing nor the watch, so it stays invisible until the next full crawl. There's no recovery on Linux or Windows. This is expo/expo#48950 - `npx expo install` against a running dev server leaves the new modules unresolvable until restart, which is a lot more painful than it sounds now that agents routinely run Metro in a VM. @brentvatne diagnosed it in expo/expo#49363, with a fix in Expo's fork that we can hopefully replace with this one. Now that we own the crawl (#1906), the fix is very simply to call `dirCallback` before `readdir` instead of after, which also moves it inside `recReaddir`'s existing `try`. Separate issues not fixed here, both pre-existing and both covered by expo/expo#49363 : - An `fs.watch` that emits `error` is never removed from `#watched`. Node emits no `close` after `error`, so the path can never be re-watched, and `#stopWatching` waits on a `close` that will not arrive. - On win32, `fs.watch` can report an event with no filename. `#detectChangedFile` drops it when `#dirRegistry[dir]` is empty, which is exactly the state a newly watched directory is in. Changelog: ``` - **[Fix]**: `FallbackWatcher` no longer misses files written to a directory while it is being crawled ``` Test Plan: ``` yarn jest packages/metro-file-map yarn flow check yarn lint ``` New `watchers/__tests__/FallbackWatcher-test.js` asserts the ordering directly, that `fs.watch` precedes `readdir` for the same directory, on the initial crawl and on a directory created while watching, plus that a directory whose `fs.watch` throws is skipped without failing the crawl. All three fail on the parent commit and pass here. The race can't be asserted behaviourally on macOS, because FSEvents delivers with a latency window and a watch started immediately after a write still reports it: ``` $ node -e "fs.writeFileSync(d+'/raced.js', ''); fs.watch(d, (e, f) => console.log(e, f))" rename raced.js ``` That's why the issue is Linux/Windows only, and why the assertion is on the ordering rather than on a missed event. Metro has no Windows coverage for this backend, so the win32 path is unexercised either way.
Collaborator
Author
|
(Rebasing wasn't automatic because GH sees a ship-it merged PR as closed and assumes the stack is broken. Anyway, did it the old-fashioned way) |
Collaborator
Author
|
@javache / @GijsWeterings / @vzaidman this is quite a high impact fix, it turns out. Metro on Linux outside Meta (eg, agentic dev) gets in a real mess during package installs without this. Meta doesn't use FallbackWatcher so there should be no risk there. Thanks 馃檹 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
FallbackWatcherstarts anfs.watchon each directory fromrecReaddir'sdirCallback, and the crawl calls that afterreaddirhas returned.Anything written into a directory between the two appears in neither the listing nor the watch, so it stays invisible until the next full crawl. There's no recovery on Linux or Windows.
This is expo/expo#48950 -
npx expo installagainst a running dev server leaves the new modules unresolvable until restart, which is a lot more painful than it sounds now that agents routinely run Metro in a VM. @brentvatne diagnosed it in expo/expo#49363, with a fix in Expo's fork that we can hopefully replace with this one.Now that we own the crawl (#1906), the fix is very simply to call
dirCallbackbeforereaddirinstead of after, which also moves it insiderecReaddir's existingtry.Separate issues not fixed here, both pre-existing and both covered by expo/expo#49363 :
fs.watchthat emitserroris never removed from#watched. Node emits nocloseaftererror, so the path can never be re-watched, and#stopWatchingwaits on aclosethat will not arrive.fs.watchcan report an event with no filename.#detectChangedFiledrops it when#dirRegistry[dir]is empty, which is exactly the state a newly watched directory is in.Changelog:
Test Plan:
New
watchers/__tests__/FallbackWatcher-test.jsasserts the ordering directly, thatfs.watchprecedesreaddirfor the same directory, on the initial crawl and on a directory created while watching, plus that a directory whosefs.watchthrows is skipped without failing the crawl. All three fail on the parent commit and pass here.The race can't be asserted behaviourally on macOS, because FSEvents delivers with a latency window and a watch started immediately after a write still reports it:
That's why the issue is Linux/Windows only, and why the assertion is on the ordering rather than on a missed event. Metro has no Windows coverage for this backend, so the win32 path is unexercised either way.