fs: remove race condition for recursive watch on Linux

Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#51406 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
marco-ippolito · Jan 29, 2024 · 192a3fa · 192a3fa
1 parent 80ad1e4
commit 192a3fa
Show file tree

Hide file tree

Showing 8 changed files with 182 additions and 198 deletions.
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -1255,7 +1255,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
@@ -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
@@ -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
@@ -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');
+});