From 99d7b737bff82c2e16dc13b9004c6ad7c782f083 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 8 Jan 2024 19:07:59 +0100 Subject: [PATCH 1/8] fs: remove race condition for recursive watch on Linux Signed-off-by: Matteo Collina --- lib/internal/fs/promises.js | 2 +- lib/internal/fs/recursive_watch.js | 57 ++++--------------- test/parallel/test-fs-recursive-sync-write.js | 24 ++++++++ 3 files changed, 37 insertions(+), 46 deletions(-) create mode 100644 test/parallel/test-fs-recursive-sync-write.js diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 42364c78c667cb..45a3a6fdfe6dc8 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -1254,7 +1254,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..800cb59fcc7ad6 100644 --- a/lib/internal/fs/recursive_watch.js +++ b/lib/internal/fs/recursive_watch.js @@ -3,8 +3,6 @@ const { ArrayPrototypePush, SafePromiseAllReturnVoid, - Promise, - PromisePrototypeThen, SafeMap, SafeSet, StringPrototypeStartsWith, @@ -31,42 +29,13 @@ 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; @@ -135,13 +104,15 @@ class FSWatcher extends EventEmitter { } } - 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; } @@ -159,7 +130,7 @@ class FSWatcher extends EventEmitter { if (file.isFile()) { this.#watchFile(f); } else if (file.isDirectory() && !file.isSymbolicLink()) { - await this.#watchFolder(f); + this.#watchFolder(f); } } } @@ -220,14 +191,10 @@ class FSWatcher extends EventEmitter { 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); - } - }, - ); + this.#watchFolder(filename); + for (const f of this.#files.keys()) { + this.#watchFile(f); + } } else { this.#watchFile(filename); } diff --git a/test/parallel/test-fs-recursive-sync-write.js b/test/parallel/test-fs-recursive-sync-write.js new file mode 100644 index 00000000000000..d9b880dc40da36 --- /dev/null +++ b/test/parallel/test-fs-recursive-sync-write.js @@ -0,0 +1,24 @@ +'use strict'; + +const common = require('../common'); +const { mkdtempSync, watch, writeFileSync } = require('node:fs'); +const { tmpdir } = require('node:os'); +const { join } = require('node:path'); +const {platformTimeout} = require('../common'); +const tmpDir = mkdtempSync(join(tmpdir(), 'repro-test-')); +const filename = join(tmpDir, 'test.file'); +const assert = require('assert'); + +const keepalive = setTimeout(() => { + throw new Error('timed out'); +}, common.platformTimeout(30_0000)); + +const watcher = watch(tmpDir, { recursive: true }, common.mustCall((eventType, _filename) => { + clearTimeout(keepalive); + watcher.close(); + assert.strictEqual(eventType, 'rename'); + assert.strictEqual(join(tmpDir, _filename), filename); + console.log(eventType, filename); +})); + +writeFileSync(filename, 'foobar2'); From 0101f447aad7f62dccd7a3aa53441b26e8b1a6be Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 8 Jan 2024 19:14:31 +0100 Subject: [PATCH 2/8] fixup Signed-off-by: Matteo Collina --- lib/internal/fs/recursive_watch.js | 5 ++--- test/parallel/test-fs-recursive-sync-write.js | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/internal/fs/recursive_watch.js b/lib/internal/fs/recursive_watch.js index 800cb59fcc7ad6..779a387b2a5e0a 100644 --- a/lib/internal/fs/recursive_watch.js +++ b/lib/internal/fs/recursive_watch.js @@ -1,8 +1,7 @@ 'use strict'; const { - ArrayPrototypePush, - SafePromiseAllReturnVoid, + Promise, SafeMap, SafeSet, StringPrototypeStartsWith, @@ -109,7 +108,7 @@ class FSWatcher extends EventEmitter { try { const files = readdirSync(folder, { - withFileTypes: true + withFileTypes: true, }); for (const file of files) { diff --git a/test/parallel/test-fs-recursive-sync-write.js b/test/parallel/test-fs-recursive-sync-write.js index d9b880dc40da36..47cd4d8570c8b0 100644 --- a/test/parallel/test-fs-recursive-sync-write.js +++ b/test/parallel/test-fs-recursive-sync-write.js @@ -4,7 +4,6 @@ const common = require('../common'); const { mkdtempSync, watch, writeFileSync } = require('node:fs'); const { tmpdir } = require('node:os'); const { join } = require('node:path'); -const {platformTimeout} = require('../common'); const tmpDir = mkdtempSync(join(tmpdir(), 'repro-test-')); const filename = join(tmpDir, 'test.file'); const assert = require('assert'); From 4586010723d2743705d84f18f3f2fe25cae393d8 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Jan 2024 17:53:50 +0100 Subject: [PATCH 3/8] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-recursive-sync-write.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-fs-recursive-sync-write.js b/test/parallel/test-fs-recursive-sync-write.js index 47cd4d8570c8b0..c3ed44f829c613 100644 --- a/test/parallel/test-fs-recursive-sync-write.js +++ b/test/parallel/test-fs-recursive-sync-write.js @@ -2,22 +2,24 @@ const common = require('../common'); const { mkdtempSync, watch, writeFileSync } = require('node:fs'); -const { tmpdir } = require('node:os'); const { join } = require('node:path'); -const tmpDir = mkdtempSync(join(tmpdir(), 'repro-test-')); -const filename = join(tmpDir, 'test.file'); +const tmpdir = require('../common/tmpdir.js'); const assert = require('assert'); +tmpdir.refresh(); + +const tmpDir = tmpdir.path; +const filename = join(tmpDir, 'test.file'); + const keepalive = setTimeout(() => { throw new Error('timed out'); -}, common.platformTimeout(30_0000)); +}, 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); - console.log(eventType, filename); })); writeFileSync(filename, 'foobar2'); From ff16eca723e769fb7752dd51e34842e8c87ada1a Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Jan 2024 17:56:53 +0100 Subject: [PATCH 4/8] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-recursive-sync-write.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-recursive-sync-write.js b/test/parallel/test-fs-recursive-sync-write.js index c3ed44f829c613..0b3a19355893f9 100644 --- a/test/parallel/test-fs-recursive-sync-write.js +++ b/test/parallel/test-fs-recursive-sync-write.js @@ -1,7 +1,7 @@ 'use strict'; const common = require('../common'); -const { mkdtempSync, watch, writeFileSync } = require('node:fs'); +const { watch, writeFileSync } = require('node:fs'); const { join } = require('node:path'); const tmpdir = require('../common/tmpdir.js'); const assert = require('assert'); From 63d8245bb4796b82f78bfe83e1774dcbb33666d7 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Jan 2024 18:10:38 +0100 Subject: [PATCH 5/8] update old tests Signed-off-by: Matteo Collina --- test/parallel/test-fs-recursive-sync-write.js | 14 ++++ ...ecursive-add-file-to-existing-subfolder.js | 69 +++++++++---------- ...-watch-recursive-add-file-to-new-folder.js | 64 ++++++++--------- ...st-fs-watch-recursive-add-file-with-url.js | 53 +++++++------- .../test-fs-watch-recursive-add-file.js | 39 +++++------ .../test-fs-watch-recursive-add-folder.js | 43 +++++------- .../test-fs-watch-recursive-assert-leaks.js | 45 ++++++------ .../test-fs-watch-recursive-symlink.js | 11 ++- .../test-fs-watch-recursive-update-file.js | 51 ++++++-------- 9 files changed, 185 insertions(+), 204 deletions(-) diff --git a/test/parallel/test-fs-recursive-sync-write.js b/test/parallel/test-fs-recursive-sync-write.js index 0b3a19355893f9..ab21735bc4196a 100644 --- a/test/parallel/test-fs-recursive-sync-write.js +++ b/test/parallel/test-fs-recursive-sync-write.js @@ -6,6 +6,16 @@ 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; @@ -23,3 +33,7 @@ const watcher = watch(tmpDir, { recursive: true }, common.mustCall((eventType, _ })); writeFileSync(filename, 'foobar2'); + +process.once('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); +}); 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..df8d01f06b985d 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 @@ -21,39 +21,36 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // 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 file = 'folder-5'; - const filePath = path.join(testDirectory, file); - fs.mkdirSync(filePath); - - 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 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; - } - }); - - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(childrenAbsolutePath, 'world'); - - process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +// 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 file = 'folder-5'; +const filePath = path.join(testDirectory, file); +fs.mkdirSync(filePath); + +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 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; + } +}); + +fs.writeFileSync(childrenAbsolutePath, 'world'); + +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..ce0c790f80e65c 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 @@ -21,37 +21,33 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // 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 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 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; - } - }); - - await setTimeout(common.platformTimeout(100)); - fs.mkdirSync(filePath); - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(childrenAbsolutePath, 'world'); - - process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +// 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 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 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; + } +}); + +fs.mkdirSync(filePath); +fs.writeFileSync(childrenAbsolutePath, 'world'); + +process.once('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); +}); diff --git a/test/parallel/test-fs-watch-recursive-add-file-with-url.js b/test/parallel/test-fs-watch-recursive-add-file-with-url.js index ee726961c41e9e..4e1855adc52d70 100644 --- a/test/parallel/test-fs-watch-recursive-add-file-with-url.js +++ b/test/parallel/test-fs-watch-recursive-add-file-with-url.js @@ -22,31 +22,28 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // Add a file to already watching folder, and use URL as the path - - const rootDirectory = fs.mkdtempSync(testDir + path.sep); - const testDirectory = path.join(rootDirectory, 'test-5'); - fs.mkdirSync(testDirectory); - - const filePath = path.join(testDirectory, 'file-8.txt'); - const url = pathToFileURL(testDirectory); - - const watcher = fs.watch(url, { recursive: true }); - let watcherClosed = false; - watcher.on('change', function(event, filename) { - assert.strictEqual(event, 'rename'); - - if (filename === path.basename(filePath)) { - watcher.close(); - watcherClosed = true; - } - }); - - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(filePath, 'world'); - - process.on('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +// Add a file to already watching folder, and use URL as the path + +const rootDirectory = fs.mkdtempSync(testDir + path.sep); +const testDirectory = path.join(rootDirectory, 'test-5'); +fs.mkdirSync(testDirectory); + +const filePath = path.join(testDirectory, 'file-8.txt'); +const url = pathToFileURL(testDirectory); + +const watcher = fs.watch(url, { recursive: true }); +let watcherClosed = false; +watcher.on('change', function(event, filename) { + assert.strictEqual(event, 'rename'); + + if (filename === path.basename(filePath)) { + watcher.close(); + watcherClosed = true; + } +}); + +fs.writeFileSync(filePath, 'world'); + +process.on('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..da1b3359c838bf 100644 --- a/test/parallel/test-fs-watch-recursive-add-file.js +++ b/test/parallel/test-fs-watch-recursive-add-file.js @@ -21,30 +21,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-add-folder.js b/test/parallel/test-fs-watch-recursive-add-folder.js index 1851a7850f66ff..841a2c6b2a9bf2 100644 --- a/test/parallel/test-fs-watch-recursive-add-folder.js +++ b/test/parallel/test-fs-watch-recursive-add-folder.js @@ -6,10 +6,6 @@ const { setTimeout } = require('timers/promises'); 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.'); @@ -21,30 +17,27 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // Add a folder to already watching folder +// Add a folder to already watching folder - const rootDirectory = fs.mkdtempSync(testDir + path.sep); - const testDirectory = path.join(rootDirectory, 'test-2'); - fs.mkdirSync(testDirectory); +const rootDirectory = fs.mkdtempSync(testDir + path.sep); +const testDirectory = path.join(rootDirectory, 'test-2'); +fs.mkdirSync(testDirectory); - const testFile = path.join(testDirectory, 'folder-2'); +const testFile = path.join(testDirectory, 'folder-2'); - 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.mkdirSync(testFile); +fs.mkdirSync(testFile); - 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-symlink.js b/test/parallel/test-fs-watch-recursive-symlink.js index 602ec58eab0d7a..3b2d8f63003fc4 100644 --- a/test/parallel/test-fs-watch-recursive-symlink.js +++ b/test/parallel/test-fs-watch-recursive-symlink.js @@ -21,7 +21,7 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { +{ // Add a recursive symlink to the parent folder const testDirectory = fs.mkdtempSync(testDir + path.sep); @@ -48,15 +48,14 @@ tmpdir.refresh(); } }); - await setTimeout(common.platformTimeout(100)); fs.writeFileSync(filePath, 'world'); process.once('exit', function() { assert(watcherClosed, 'watcher Object was not closed'); }); -})().then(common.mustCall()); +} -(async () => { +{ // This test checks how a symlink to outside the tracking folder can trigger change // tmp/sub-directory/tracking-folder/symlink-folder -> tmp/sub-directory @@ -89,12 +88,10 @@ tmpdir.refresh(); } }); - await setTimeout(common.platformTimeout(100)); fs.writeFileSync(forbiddenFile, 'world'); - await setTimeout(common.platformTimeout(100)); fs.writeFileSync(acceptableFile, 'acceptable'); process.once('exit', function() { assert(watcherClosed, 'watcher Object was not closed'); }); -})().then(common.mustCall()); +} diff --git a/test/parallel/test-fs-watch-recursive-update-file.js b/test/parallel/test-fs-watch-recursive-update-file.js index 57d3bffc7a92b0..a8a23ea135ee63 100644 --- a/test/parallel/test-fs-watch-recursive-update-file.js +++ b/test/parallel/test-fs-watch-recursive-update-file.js @@ -21,32 +21,25 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -(async () => { - // 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 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'); - - if (filename === path.basename(testFile)) { - watcher.close(); - watcherClosed = true; - } - })); - - await setTimeout(common.platformTimeout(100)); - fs.writeFileSync(testFile, 'hello'); - - process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); - }); -})().then(common.mustCall()); +// 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 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'); + + if (filename === path.basename(testFile)) { + watcher.close(); + watcherClosed = true; + } +})); + +fs.writeFileSync(testFile, 'hello'); From faf947d69034a6d36bcdaf55110af2f683889d1b Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 15 Jan 2024 19:27:13 +0100 Subject: [PATCH 6/8] major rewrite of Linux recursive watch Signed-off-by: Matteo Collina --- lib/internal/fs/recursive_watch.js | 61 +++++++++++-------- test/parallel/test-fs-recursive-sync-write.js | 4 -- ...ecursive-add-file-to-existing-subfolder.js | 1 - ...-watch-recursive-add-file-to-new-folder.js | 1 - ...st-fs-watch-recursive-add-file-with-url.js | 1 - .../test-fs-watch-recursive-add-file.js | 1 - .../test-fs-watch-recursive-add-folder.js | 1 - .../test-fs-watch-recursive-symlink.js | 1 - .../test-fs-watch-recursive-update-file.js | 3 - 9 files changed, 35 insertions(+), 39 deletions(-) diff --git a/lib/internal/fs/recursive_watch.js b/lib/internal/fs/recursive_watch.js index 779a387b2a5e0a..7d8b12eeb93445 100644 --- a/lib/internal/fs/recursive_watch.js +++ b/lib/internal/fs/recursive_watch.js @@ -40,6 +40,7 @@ class FSWatcher extends EventEmitter { #options = null; #closed = false; #files = new SafeMap(); + #watchers = new SafeMap(); #symbolicFiles = new SafeSet(); #rootPath = pathResolve(); #watchingFile = false; @@ -79,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(); @@ -92,13 +93,13 @@ 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); } } } @@ -125,10 +126,8 @@ 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()) { + this.#watchFile(f); + if (file.isDirectory() && !file.isSymbolicLink()) { this.#watchFolder(f); } } @@ -143,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) { @@ -175,6 +182,7 @@ class FSWatcher extends EventEmitter { this.emit('change', 'change', pathRelative(this.#rootPath, file)); } }); + this.#watchers.set(file, watcher); } [kFSWatchStart](filename) { @@ -187,15 +195,9 @@ class FSWatcher extends EventEmitter { this.#closed = false; this.#watchingFile = file.isFile(); + this.#watchFile(filename); if (file.isDirectory()) { - this.#files.set(filename, file); - this.#watchFolder(filename); - for (const f of this.#files.keys()) { - this.#watchFile(f); - } - } else { - this.#watchFile(filename); } } catch (error) { if (error.code === 'ENOENT') { @@ -230,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 }); @@ -243,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-recursive-sync-write.js b/test/parallel/test-fs-recursive-sync-write.js index ab21735bc4196a..38dce82fb115aa 100644 --- a/test/parallel/test-fs-recursive-sync-write.js +++ b/test/parallel/test-fs-recursive-sync-write.js @@ -33,7 +33,3 @@ const watcher = watch(tmpDir, { recursive: true }, common.mustCall((eventType, _ })); writeFileSync(filename, 'foobar2'); - -process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); -}); 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 df8d01f06b985d..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()`'); 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 ce0c790f80e65c..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()`'); diff --git a/test/parallel/test-fs-watch-recursive-add-file-with-url.js b/test/parallel/test-fs-watch-recursive-add-file-with-url.js index 4e1855adc52d70..23f4a9d2bbad4b 100644 --- a/test/parallel/test-fs-watch-recursive-add-file-with-url.js +++ b/test/parallel/test-fs-watch-recursive-add-file-with-url.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()`'); diff --git a/test/parallel/test-fs-watch-recursive-add-file.js b/test/parallel/test-fs-watch-recursive-add-file.js index da1b3359c838bf..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()`'); diff --git a/test/parallel/test-fs-watch-recursive-add-folder.js b/test/parallel/test-fs-watch-recursive-add-folder.js index 841a2c6b2a9bf2..9024acb9bb77a4 100644 --- a/test/parallel/test-fs-watch-recursive-add-folder.js +++ b/test/parallel/test-fs-watch-recursive-add-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()`'); diff --git a/test/parallel/test-fs-watch-recursive-symlink.js b/test/parallel/test-fs-watch-recursive-symlink.js index 3b2d8f63003fc4..0378a20e4c2bd4 100644 --- a/test/parallel/test-fs-watch-recursive-symlink.js +++ b/test/parallel/test-fs-watch-recursive-symlink.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()`'); diff --git a/test/parallel/test-fs-watch-recursive-update-file.js b/test/parallel/test-fs-watch-recursive-update-file.js index a8a23ea135ee63..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()`'); @@ -31,14 +30,12 @@ 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'); if (filename === path.basename(testFile)) { watcher.close(); - watcherClosed = true; } })); From 00ce83355b4fc9c075b4051c19c72b501447c3ac Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 19 Jan 2024 18:41:59 +0100 Subject: [PATCH 7/8] fixup Signed-off-by: Matteo Collina --- .../test-fs-watch-recursive-add-folder.js | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/test/parallel/test-fs-watch-recursive-add-folder.js b/test/parallel/test-fs-watch-recursive-add-folder.js index 9024acb9bb77a4..1851a7850f66ff 100644 --- a/test/parallel/test-fs-watch-recursive-add-folder.js +++ b/test/parallel/test-fs-watch-recursive-add-folder.js @@ -1,10 +1,15 @@ 'use strict'; const common = require('../common'); +const { setTimeout } = require('timers/promises'); 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.'); @@ -16,27 +21,30 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -// Add a folder to already watching folder +(async () => { + // Add a folder to already watching folder -const rootDirectory = fs.mkdtempSync(testDir + path.sep); -const testDirectory = path.join(rootDirectory, 'test-2'); -fs.mkdirSync(testDirectory); + const rootDirectory = fs.mkdtempSync(testDir + path.sep); + const testDirectory = path.join(rootDirectory, 'test-2'); + fs.mkdirSync(testDirectory); -const testFile = path.join(testDirectory, 'folder-2'); + const testFile = path.join(testDirectory, 'folder-2'); -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; + } + }); -fs.mkdirSync(testFile); + await setTimeout(common.platformTimeout(100)); + fs.mkdirSync(testFile); -process.once('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); -}); + process.once('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); + }); +})().then(common.mustCall()); From 607bf16ba698f4e7134ee052ad7dbfbb3388d87c Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 22 Jan 2024 16:31:13 +0100 Subject: [PATCH 8/8] fixup Signed-off-by: Matteo Collina --- ...st-fs-watch-recursive-add-file-with-url.js | 42 ++++++++++--------- .../test-fs-watch-recursive-symlink.js | 12 ++++-- ... => test-fs-watch-recursive-sync-write.js} | 0 3 files changed, 31 insertions(+), 23 deletions(-) rename test/parallel/{test-fs-recursive-sync-write.js => test-fs-watch-recursive-sync-write.js} (100%) diff --git a/test/parallel/test-fs-watch-recursive-add-file-with-url.js b/test/parallel/test-fs-watch-recursive-add-file-with-url.js index 23f4a9d2bbad4b..ee726961c41e9e 100644 --- a/test/parallel/test-fs-watch-recursive-add-file-with-url.js +++ b/test/parallel/test-fs-watch-recursive-add-file-with-url.js @@ -1,6 +1,7 @@ 'use strict'; const common = require('../common'); +const { setTimeout } = require('timers/promises'); if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); @@ -21,28 +22,31 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -// Add a file to already watching folder, and use URL as the path +(async () => { + // Add a file to already watching folder, and use URL as the path -const rootDirectory = fs.mkdtempSync(testDir + path.sep); -const testDirectory = path.join(rootDirectory, 'test-5'); -fs.mkdirSync(testDirectory); + const rootDirectory = fs.mkdtempSync(testDir + path.sep); + const testDirectory = path.join(rootDirectory, 'test-5'); + fs.mkdirSync(testDirectory); -const filePath = path.join(testDirectory, 'file-8.txt'); -const url = pathToFileURL(testDirectory); + const filePath = path.join(testDirectory, 'file-8.txt'); + const url = pathToFileURL(testDirectory); -const watcher = fs.watch(url, { recursive: true }); -let watcherClosed = false; -watcher.on('change', function(event, filename) { - assert.strictEqual(event, 'rename'); + const watcher = fs.watch(url, { recursive: true }); + let watcherClosed = false; + watcher.on('change', function(event, filename) { + assert.strictEqual(event, 'rename'); - if (filename === path.basename(filePath)) { - watcher.close(); - watcherClosed = true; - } -}); + if (filename === path.basename(filePath)) { + watcher.close(); + watcherClosed = true; + } + }); -fs.writeFileSync(filePath, 'world'); + await setTimeout(common.platformTimeout(100)); + fs.writeFileSync(filePath, 'world'); -process.on('exit', function() { - assert(watcherClosed, 'watcher Object was not closed'); -}); + process.on('exit', function() { + assert(watcherClosed, 'watcher Object was not closed'); + }); +})().then(common.mustCall()); diff --git a/test/parallel/test-fs-watch-recursive-symlink.js b/test/parallel/test-fs-watch-recursive-symlink.js index 0378a20e4c2bd4..602ec58eab0d7a 100644 --- a/test/parallel/test-fs-watch-recursive-symlink.js +++ b/test/parallel/test-fs-watch-recursive-symlink.js @@ -1,6 +1,7 @@ 'use strict'; const common = require('../common'); +const { setTimeout } = require('timers/promises'); if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); @@ -20,7 +21,7 @@ const tmpdir = require('../common/tmpdir'); const testDir = tmpdir.path; tmpdir.refresh(); -{ +(async () => { // Add a recursive symlink to the parent folder const testDirectory = fs.mkdtempSync(testDir + path.sep); @@ -47,14 +48,15 @@ tmpdir.refresh(); } }); + await setTimeout(common.platformTimeout(100)); fs.writeFileSync(filePath, 'world'); process.once('exit', function() { assert(watcherClosed, 'watcher Object was not closed'); }); -} +})().then(common.mustCall()); -{ +(async () => { // This test checks how a symlink to outside the tracking folder can trigger change // tmp/sub-directory/tracking-folder/symlink-folder -> tmp/sub-directory @@ -87,10 +89,12 @@ tmpdir.refresh(); } }); + await setTimeout(common.platformTimeout(100)); fs.writeFileSync(forbiddenFile, 'world'); + await setTimeout(common.platformTimeout(100)); fs.writeFileSync(acceptableFile, 'acceptable'); process.once('exit', function() { assert(watcherClosed, 'watcher Object was not closed'); }); -} +})().then(common.mustCall()); diff --git a/test/parallel/test-fs-recursive-sync-write.js b/test/parallel/test-fs-watch-recursive-sync-write.js similarity index 100% rename from test/parallel/test-fs-recursive-sync-write.js rename to test/parallel/test-fs-watch-recursive-sync-write.js