Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 15a7f21

Browse filesBrowse files
mcollinatargos
mcollina
authored and
targos
committed
fs: remove race condition for recursive watch on Linux
Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #51406 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
1 parent d8e1058 commit 15a7f21
Copy full SHA for 15a7f21
Expand file treeCollapse file tree

8 files changed

+182
-198
lines changed
Open diff view settings
Collapse file

‎lib/internal/fs/promises.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/promises.js
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1255,7 +1255,7 @@ async function* _watch(filename, options = kEmptyObject) {
12551255
// e.g. Linux due to the limitations of inotify.
12561256
if (options.recursive && !isOSX && !isWindows) {
12571257
const watcher = new nonNativeWatcher.FSWatcher(options);
1258-
await watcher[kFSWatchStart](filename);
1258+
watcher[kFSWatchStart](filename);
12591259
yield* watcher;
12601260
return;
12611261
}
Collapse file

‎lib/internal/fs/recursive_watch.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/recursive_watch.js
+44-69Lines changed: 44 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
'use strict';
22

33
const {
4-
ArrayPrototypePush,
5-
SafePromiseAllReturnVoid,
64
Promise,
7-
PromisePrototypeThen,
85
SafeMap,
96
SafeSet,
107
StringPrototypeStartsWith,
@@ -31,47 +28,19 @@ const {
3128
} = require('path');
3229

3330
let internalSync;
34-
let internalPromises;
35-
36-
function lazyLoadFsPromises() {
37-
internalPromises ??= require('fs/promises');
38-
return internalPromises;
39-
}
4031

4132
function lazyLoadFsSync() {
4233
internalSync ??= require('fs');
4334
return internalSync;
4435
}
45-
let kResistStopPropagation;
46-
47-
async function traverse(dir, files = new SafeMap(), symbolicLinks = new SafeSet()) {
48-
const { opendir } = lazyLoadFsPromises();
49-
50-
const filenames = await opendir(dir);
51-
const subdirectories = [];
52-
53-
for await (const file of filenames) {
54-
const f = pathJoin(dir, file.name);
55-
56-
files.set(f, file);
57-
58-
// Do not follow symbolic links
59-
if (file.isSymbolicLink()) {
60-
symbolicLinks.add(f);
61-
} else if (file.isDirectory()) {
62-
ArrayPrototypePush(subdirectories, traverse(f, files));
63-
}
64-
}
65-
66-
await SafePromiseAllReturnVoid(subdirectories);
6736

68-
return files;
69-
}
37+
let kResistStopPropagation;
7038

7139
class FSWatcher extends EventEmitter {
7240
#options = null;
7341
#closed = false;
7442
#files = new SafeMap();
43+
#watchers = new SafeMap();
7544
#symbolicFiles = new SafeSet();
7645
#rootPath = pathResolve();
7746
#watchingFile = false;
@@ -111,11 +80,11 @@ class FSWatcher extends EventEmitter {
11180
return;
11281
}
11382

114-
const { unwatchFile } = lazyLoadFsSync();
11583
this.#closed = true;
11684

11785
for (const file of this.#files.keys()) {
118-
unwatchFile(file);
86+
this.#watchers.get(file).close();
87+
this.#watchers.delete(file);
11988
}
12089

12190
this.#files.clear();
@@ -124,24 +93,26 @@ class FSWatcher extends EventEmitter {
12493
}
12594

12695
#unwatchFiles(file) {
127-
const { unwatchFile } = lazyLoadFsSync();
128-
12996
this.#symbolicFiles.delete(file);
13097

13198
for (const filename of this.#files.keys()) {
13299
if (StringPrototypeStartsWith(filename, file)) {
133-
unwatchFile(filename);
100+
this.#files.delete(filename);
101+
this.#watchers.get(filename).close();
102+
this.#watchers.delete(filename);
134103
}
135104
}
136105
}
137106

138-
async #watchFolder(folder) {
139-
const { opendir } = lazyLoadFsPromises();
107+
#watchFolder(folder) {
108+
const { readdirSync } = lazyLoadFsSync();
140109

141110
try {
142-
const files = await opendir(folder);
111+
const files = readdirSync(folder, {
112+
withFileTypes: true,
113+
});
143114

144-
for await (const file of files) {
115+
for (const file of files) {
145116
if (this.#closed) {
146117
break;
147118
}
@@ -155,11 +126,9 @@ class FSWatcher extends EventEmitter {
155126
this.#symbolicFiles.add(f);
156127
}
157128

158-
this.#files.set(f, file);
159-
if (file.isFile()) {
160-
this.#watchFile(f);
161-
} else if (file.isDirectory() && !file.isSymbolicLink()) {
162-
await this.#watchFolder(f);
129+
this.#watchFile(f);
130+
if (file.isDirectory() && !file.isSymbolicLink()) {
131+
this.#watchFolder(f);
163132
}
164133
}
165134
}
@@ -173,22 +142,30 @@ class FSWatcher extends EventEmitter {
173142
return;
174143
}
175144

176-
const { watchFile } = lazyLoadFsSync();
177-
const existingStat = this.#files.get(file);
145+
const { watch, statSync } = lazyLoadFsSync();
146+
147+
if (this.#files.has(file)) {
148+
return;
149+
}
150+
151+
{
152+
const existingStat = statSync(file);
153+
this.#files.set(file, existingStat);
154+
}
178155

179-
watchFile(file, {
156+
const watcher = watch(file, {
180157
persistent: this.#options.persistent,
181-
}, (currentStats, previousStats) => {
182-
if (existingStat && !existingStat.isDirectory() &&
183-
currentStats.nlink !== 0 && existingStat.mtimeMs === currentStats.mtimeMs) {
184-
return;
185-
}
158+
}, (eventType, filename) => {
159+
const existingStat = this.#files.get(file);
160+
const currentStats = statSync(file);
186161

187162
this.#files.set(file, currentStats);
188163

189-
if (currentStats.birthtimeMs === 0 && previousStats.birthtimeMs !== 0) {
164+
if (currentStats.birthtimeMs === 0 && existingStat.birthtimeMs !== 0) {
190165
// The file is now deleted
191166
this.#files.delete(file);
167+
this.#watchers.delete(file);
168+
watcher.close();
192169
this.emit('change', 'rename', pathRelative(this.#rootPath, file));
193170
this.#unwatchFiles(file);
194171
} else if (file === this.#rootPath && this.#watchingFile) {
@@ -205,6 +182,7 @@ class FSWatcher extends EventEmitter {
205182
this.emit('change', 'change', pathRelative(this.#rootPath, file));
206183
}
207184
});
185+
this.#watchers.set(file, watcher);
208186
}
209187

210188
[kFSWatchStart](filename) {
@@ -217,19 +195,9 @@ class FSWatcher extends EventEmitter {
217195
this.#closed = false;
218196
this.#watchingFile = file.isFile();
219197

198+
this.#watchFile(filename);
220199
if (file.isDirectory()) {
221-
this.#files.set(filename, file);
222-
223-
PromisePrototypeThen(
224-
traverse(filename, this.#files, this.#symbolicFiles),
225-
() => {
226-
for (const f of this.#files.keys()) {
227-
this.#watchFile(f);
228-
}
229-
},
230-
);
231-
} else {
232-
this.#watchFile(filename);
200+
this.#watchFolder(filename);
233201
}
234202
} catch (error) {
235203
if (error.code === 'ENOENT') {
@@ -264,7 +232,10 @@ class FSWatcher extends EventEmitter {
264232
resolve({ __proto__: null, value: { eventType, filename } });
265233
});
266234
} : (resolve, reject) => {
267-
const onAbort = () => reject(new AbortError(undefined, { cause: signal.reason }));
235+
const onAbort = () => {
236+
this.close();
237+
reject(new AbortError(undefined, { cause: signal.reason }));
238+
};
268239
if (signal.aborted) return onAbort();
269240
kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
270241
signal.addEventListener('abort', onAbort, { __proto__: null, once: true, [kResistStopPropagation]: true });
@@ -277,6 +248,10 @@ class FSWatcher extends EventEmitter {
277248
next: () => (this.#closed ?
278249
{ __proto__: null, done: true } :
279250
new Promise(promiseExecutor)),
251+
return: () => {
252+
this.close();
253+
return { __proto__: null, done: true };
254+
},
280255
[SymbolAsyncIterator]() { return this; },
281256
};
282257
}
Collapse file

‎test/parallel/test-fs-watch-recursive-add-file-to-existing-subfolder.js‎

Copy file name to clipboard
+25-29Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
'use strict';
22

33
const common = require('../common');
4-
const { setTimeout } = require('timers/promises');
54

65
if (common.isIBMi)
76
common.skip('IBMi does not support `fs.watch()`');
@@ -21,39 +20,36 @@ const tmpdir = require('../common/tmpdir');
2120
const testDir = tmpdir.path;
2221
tmpdir.refresh();
2322

24-
(async () => {
25-
// Add a file to subfolder of a watching folder
23+
// Add a file to subfolder of a watching folder
2624

27-
const rootDirectory = fs.mkdtempSync(testDir + path.sep);
28-
const testDirectory = path.join(rootDirectory, 'test-4');
29-
fs.mkdirSync(testDirectory);
25+
const rootDirectory = fs.mkdtempSync(testDir + path.sep);
26+
const testDirectory = path.join(rootDirectory, 'test-4');
27+
fs.mkdirSync(testDirectory);
3028

31-
const file = 'folder-5';
32-
const filePath = path.join(testDirectory, file);
33-
fs.mkdirSync(filePath);
29+
const file = 'folder-5';
30+
const filePath = path.join(testDirectory, file);
31+
fs.mkdirSync(filePath);
3432

35-
const subfolderPath = path.join(filePath, 'subfolder-6');
36-
fs.mkdirSync(subfolderPath);
33+
const subfolderPath = path.join(filePath, 'subfolder-6');
34+
fs.mkdirSync(subfolderPath);
3735

38-
const childrenFile = 'file-7.txt';
39-
const childrenAbsolutePath = path.join(subfolderPath, childrenFile);
40-
const relativePath = path.join(file, path.basename(subfolderPath), childrenFile);
36+
const childrenFile = 'file-7.txt';
37+
const childrenAbsolutePath = path.join(subfolderPath, childrenFile);
38+
const relativePath = path.join(file, path.basename(subfolderPath), childrenFile);
4139

42-
const watcher = fs.watch(testDirectory, { recursive: true });
43-
let watcherClosed = false;
44-
watcher.on('change', function(event, filename) {
45-
assert.strictEqual(event, 'rename');
40+
const watcher = fs.watch(testDirectory, { recursive: true });
41+
let watcherClosed = false;
42+
watcher.on('change', function(event, filename) {
43+
assert.strictEqual(event, 'rename');
4644

47-
if (filename === relativePath) {
48-
watcher.close();
49-
watcherClosed = true;
50-
}
51-
});
45+
if (filename === relativePath) {
46+
watcher.close();
47+
watcherClosed = true;
48+
}
49+
});
5250

53-
await setTimeout(common.platformTimeout(100));
54-
fs.writeFileSync(childrenAbsolutePath, 'world');
51+
fs.writeFileSync(childrenAbsolutePath, 'world');
5552

56-
process.once('exit', function() {
57-
assert(watcherClosed, 'watcher Object was not closed');
58-
});
59-
})().then(common.mustCall());
53+
process.once('exit', function() {
54+
assert(watcherClosed, 'watcher Object was not closed');
55+
});
Collapse file

‎test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js‎

Copy file name to clipboard
+23-28Lines changed: 23 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
'use strict';
22

33
const common = require('../common');
4-
const { setTimeout } = require('timers/promises');
54

65
if (common.isIBMi)
76
common.skip('IBMi does not support `fs.watch()`');
@@ -21,37 +20,33 @@ const tmpdir = require('../common/tmpdir');
2120
const testDir = tmpdir.path;
2221
tmpdir.refresh();
2322

24-
(async () => {
25-
// Add a file to newly created folder to already watching folder
23+
// Add a file to newly created folder to already watching folder
2624

27-
const rootDirectory = fs.mkdtempSync(testDir + path.sep);
28-
const testDirectory = path.join(rootDirectory, 'test-3');
29-
fs.mkdirSync(testDirectory);
25+
const rootDirectory = fs.mkdtempSync(testDir + path.sep);
26+
const testDirectory = path.join(rootDirectory, 'test-3');
27+
fs.mkdirSync(testDirectory);
3028

31-
const filePath = path.join(testDirectory, 'folder-3');
29+
const filePath = path.join(testDirectory, 'folder-3');
3230

33-
const childrenFile = 'file-4.txt';
34-
const childrenAbsolutePath = path.join(filePath, childrenFile);
35-
const childrenRelativePath = path.join(path.basename(filePath), childrenFile);
31+
const childrenFile = 'file-4.txt';
32+
const childrenAbsolutePath = path.join(filePath, childrenFile);
33+
const childrenRelativePath = path.join(path.basename(filePath), childrenFile);
3634

37-
const watcher = fs.watch(testDirectory, { recursive: true });
38-
let watcherClosed = false;
39-
watcher.on('change', function(event, filename) {
40-
assert.strictEqual(event, 'rename');
41-
assert.ok(filename === path.basename(filePath) || filename === childrenRelativePath);
35+
const watcher = fs.watch(testDirectory, { recursive: true });
36+
let watcherClosed = false;
37+
watcher.on('change', function(event, filename) {
38+
assert.strictEqual(event, 'rename');
39+
assert.ok(filename === path.basename(filePath) || filename === childrenRelativePath);
4240

43-
if (filename === childrenRelativePath) {
44-
watcher.close();
45-
watcherClosed = true;
46-
}
47-
});
41+
if (filename === childrenRelativePath) {
42+
watcher.close();
43+
watcherClosed = true;
44+
}
45+
});
4846

49-
await setTimeout(common.platformTimeout(100));
50-
fs.mkdirSync(filePath);
51-
await setTimeout(common.platformTimeout(100));
52-
fs.writeFileSync(childrenAbsolutePath, 'world');
47+
fs.mkdirSync(filePath);
48+
fs.writeFileSync(childrenAbsolutePath, 'world');
5349

54-
process.once('exit', function() {
55-
assert(watcherClosed, 'watcher Object was not closed');
56-
});
57-
})().then(common.mustCall());
50+
process.once('exit', function() {
51+
assert(watcherClosed, 'watcher Object was not closed');
52+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.