From 63d77137ee3ef3809cb54cca4750a4a27ab66356 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 3 Apr 2024 16:40:46 +0200 Subject: [PATCH 01/10] fs: fixes recursive fs.watch crash on Linux when deleting files Signed-off-by: Matteo Collina Fixes: https://github.com/nodejs/node/issues/52018 --- lib/internal/fs/recursive_watch.js | 10 +++++--- .../test-fs-watch-recursive-delete.js | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-fs-watch-recursive-delete.js diff --git a/lib/internal/fs/recursive_watch.js b/lib/internal/fs/recursive_watch.js index 7d8b12eeb93445..3e421836191b52 100644 --- a/lib/internal/fs/recursive_watch.js +++ b/lib/internal/fs/recursive_watch.js @@ -157,11 +157,15 @@ class FSWatcher extends EventEmitter { persistent: this.#options.persistent, }, (eventType, filename) => { const existingStat = this.#files.get(file); - const currentStats = statSync(file); + let currentStats - this.#files.set(file, currentStats); + try { + currentStats = statSync(file); + this.#files.set(file, currentStats); + } catch { + } - if (currentStats.birthtimeMs === 0 && existingStat.birthtimeMs !== 0) { + if (currentStats === undefined || currentStats.birthtimeMs === 0 && existingStat.birthtimeMs !== 0) { // The file is now deleted this.#files.delete(file); this.#watchers.delete(file); diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js new file mode 100644 index 00000000000000..fa62a8efcb490e --- /dev/null +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -0,0 +1,25 @@ +'use strict' + +const common = require('../common') +const tmpdir = require('../common/tmpdir'); +const fs = require('fs') +const { join } = require('path') +const assert = require('node:assert') + +tmpdir.refresh(); + +try { + fs.mkdirSync(tmpdir.resolve('./parent/child'), { recursive: true }) +} catch (e) { + console.error(e) +} + +fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test') + +const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCall((eventType, filename) => { + // We are only checking for the filename to avoid having Windows, Linux and Mac specific assertions + assert.strictEqual(filename.indexOf('test.tmp') >= 0, true) + watcher.close() +})) + +fs.rmSync(tmpdir.resolve('./parent/child/test.tmp')) From 6e8c33b5f5b377601afa756817d035030b8b25ec Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 3 Apr 2024 16:52:27 +0200 Subject: [PATCH 02/10] fixup Signed-off-by: Matteo Collina --- lib/internal/fs/recursive_watch.js | 5 ++-- .../test-fs-watch-recursive-delete.js | 25 ++++++++----------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/lib/internal/fs/recursive_watch.js b/lib/internal/fs/recursive_watch.js index 3e421836191b52..d312e54013cfb9 100644 --- a/lib/internal/fs/recursive_watch.js +++ b/lib/internal/fs/recursive_watch.js @@ -157,15 +157,16 @@ class FSWatcher extends EventEmitter { persistent: this.#options.persistent, }, (eventType, filename) => { const existingStat = this.#files.get(file); - let currentStats + let currentStats; try { currentStats = statSync(file); this.#files.set(file, currentStats); } catch { + // This happens if the file was removed } - if (currentStats === undefined || currentStats.birthtimeMs === 0 && existingStat.birthtimeMs !== 0) { + if (currentStats === undefined || (currentStats.birthtimeMs === 0 && existingStat.birthtimeMs !== 0)) { // The file is now deleted this.#files.delete(file); this.#watchers.delete(file); diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index fa62a8efcb490e..0da94908ea5297 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -1,25 +1,20 @@ -'use strict' +'use strict'; -const common = require('../common') +const common = require('../common'); const tmpdir = require('../common/tmpdir'); -const fs = require('fs') -const { join } = require('path') -const assert = require('node:assert') +const fs = require('fs'); +const assert = require('node:assert'); tmpdir.refresh(); -try { - fs.mkdirSync(tmpdir.resolve('./parent/child'), { recursive: true }) -} catch (e) { - console.error(e) -} +fs.mkdirSync(tmpdir.resolve('./parent/child'), { recursive: true }); -fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test') +fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test'); const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCall((eventType, filename) => { // We are only checking for the filename to avoid having Windows, Linux and Mac specific assertions - assert.strictEqual(filename.indexOf('test.tmp') >= 0, true) - watcher.close() -})) + assert.strictEqual(filename.indexOf('test.tmp') >= 0, true); + watcher.close(); +})); -fs.rmSync(tmpdir.resolve('./parent/child/test.tmp')) +fs.rmSync(tmpdir.resolve('./parent/child/test.tmp')); From 52fd037b17802a7f5ba1ed05db45e395742101c7 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Apr 2024 11:45:50 +0200 Subject: [PATCH 03/10] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-watch-recursive-delete.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index 0da94908ea5297..c93e3f9f035ccf 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -17,4 +17,5 @@ const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common watcher.close(); })); -fs.rmSync(tmpdir.resolve('./parent/child/test.tmp')); +// We must use the asynchronous version of `fs.rm()` to let the watcher be set up properly +fs.rm(tmpdir.resolve('./parent/child'), { recursive: true }, common.mustCall()); From a95ff831ae4d2bcc5e1b94f9de938f9edecc5580 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Apr 2024 11:46:31 +0200 Subject: [PATCH 04/10] Update test/parallel/test-fs-watch-recursive-delete.js Co-authored-by: Moshe Atlow --- test/parallel/test-fs-watch-recursive-delete.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index c93e3f9f035ccf..08368b76fe4b9a 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -13,8 +13,9 @@ fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test'); const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCall((eventType, filename) => { // We are only checking for the filename to avoid having Windows, Linux and Mac specific assertions - assert.strictEqual(filename.indexOf('test.tmp') >= 0, true); - watcher.close(); + if (fs.readDirSync(tmpdir.resolve('./parent')).length === 0) { + watcher.close(); + } })); // We must use the asynchronous version of `fs.rm()` to let the watcher be set up properly From 0635a8a83d6b10c246183e25057989c5f6a9d392 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Apr 2024 11:47:23 +0200 Subject: [PATCH 05/10] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-watch-recursive-delete.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index 08368b76fe4b9a..8d372bc0bafa31 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -3,7 +3,6 @@ const common = require('../common'); const tmpdir = require('../common/tmpdir'); const fs = require('fs'); -const assert = require('node:assert'); tmpdir.refresh(); @@ -14,8 +13,8 @@ fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test'); const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCall((eventType, filename) => { // We are only checking for the filename to avoid having Windows, Linux and Mac specific assertions if (fs.readDirSync(tmpdir.resolve('./parent')).length === 0) { - watcher.close(); - } + watcher.close(); + } })); // We must use the asynchronous version of `fs.rm()` to let the watcher be set up properly From 805e8b0c8e94d69eb4d22c2853f06a2d7088a069 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Apr 2024 13:37:38 +0200 Subject: [PATCH 06/10] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-watch-recursive-delete.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index 8d372bc0bafa31..c14eb4828d509c 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -12,7 +12,7 @@ fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test'); const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCall((eventType, filename) => { // We are only checking for the filename to avoid having Windows, Linux and Mac specific assertions - if (fs.readDirSync(tmpdir.resolve('./parent')).length === 0) { + if (fs.readdirSync(tmpdir.resolve('./parent')).length === 0) { watcher.close(); } })); From 5c50a1efb5231a3733ee0f83259517d6a8ebe255 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Apr 2024 15:43:11 +0200 Subject: [PATCH 07/10] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-watch-recursive-delete.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index c14eb4828d509c..f128744f8c0479 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -10,12 +10,14 @@ fs.mkdirSync(tmpdir.resolve('./parent/child'), { recursive: true }); fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test'); -const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCall((eventType, filename) => { +const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCallAtLeast((eventType, filename) => { // We are only checking for the filename to avoid having Windows, Linux and Mac specific assertions if (fs.readdirSync(tmpdir.resolve('./parent')).length === 0) { watcher.close(); } -})); +}), 1); -// We must use the asynchronous version of `fs.rm()` to let the watcher be set up properly -fs.rm(tmpdir.resolve('./parent/child'), { recursive: true }, common.mustCall()); +// We must wait a bit `fs.rm()` to let the watcher be set up properly +setTimeout(() => { + fs.rm(tmpdir.resolve('./parent/child'), { recursive: true }, common.mustCall()); +}, common.platformTimeout(500)); From c7ed046c2544cecc5b607f046a7c7a6114d7633a Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 11 Apr 2024 16:54:50 +0200 Subject: [PATCH 08/10] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-watch-recursive-delete.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index f128744f8c0479..e036c126df6d12 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -10,12 +10,16 @@ fs.mkdirSync(tmpdir.resolve('./parent/child'), { recursive: true }); fs.writeFileSync(tmpdir.resolve('./parent/child/test.tmp'), 'test'); -const watcher = fs.watch(tmpdir.resolve('./parent'), { recursive: true }, common.mustCallAtLeast((eventType, filename) => { +const toWatch = tmpdir.resolve('./parent'); + +const onFileUpdate = common.mustCallAtLeast((eventType, filename) => { // We are only checking for the filename to avoid having Windows, Linux and Mac specific assertions if (fs.readdirSync(tmpdir.resolve('./parent')).length === 0) { watcher.close(); } -}), 1); +}, 1); + +const watcher = fs.watch(toWatch, { recursive: true }, onFileUpdate); // We must wait a bit `fs.rm()` to let the watcher be set up properly setTimeout(() => { From 91b9a982fd769d2c168cb1ce4eb30e1fefcbc66f Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 18 Apr 2024 16:18:58 +0200 Subject: [PATCH 09/10] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-watch-recursive-delete.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index e036c126df6d12..d1929bf3e8cd56 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -4,6 +4,9 @@ const common = require('../common'); const tmpdir = require('../common/tmpdir'); const fs = require('fs'); +if (common.isSunOS) + common.skip('SunOS behaves differently') + tmpdir.refresh(); fs.mkdirSync(tmpdir.resolve('./parent/child'), { recursive: true }); From 3a358b291a3f0cfb93ed808c643034c7b6f2bdb0 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 18 Apr 2024 16:26:10 +0200 Subject: [PATCH 10/10] fixup Signed-off-by: Matteo Collina --- test/parallel/test-fs-watch-recursive-delete.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-watch-recursive-delete.js b/test/parallel/test-fs-watch-recursive-delete.js index d1929bf3e8cd56..27d1ebcc14e005 100644 --- a/test/parallel/test-fs-watch-recursive-delete.js +++ b/test/parallel/test-fs-watch-recursive-delete.js @@ -5,7 +5,7 @@ const tmpdir = require('../common/tmpdir'); const fs = require('fs'); if (common.isSunOS) - common.skip('SunOS behaves differently') + common.skip('SunOS behaves differently'); tmpdir.refresh();