Skip to content

Commit eeef313

Browse files
committed
metro-file-map: Watch directories before listing them
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: `inotify` watches are per-directory, and the parent's watch doesn't report a write into a child directory, which I've confirmed holds for `fs.watch` on macOS too. The bounded pool makes the window a scheduling hop rather than a microtask, since other visits interleave with it. 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. Brent diagnosed it and fixed it for Expo's fork in expo/expo#49363. Now that we own the crawl, the fix is to call `dirCallback` before `readdir` instead of after, which also moves it inside `recReaddir`'s existing `try`. Expo's fix has to hoist the watch into `walker`'s `filterDir` hook and give `fs.watch` its own `try`/`catch`, because a throw out of `filterDir` escapes into `walker`'s internals; here an `fs.watch` that throws `ENOENT` for a directory that has since gone is routed to `errorCallback` - `#checkedEmitError`, which drops `ENOENT` - and the crawl skips it rather than descending. The tradeoff is that a directory is now reported before we know it can be listed, so one that vanishes in between is reported and then reported as deleted. That's inherent in watching first, and the delete event comes from the watch we just registered. Not fixed here, both pre-existing and both also addressed by 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.
1 parent eeaffeb commit eeef313

2 files changed

Lines changed: 151 additions & 4 deletions

File tree

packages/metro-file-map/src/watchers/FallbackWatcher.js

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,17 @@ export default class FallbackWatcher extends AbstractWatcher {
182182
if (this.#watched[dir]) {
183183
return false;
184184
}
185-
const watcher = fs.watch(dir, {persistent: true}, (event, filename) =>
186-
this.#normalizeChange(dir, event, filename),
187-
);
185+
let watcher: FSWatcher;
186+
try {
187+
watcher = fs.watch(dir, {persistent: true}, (event, filename) =>
188+
this.#normalizeChange(dir, event, filename),
189+
);
190+
} catch (error) {
191+
// A directory we cannot watch is still worth crawling, so report the
192+
// error and carry on rather than losing the subtree under it.
193+
this.#checkedEmitError(error);
194+
return false;
195+
}
188196
this.#watched[dir] = watcher;
189197

190198
watcher.on('error', this.#checkedEmitError);
@@ -463,12 +471,14 @@ async function recReaddir(
463471
if (ignored != null && common.posixPathMatchesPattern(ignored, entry)) {
464472
return;
465473
}
474+
// Report the directory before listing it. A consumer that starts watching
475+
// here would otherwise miss anything written between the two.
476+
dirCallback(entry, stats);
466477
names = await fsPromises.readdir(entry);
467478
} catch (error) {
468479
errorCallback(error);
469480
return;
470481
}
471-
dirCallback(entry, stats);
472482
for (const name of names) {
473483
pending.push(path.join(entry, name));
474484
}
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @flow strict-local
8+
* @format
9+
* @oncall react_native
10+
*/
11+
12+
import FallbackWatcher from '../FallbackWatcher';
13+
import {createTempWatchRoot} from './helpers';
14+
import fs from 'node:fs';
15+
import {join} from 'node:path';
16+
17+
jest.useRealTimers();
18+
jest.setTimeout(10 * 1000);
19+
20+
const {mkdir, rm, writeFile} = fs.promises;
21+
22+
describe('FallbackWatcher', () => {
23+
let watchRoot: string;
24+
let watcher: ?FallbackWatcher;
25+
let calls: Array<string>;
26+
let watchFailure: ?{code: string, path: string};
27+
28+
const indexOfCall = (op: 'watch' | 'readdir', dir: string) =>
29+
calls.indexOf(`${op}:${dir}`);
30+
31+
const expectWatchedBeforeListed = (dir: string) => {
32+
expect(indexOfCall('watch', dir)).toBeGreaterThanOrEqual(0);
33+
expect(indexOfCall('watch', dir)).toBeLessThan(indexOfCall('readdir', dir));
34+
};
35+
36+
beforeEach(async () => {
37+
watchRoot = await createTempWatchRoot('Fallback', false);
38+
calls = [];
39+
watchFailure = null;
40+
41+
const {watch} = fs;
42+
jest.spyOn(fs, 'watch').mockImplementation((dir, ...args) => {
43+
calls.push(`watch:${String(dir)}`);
44+
const failure = watchFailure;
45+
if (failure != null && dir === failure.path) {
46+
const error = new Error(`Cannot watch path '${String(dir)}'.`);
47+
// $FlowFixMe[prop-missing] code
48+
error.code = failure.code;
49+
throw error;
50+
}
51+
return watch(dir, ...args);
52+
});
53+
const {readdir} = fs.promises;
54+
// $FlowFixMe[incompatible-call] - variadic passthrough
55+
jest.spyOn(fs.promises, 'readdir').mockImplementation((dir, ...args) => {
56+
calls.push(`readdir:${String(dir)}`);
57+
return readdir(dir, ...args);
58+
});
59+
60+
watcher = new FallbackWatcher(watchRoot, {
61+
dot: true,
62+
globs: [],
63+
ignored: null,
64+
watchmanDeferStates: [],
65+
});
66+
});
67+
68+
afterEach(async () => {
69+
await watcher?.stopWatching();
70+
jest.restoreAllMocks();
71+
await rm(watchRoot, {recursive: true});
72+
});
73+
74+
// A file written into a directory after it has been listed but before it is
75+
// watched is reported by neither the listing nor any subsequent event, and is
76+
// missed until the next full crawl. This is how installing a package against
77+
// a running server loses files: https://github.com/expo/expo/issues/48950
78+
describe('watches each directory before listing it', () => {
79+
test('during the initial crawl', async () => {
80+
await mkdir(join(watchRoot, 'a', 'b'), {recursive: true});
81+
82+
await watcher?.startWatching();
83+
84+
for (const dir of ['', 'a', join('a', 'b')]) {
85+
expectWatchedBeforeListed(join(watchRoot, dir));
86+
}
87+
});
88+
89+
test('for a directory created while watching', async () => {
90+
await watcher?.startWatching();
91+
calls = [];
92+
93+
const nested = join(watchRoot, 'new', 'nested');
94+
await mkdir(nested, {recursive: true});
95+
await writeFile(join(nested, 'file.js'), '');
96+
await waitFor(() => indexOfCall('readdir', nested) >= 0);
97+
98+
for (const dir of [join(watchRoot, 'new'), nested]) {
99+
expectWatchedBeforeListed(dir);
100+
}
101+
});
102+
});
103+
104+
// A watch we cannot establish - one directory over the inotify limit, say -
105+
// must not cost us the files under it, which would silently truncate the
106+
// file map.
107+
test.each([
108+
['ENOSPC', 1],
109+
['ENOENT', 0],
110+
])(
111+
'crawls past a directory it cannot watch (%s)',
112+
async (code, expectedErrors) => {
113+
await mkdir(join(watchRoot, 'a', 'b'), {recursive: true});
114+
watchFailure = {code, path: join(watchRoot, 'a')};
115+
const errors: Array<Error> = [];
116+
watcher?.onError(error => {
117+
errors.push(error);
118+
});
119+
120+
await expect(watcher?.startWatching()).resolves.toBeUndefined();
121+
122+
expect(errors).toHaveLength(expectedErrors);
123+
for (const dir of ['a', join('a', 'b')]) {
124+
expect(
125+
indexOfCall('readdir', join(watchRoot, dir)),
126+
).toBeGreaterThanOrEqual(0);
127+
}
128+
},
129+
);
130+
});
131+
132+
async function waitFor(predicate: () => boolean): Promise<void> {
133+
const deadline = Date.now() + 5000;
134+
while (!predicate() && Date.now() < deadline) {
135+
await new Promise(resolve => setTimeout(resolve, 20));
136+
}
137+
}

0 commit comments

Comments
 (0)