FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: remove race condition for recursive watch on Linux · nodejs/node@bbd1351 · GitHub

/ node Public

Commit bbd1351

Browse files
mcollina
authored and
richardlau
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 e02dbf0 commit bbd1351

8 files changed

Lines changed: 182 additions & 198 deletions

‎lib/internal/fs/promises.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1098,7 +1098,7 @@ async function* _watch(filename, options = kEmptyObject) {
10981098
// e.g. Linux due to the limitations of inotify.
10991099
if (options.recursive && !isOSX && !isWindows) {
11001100
const watcher = new nonNativeWatcher.FSWatcher(options);
1101-
await watcher[kFSWatchStart](filename);
1101+
watcher[kFSWatchStart](filename);
11021102
yield* watcher;
11031103
return;
11041104
}

‎lib/internal/fs/recursive_watch.js‎

Lines 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
}

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

Lines 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+
});

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

Lines 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)
Back | FazBrowse Home | New Git URL