Skip to content

Commit

Permalink
NodeWatcher - fix scan / add race, minimise recursive watch delay
Browse files Browse the repository at this point in the history 
Summary:
This addresses some inconsistencies and bugs in the `NodeWatcher` (`fs.watch`-based fallback for non-Watchman, non-macOS), so we can improve test strictness ahead of other changes.

## Scan/'add' race
As noted in the deleted test workarounds, there was previously a race when creating a subtree between
 1. The scan of a subtree's contents in when processing the 'add' event for a root.
2.  The 'add' event fired at or under the root for a new file or directory, if a watch has already been established.

This could result in multiple add events emitted for a given file or directory.

This diff makes some simple changes to ensure that an add event is only emitted for previously-unwatched directories or previously-unregistered files (`_watchdir` and `_register` return booleans).

## Watch initialisation delays
A distinct but related issue occurred when a file was deleted from a new directory - the deletion may be missed if a watch hasn't yet been established on a new directory.

This turned out to be at least in part because we perform the recursive watch by intercepting directory addition events *within* a debounce wrapper (ie, after an explicit delay).

This diff moves the traversal to begin immediately on processing a directory addition.

Reviewed By: jacdebug

Differential Revision: D41713791

fbshipit-source-id: 714870799f3819a07e7cd96a6d8c87328aaa9ca6
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Dec 13, 2022
1 parent 528c891 commit 51fb7e3
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 79 deletions.
138 changes: 77 additions & 61 deletions packages/metro-file-map/src/watchers/NodeWatcher.js
View file
Expand Up @@ -25,14 +25,20 @@ const fs = require('fs');
const platform = require('os').platform();
const path = require('path');

const DEFAULT_DELAY = common.DEFAULT_DELAY;
const CHANGE_EVENT = common.CHANGE_EVENT;
const DELETE_EVENT = common.DELETE_EVENT;
const ADD_EVENT = common.ADD_EVENT;
const ALL_EVENT = common.ALL_EVENT;

/**
* This setting delays all events. It suppresses 'change' events that
* immediately follow an 'add', and debounces successive 'change' events to
* only emit the latest.
*/
const DEBOUNCE_MS = 100;

module.exports = class NodeWatcher extends EventEmitter {
_changeTimers: {[key: string]: TimeoutID, __proto__: null};
_changeTimers: Map<string, TimeoutID> = new Map();
_dirRegistry: {
[directory: string]: {[file: string]: true, __proto__: null},
__proto__: null,
Expand All @@ -52,14 +58,15 @@ module.exports = class NodeWatcher extends EventEmitter {
common.assignOptions(this, opts);

this.watched = Object.create(null);
this._changeTimers = Object.create(null);
this._dirRegistry = Object.create(null);
this.root = path.resolve(dir);

this._watchdir(this.root);
common.recReaddir(
this.root,
this._watchdir,
dir => {
this._watchdir(dir);
},
filename => {
this._register(filename);
},
Expand All @@ -82,21 +89,27 @@ module.exports = class NodeWatcher extends EventEmitter {
* filename => true
* }
* }
*
* Return false if ignored or already registered.
*/
_register(filepath: string): boolean {
const dir = path.dirname(filepath);
const filename = path.basename(filepath);
if (this._dirRegistry[dir] && this._dirRegistry[dir][filename]) {
return false;
}

const relativePath = path.relative(this.root, filepath);
if (
!common.isFileIncluded(this.globs, this.dot, this.doIgnore, relativePath)
) {
return false;
}

const dir = path.dirname(filepath);
if (!this._dirRegistry[dir]) {
this._dirRegistry[dir] = Object.create(null);
}

const filename = path.basename(filepath);
this._dirRegistry[dir][filename] = true;

return true;
Expand Down Expand Up @@ -146,11 +159,10 @@ module.exports = class NodeWatcher extends EventEmitter {
/**
* Watch a directory.
*/
_watchdir: string => void = (dir: string) => {
_watchdir: string => boolean = (dir: string) => {
if (this.watched[dir]) {
return;
return false;
}

const watcher = fs.watch(dir, {persistent: true}, (event, filename) =>
this._normalizeChange(dir, event, filename),
);
Expand All @@ -161,6 +173,7 @@ module.exports = class NodeWatcher extends EventEmitter {
if (this.root !== dir) {
this._register(dir);
}
return true;
};

/**
Expand Down Expand Up @@ -249,24 +262,43 @@ module.exports = class NodeWatcher extends EventEmitter {
if (error && error.code !== 'ENOENT') {
this.emit('error', error);
} else if (!error && stat.isDirectory()) {
// win32 emits usless change events on dirs.
if (event !== 'change') {
this._watchdir(fullPath);
if (
stat &&
common.isFileIncluded(
this.globs,
this.dot,
this.doIgnore,
relativePath,
)
) {
this._emitEvent(ADD_EVENT, relativePath, {
modifiedTime: stat.mtime.getTime(),
size: stat.size,
type: 'd',
});
}
if (event === 'change') {
// win32 emits usless change events on dirs.
return;
}
if (
stat &&
common.isFileIncluded(
this.globs,
this.dot,
this.doIgnore,
relativePath,
)
) {
common.recReaddir(
path.resolve(this.root, relativePath),
(dir, stats) => {
if (this._watchdir(dir)) {
this._emitEvent(ADD_EVENT, path.relative(this.root, dir), {
modifiedTime: stats.mtime.getTime(),
size: stats.size,
type: 'd',
});
}
},
(file, stats) => {
if (this._register(file)) {
this._emitEvent(ADD_EVENT, path.relative(this.root, file), {
modifiedTime: stats.mtime.getTime(),
size: stats.size,
type: 'f',
});
}
},
function endCallback() {},
this._checkedEmitError,
this.ignored,
);
}
} else {
const registered = this._registered(fullPath);
Expand Down Expand Up @@ -300,48 +332,31 @@ module.exports = class NodeWatcher extends EventEmitter {
}

/**
* Triggers a 'change' event after debounding it to take care of duplicate
* events on os x.
* Emits the given event after debouncing, to 1) suppress 'change' events
* immediately following an 'add', and 2) to only emit the latest 'change'
* event when received in quick succession for a given file.
*
* See also note above for DEBOUNCE_MS.
*/
_emitEvent(type: string, file: string, metadata?: ChangeEventMetadata) {
const key = type + '-' + file;
const addKey = ADD_EVENT + '-' + file;
if (type === CHANGE_EVENT && this._changeTimers[addKey]) {
if (type === CHANGE_EVENT && this._changeTimers.has(addKey)) {
// Ignore the change event that is immediately fired after an add event.
// (This happens on Linux).
return;
}
clearTimeout(this._changeTimers[key]);
this._changeTimers[key] = setTimeout(() => {
delete this._changeTimers[key];
if (type === ADD_EVENT && metadata?.type === 'd') {
// Recursively emit add events and watch for sub-files/folders
common.recReaddir(
path.resolve(this.root, file),
(dir, stats) => {
this._watchdir(dir);
this._rawEmitEvent(ADD_EVENT, path.relative(this.root, dir), {
modifiedTime: stats.mtime.getTime(),
size: stats.size,
type: 'd',
});
},
(file, stats) => {
this._register(file);
this._rawEmitEvent(ADD_EVENT, path.relative(this.root, file), {
modifiedTime: stats.mtime.getTime(),
size: stats.size,
type: 'f',
});
},
function endCallback() {},
this._checkedEmitError,
this.ignored,
);
} else {
const existingTimer = this._changeTimers.get(key);
if (existingTimer) {
clearTimeout(existingTimer);
}
this._changeTimers.set(
key,
setTimeout(() => {
this._changeTimers.delete(key);
this._rawEmitEvent(type, file, metadata);
}
}, DEFAULT_DELAY);
}, DEBOUNCE_MS),
);
}

/**
Expand All @@ -366,7 +381,8 @@ module.exports = class NodeWatcher extends EventEmitter {
function isIgnorableFileError(error: Error | {code: string}) {
return (
error.code === 'ENOENT' ||
// Workaround Windows node issue #4337.
// Workaround Windows EPERM on watched folder deletion.
// https://github.com/nodejs/node-v0.x-archive/issues/4337
(error.code === 'EPERM' && platform === 'win32')
);
}
20 changes: 3 additions & 17 deletions packages/metro-file-map/src/watchers/__tests__/integration-test.js
View file
Expand Up @@ -104,7 +104,7 @@ describe.each(Object.keys(WATCHERS))(
// Even though we write it before initialising watchers, OS-level
// delays/debouncing(?) can mean the write is *sometimes* reported by
// the watcher.
ignored: /\.watchmanconfig/,
ignored: /(\.watchmanconfig|ignored-)/,
watchmanDeferStates: [],
};

Expand Down Expand Up @@ -267,6 +267,7 @@ describe.each(Object.keys(WATCHERS))(
async () => {
await mkdir(join(appRoot, 'subdir', 'subdir2'), {recursive: true});
await Promise.all([
writeFile(join(appRoot, 'subdir', 'ignored-file.js'), ''),
writeFile(join(appRoot, 'subdir', 'deep.js'), ''),
writeFile(join(appRoot, 'subdir', 'subdir2', 'deeper.js'), ''),
]);
Expand All @@ -277,24 +278,9 @@ describe.each(Object.keys(WATCHERS))(
[join('app', 'subdir', 'deep.js'), 'add'],
[join('app', 'subdir', 'subdir2', 'deeper.js'), 'add'],
],
{
// FIXME: NodeWatcher may report events multiple times as it
// establishes watches on new directories and then crawls them
// recursively, emitting all contents. When a directory is created
// then immediately populated, the new contents may be seen by both
// the crawl and the watch.
rejectUnexpected: !(watcherInstance instanceof NodeWatcher),
},
{rejectUnexpected: true},
);

// FIXME: Because NodeWatcher recursively watches new subtrees and
// watch initialization is not instantaneous, we need to allow some
// time for NodeWatcher to watch the new directories, othwerwise
// deletion events may be missed.
if (watcherInstance instanceof NodeWatcher) {
await new Promise(resolve => setTimeout(resolve, 200));
}

await allEvents(
async () => {
await rm(join(appRoot, 'subdir'), {recursive: true});
Expand Down
1 change: 0 additions & 1 deletion packages/metro-file-map/src/watchers/common.js
View file
Expand Up @@ -31,7 +31,6 @@ const walker = require('walker');
/**
* Constants
*/
export const DEFAULT_DELAY = 100;
export const CHANGE_EVENT = 'change';
export const DELETE_EVENT = 'delete';
export const ADD_EVENT = 'add';
Expand Down

0 comments on commit 51fb7e3

Please sign in to comment.