From 9254e601f1e28067be3e9dd1c540f897bd8b5709 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 25 Jan 2024 09:53:21 +0100 Subject: [PATCH] fs: remove race condition for recursive watch on Linux Signed-off-by: Matteo Collina PR-URL: https://github.com/nodejs/node/pull/51406 Reviewed-By: Yagiz Nizipli Reviewed-By: Marco Ippolito Reviewed-By: Moshe Atlow --- lib/internal/fs/promises.js | 2 +- lib/internal/fs/recursive_watch.js | 113 +++++++----------- ...ecursive-add-file-to-existing-subfolder.js | 54 ++++----- ...-watch-recursive-add-file-to-new-folder.js | 51 ++++---- .../test-fs-watch-recursive-add-file.js | 40 +++---- .../test-fs-watch-recursive-assert-leaks.js | 45 ++++--- .../test-fs-watch-recursive-sync-write.js | 35 ++++++ .../test-fs-watch-recursive-update-file.js | 40 +++---- 8 files changed, 182 insertions(+), 198 deletions(-) create mode 100644 test/parallel/test-fs-watch-recursive-sync-write.js diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index bdefea99edfe3c..7317f29bc5393d 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -1098,7 +1098,7 @@ async function* _watch(filename, options = kEmptyObject) { // e.g. Linux due to the limitations of inotify. if (options.recursive && !isOSX && !isWindows) { const watcher = new nonNativeWatcher.FSWatcher(options); - await watcher[kFSWatchStart](filename); + watcher[kFSWatchStart](filename); yield* watcher; return; } diff --git a/lib/internal/fs/recursive_watch.js b/lib/internal/fs/recursive_watch.js index 54298832da5a1b..7d8b12eeb93445 100644 --- a/lib/internal/fs/recursive_watch.js +++ b/lib/internal/fs/recursive_watch.js @@ -1,10 +1,7 @@ 'use strict'; const { - ArrayPrototypePush, - SafePromiseAllReturnVoid, Promise, - PromisePrototypeThen, SafeMap, SafeSet, StringPrototypeStartsWith, @@ -31,47 +28,19 @@ const { } = require('path'); let internalSync; -let internalPromises; - -function lazyLoadFsPromises() { - internalPromises ??= require('fs/promises'); - return internalPromises; -} function lazyLoadFsSync() { internalSync ??= require('fs'); return internalSync; } -let kResistStopPropagation; - -async function traverse(dir, files = new SafeMap(), symbolicLinks = new SafeSet()) { - const { opendir } = lazyLoadFsPromises(); - - const filenames = await opendir(dir); - const subdirectories = []; - - for await (const file of filenames) { - const f = pathJoin(dir, file.name); - - files.set(f, file); - - // Do not follow symbolic links - if (file.isSymbolicLink()) { - symbolicLinks.add(f); - } else if (file.isDirectory()) { - ArrayPrototypePush(subdirectories, traverse(f, files)); - } - } - - await SafePromiseAllReturnVoid(subdirectories); - return files; -} +let kResistStopPropagation; class FSWatcher extends EventEmitter { #options = null; #closed = false; #files = new SafeMap(); + #watchers = new SafeMap(); #symbolicFiles = new SafeSet(); #rootPath = pathResolve(); #watchingFile = false; @@ -111,11 +80,11 @@ class FSWatcher extends EventEmitter { return; } - const { unwatchFile } = lazyLoadFsSync(); this.#closed = true; for (const file of this.#files.keys()) { - unwatchFile(file); + this.#watchers.get(file).close(); + this.#watchers.delete(file); } this.#files.clear(); @@ -124,24 +93,26 @@ class FSWatcher extends EventEmitter { } #unwatchFiles(file) { - const { unwatchFile } = lazyLoadFsSync(); - this.#symbolicFiles.delete(file); for (const filename of this.#files.keys()) { if (StringPrototypeStartsWith(filename, file)) { - unwatchFile(filename); + this.#files.delete(filename); + this.#watchers.get(filename).close(); + this.#watchers.delete(filename); } } } - async #watchFolder(folder) { - const { opendir } = lazyLoadFsPromises(); + #watchFolder(folder) { + const { readdirSync } = lazyLoadFsSync(); try { - const files = await opendir(folder); + const files = readdirSync(folder, { + withFileTypes: true, + }); - for await (const file of files) { + for (const file of files) { if (this.#closed) { break; } @@ -155,11 +126,9 @@ class FSWatcher extends EventEmitter { this.#symbolicFiles.add(f); } - this.#files.set(f, file); - if (file.isFile()) { - this.#watchFile(f); - } else if (file.isDirectory() && !file.isSymbolicLink()) { - await this.#watchFolder(f); + this.#watchFile(f); + if (file.isDirectory() && !file.isSymbolicLink()) { + this.#watchFolder(f); } } } @@ -173,22 +142,30 @@ class FSWatcher extends EventEmitter { return; } - const { watchFile } = lazyLoadFsSync(); - const existingStat = this.#files.get(file); + const { watch, statSync } = lazyLoadFsSync(); + + if (this.#files.has(file)) { + return; + } + + { + const existingStat = statSync(file); + this.#files.set(file, existingStat); + } - watchFile(file, { + const watcher = watch(file, { persistent: this.#options.persistent, - }, (currentStats, previousStats) => { - if (existingStat && !existingStat.isDirectory() && - currentStats.nlink !== 0 && existingStat.mtimeMs === currentStats.mtimeMs) { - return; - } + }, (eventType, filename) => { + const existingStat = this.#files.get(file); + const currentStats = statSync(file); this.#files.set(file, currentStats); - if (currentStats.birthtimeMs === 0 && previousStats.birthtimeMs !== 0) { + if (currentStats.birthtimeMs === 0 && existingStat.birthtimeMs !== 0) { // The file is now deleted this.#files.delete(file); + this.#watchers.delete(file); + watcher.close(); this.emit('change', 'rename', pathRelative(this.#rootPath, file)); this.#unwatchFiles(file); } else if (file === this.#rootPath && this.#watchingFile) { @@ -205,6 +182,7 @@ class FSWatcher extends EventEmitter { this.emit('change', 'change', pathRelative(this.#rootPath, file)); } }); + this.#watchers.set(file, watcher); } [kFSWatchStart](filename) { @@ -217,19 +195,9 @@ class FSWatcher extends EventEmitter { this.#closed = false; this.#watchingFile = file.isFile(); + this.#watchFile(filename); if (file.isDirectory()) { - this.#files.set(filename, file); - - PromisePrototypeThen( - traverse(filename, this.#files, this.#symbolicFiles), - () => { - for (const f of this.#files.keys()) { - this.#watchFile(f); - } - }, - ); - } else { - this.#watchFile(filename); + this.#watchFolder(filename); } } catch (error) { if (error.code === 'ENOENT') { @@ -264,7 +232,10 @@ class FSWatcher extends EventEmitter { resolve({ __proto__: null, value: { eventType, filename } }); }); } : (resolve, reject) => { - const onAbort = () => reject(new AbortError(undefined, { cause: signal.reason })); + const onAbort = () => { + this.close(); + reject(new AbortError(undefined, { cause: signal.reason })); + }; if (signal.aborted) return onAbort(); kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation; signal.addEventListener('abort', onAbort, { __proto__: null, once: true, [kResistStopPropagation]: true }); @@ -277,6 +248,10 @@ class FSWatcher extends EventEmitter { next: () => (this.#closed ? { __proto__: null, done: true } : new Promise(promiseExecutor)), + return: () => { + this.close(); + return { __proto__: null, done: true }; + }, [SymbolAsyncIterator]() { return this; }, }; } diff --git a/test/parallel/test-fs-watch-recursive-add-file-to-existing-subfolder.js b/test/parallel/test-fs-watch-recursive-add-file-to-existing-subfolder.js index 5563dc6a525958..995c82743e49ea 100644 --- a/test/parallel/test-fs-watch-recursive-add-file-to-existing-subfolder.js +++ b/test/parallel/test-fs-watch-recursive-add-file-to-existing-subfolder.js @@ -1,7 +1,6 @@ 'use strict'; const common = require('../common'); -const { setTimeout } = require('timers/promises'); if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); @@ -21,39 +20,36 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // Add a file to subfolder of a watching folder +// Add a file to subfolder of a watching folder - const rootDirectory = fs.mkdtempSync(testDir + path.sep); - const testDirectory = path.join(rootDirectory, 'test-4'); - fs.mkdirSync(testDirectory); +const rootDirectory = fs.mkdtempSync(testDir + path.sep); +const testDirectory = path.join(rootDirectory, 'test-4'); +fs.mkdirSync(testDirectory); - const file = 'folder-5'; - const filePath = path.join(testDirectory, file); - fs.mkdirSync(filePath); +const file = 'folder-5'; +const filePath = path.join(testDirectory, file); +fs.mkdirSync(filePath); - const subfolderPath = path.join(filePath, 'subfolder-6'); - fs.mkdirSync(subfolderPath); +const subfolderPath = path.join(filePath, 'subfolder-6'); +fs.mkdirSync(subfolderPath); - const childrenFile = 'file-7.txt'; - const childrenAbsolutePath = path.join(subfolderPath, childrenFile); - const relativePath = path.join(file, path.basename(subfolderPath), childrenFile); +const childrenFile = 'file-7.txt'; +const childrenAbsolutePath = path.join(subfolderPath, childrenFile); +const relativePath = path.join(file, path.basename(subfolderPath), childrenFile); - const watcher = fs.watch(testDirectory, { recursive: true }); - let watcherClosed = false; - watcher.on('change', function(event, filename) { - assert.strictEqual(event, 'rename'); +const watcher = fs.watch(testDirectory, { recursive: true }); +let watcherClosed = false; +watcher.on('change', function(event, filename) { + assert.strictEqual(event, 'rename'); - if (filename === relativePath) { - watcher.close(); - watcherClosed = true; - } - }); + if (filename === relativePath) { + watcher.close(); + watcherClosed = true; + } +}); - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(childrenAbsolutePath, 'world'); +fs.writeFileSync(childrenAbsolutePath, 'world'); - process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +process.once('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); +}); diff --git a/test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js b/test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js index 9b74cd281b62ec..1d5f0098428c03 100644 --- a/test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js +++ b/test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js @@ -1,7 +1,6 @@ 'use strict'; const common = require('../common'); -const { setTimeout } = require('timers/promises'); if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); @@ -21,37 +20,33 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // Add a file to newly created folder to already watching folder +// Add a file to newly created folder to already watching folder - const rootDirectory = fs.mkdtempSync(testDir + path.sep); - const testDirectory = path.join(rootDirectory, 'test-3'); - fs.mkdirSync(testDirectory); +const rootDirectory = fs.mkdtempSync(testDir + path.sep); +const testDirectory = path.join(rootDirectory, 'test-3'); +fs.mkdirSync(testDirectory); - const filePath = path.join(testDirectory, 'folder-3'); +const filePath = path.join(testDirectory, 'folder-3'); - const childrenFile = 'file-4.txt'; - const childrenAbsolutePath = path.join(filePath, childrenFile); - const childrenRelativePath = path.join(path.basename(filePath), childrenFile); +const childrenFile = 'file-4.txt'; +const childrenAbsolutePath = path.join(filePath, childrenFile); +const childrenRelativePath = path.join(path.basename(filePath), childrenFile); - const watcher = fs.watch(testDirectory, { recursive: true }); - let watcherClosed = false; - watcher.on('change', function(event, filename) { - assert.strictEqual(event, 'rename'); - assert.ok(filename === path.basename(filePath) || filename === childrenRelativePath); +const watcher = fs.watch(testDirectory, { recursive: true }); +let watcherClosed = false; +watcher.on('change', function(event, filename) { + assert.strictEqual(event, 'rename'); + assert.ok(filename === path.basename(filePath) || filename === childrenRelativePath); - if (filename === childrenRelativePath) { - watcher.close(); - watcherClosed = true; - } - }); + if (filename === childrenRelativePath) { + watcher.close(); + watcherClosed = true; + } +}); - await setTimeout(common.platformTimeout(100)); - fs.mkdirSync(filePath); - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(childrenAbsolutePath, 'world'); +fs.mkdirSync(filePath); +fs.writeFileSync(childrenAbsolutePath, 'world'); - process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +process.once('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); +}); diff --git a/test/parallel/test-fs-watch-recursive-add-file.js b/test/parallel/test-fs-watch-recursive-add-file.js index d23d417cfaa410..d03a4144ac81bb 100644 --- a/test/parallel/test-fs-watch-recursive-add-file.js +++ b/test/parallel/test-fs-watch-recursive-add-file.js @@ -1,7 +1,6 @@ 'use strict'; const common = require('../common'); -const { setTimeout } = require('timers/promises'); if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); @@ -21,30 +20,27 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // Add a file to already watching folder +// Add a file to already watching folder - const rootDirectory = fs.mkdtempSync(testDir + path.sep); - const testDirectory = path.join(rootDirectory, 'test-1'); - fs.mkdirSync(testDirectory); +const rootDirectory = fs.mkdtempSync(testDir + path.sep); +const testDirectory = path.join(rootDirectory, 'test-1'); +fs.mkdirSync(testDirectory); - const testFile = path.join(testDirectory, 'file-1.txt'); +const testFile = path.join(testDirectory, 'file-1.txt'); - const watcher = fs.watch(testDirectory, { recursive: true }); - let watcherClosed = false; - watcher.on('change', function(event, filename) { - assert.strictEqual(event, 'rename'); +const watcher = fs.watch(testDirectory, { recursive: true }); +let watcherClosed = false; +watcher.on('change', function(event, filename) { + assert.strictEqual(event, 'rename'); - if (filename === path.basename(testFile)) { - watcher.close(); - watcherClosed = true; - } - }); + if (filename === path.basename(testFile)) { + watcher.close(); + watcherClosed = true; + } +}); - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(testFile, 'world'); +fs.writeFileSync(testFile, 'world'); - process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +process.once('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); +}); diff --git a/test/parallel/test-fs-watch-recursive-assert-leaks.js b/test/parallel/test-fs-watch-recursive-assert-leaks.js index ac2010cfb26376..9d178fcfe8212b 100644 --- a/test/parallel/test-fs-watch-recursive-assert-leaks.js +++ b/test/parallel/test-fs-watch-recursive-assert-leaks.js @@ -21,28 +21,25 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // Assert recursive watch does not leak handles - const rootDirectory = fs.mkdtempSync(testDir + path.sep); - const testDirectory = path.join(rootDirectory, 'test-7'); - const filePath = path.join(testDirectory, 'only-file.txt'); - fs.mkdirSync(testDirectory); - - let watcherClosed = false; - const watcher = fs.watch(testDirectory, { recursive: true }); - watcher.on('change', common.mustCallAtLeast(async (event, filename) => { - await setTimeout(common.platformTimeout(100)); - if (filename === path.basename(filePath)) { - watcher.close(); - watcherClosed = true; - } - await setTimeout(common.platformTimeout(100)); - assert(!process._getActiveHandles().some((handle) => handle.constructor.name === 'StatWatcher')); - })); - - process.on('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); +// Assert recursive watch does not leak handles +const rootDirectory = fs.mkdtempSync(testDir + path.sep); +const testDirectory = path.join(rootDirectory, 'test-7'); +const filePath = path.join(testDirectory, 'only-file.txt'); +fs.mkdirSync(testDirectory); + +let watcherClosed = false; +const watcher = fs.watch(testDirectory, { recursive: true }); +watcher.on('change', common.mustCallAtLeast(async (event, filename) => { await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(filePath, 'content'); -})().then(common.mustCall()); + if (filename === path.basename(filePath)) { + watcher.close(); + watcherClosed = true; + } + await setTimeout(common.platformTimeout(100)); + assert(!process._getActiveHandles().some((handle) => handle.constructor.name === 'StatWatcher')); +})); + +process.on('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); +}); +fs.writeFileSync(filePath, 'content'); diff --git a/test/parallel/test-fs-watch-recursive-sync-write.js b/test/parallel/test-fs-watch-recursive-sync-write.js new file mode 100644 index 00000000000000..38dce82fb115aa --- /dev/null +++ b/test/parallel/test-fs-watch-recursive-sync-write.js @@ -0,0 +1,35 @@ +'use strict'; + +const common = require('../common'); +const { watch, writeFileSync } = require('node:fs'); +const { join } = require('node:path'); +const tmpdir = require('../common/tmpdir.js'); +const assert = require('assert'); + +if (common.isIBMi) + common.skip('IBMi does not support `fs.watch()`'); + +// fs-watch on folders have limited capability in AIX. +// The testcase makes use of folder watching, and causes +// hang. This behavior is documented. Skip this for AIX. + +if (common.isAIX) + common.skip('folder watch capability is limited in AIX.'); + +tmpdir.refresh(); + +const tmpDir = tmpdir.path; +const filename = join(tmpDir, 'test.file'); + +const keepalive = setTimeout(() => { + throw new Error('timed out'); +}, common.platformTimeout(30_000)); + +const watcher = watch(tmpDir, { recursive: true }, common.mustCall((eventType, _filename) => { + clearTimeout(keepalive); + watcher.close(); + assert.strictEqual(eventType, 'rename'); + assert.strictEqual(join(tmpDir, _filename), filename); +})); + +writeFileSync(filename, 'foobar2'); diff --git a/test/parallel/test-fs-watch-recursive-update-file.js b/test/parallel/test-fs-watch-recursive-update-file.js index 57d3bffc7a92b0..ee8e8fe52b4374 100644 --- a/test/parallel/test-fs-watch-recursive-update-file.js +++ b/test/parallel/test-fs-watch-recursive-update-file.js @@ -1,7 +1,6 @@ 'use strict'; const common = require('../common'); -const { setTimeout } = require('timers/promises'); if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); @@ -21,32 +20,23 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // Watch a folder and update an already existing file in it. +// Watch a folder and update an already existing file in it. - const rootDirectory = fs.mkdtempSync(testDir + path.sep); - const testDirectory = path.join(rootDirectory, 'test-0'); - fs.mkdirSync(testDirectory); +const rootDirectory = fs.mkdtempSync(testDir + path.sep); +const testDirectory = path.join(rootDirectory, 'test-0'); +fs.mkdirSync(testDirectory); - const testFile = path.join(testDirectory, 'file-1.txt'); - fs.writeFileSync(testFile, 'hello'); +const testFile = path.join(testDirectory, 'file-1.txt'); +fs.writeFileSync(testFile, 'hello'); - const watcher = fs.watch(testDirectory, { recursive: true }); - let watcherClosed = false; - watcher.on('change', common.mustCallAtLeast(function(event, filename) { - // Libuv inconsistenly emits a rename event for the file we are watching - assert.ok(event === 'change' || event === 'rename'); +const watcher = fs.watch(testDirectory, { recursive: true }); +watcher.on('change', common.mustCallAtLeast(function(event, filename) { + // Libuv inconsistenly emits a rename event for the file we are watching + assert.ok(event === 'change' || event === 'rename'); - if (filename === path.basename(testFile)) { - watcher.close(); - watcherClosed = true; - } - })); + if (filename === path.basename(testFile)) { + watcher.close(); + } +})); - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(testFile, 'hello'); - - process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +fs.writeFileSync(testFile, 'hello');