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>

Author: mcollina

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){
 constwatcher=newnonNativeWatcher.FSWatcher(options);
-awaitwatcher[kFSWatchStart](filename);
+watcher[kFSWatchStart](filename);
 yield*watcher;
 return;
 }

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');
 
 letinternalSync;
-letinternalPromises;
-
-functionlazyLoadFsPromises(){
-internalPromises??=require('fs/promises');
-returninternalPromises;
-}
 
 functionlazyLoadFsSync(){
 internalSync??=require('fs');
 returninternalSync;
 }
-letkResistStopPropagation;
-
-asyncfunctiontraverse(dir,files=newSafeMap(),symbolicLinks=newSafeSet()){
-const{ opendir }=lazyLoadFsPromises();
-
-constfilenames=awaitopendir(dir);
-constsubdirectories=[];
-
-forawait(constfileoffilenames){
-constf=pathJoin(dir,file.name);
-
-files.set(f,file);
-
-// Do not follow symbolic links
-if(file.isSymbolicLink()){
-symbolicLinks.add(f);
-}elseif(file.isDirectory()){
-ArrayPrototypePush(subdirectories,traverse(f,files));
-}
-}
-
-awaitSafePromiseAllReturnVoid(subdirectories);
 
-returnfiles;
-}
+letkResistStopPropagation;
 
 classFSWatcherextendsEventEmitter{
  #options =null;
  #closed =false;
  #files =newSafeMap();
+ #watchers =newSafeMap();
  #symbolicFiles =newSafeSet();
  #rootPath =pathResolve();
  #watchingFile =false;
@@ -111,11 +80,11 @@ class FSWatcher extends EventEmitter {
 return;
 }
 
-const{ unwatchFile }=lazyLoadFsSync();
 this.#closed =true;
 
 for(constfileofthis.#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(constfilenameofthis.#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{
-constfiles=awaitopendir(folder);
+constfiles=readdirSync(folder,{
+withFileTypes: true,
+});
 
-forawait(constfileoffiles){
+for(constfileoffiles){
 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);
-}elseif(file.isDirectory()&&!file.isSymbolicLink()){
-awaitthis.#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();
-constexistingStat=this.#files.get(file);
+const{ watch, statSync }=lazyLoadFsSync();
+
+if(this.#files.has(file)){
+return;
+}
+
+{
+constexistingStat=statSync(file);
+this.#files.set(file,existingStat);
+}
 
-watchFile(file,{
+constwatcher=watch(file,{
 persistent: this.#options.persistent,
-},(currentStats,previousStats)=>{
-if(existingStat&&!existingStat.isDirectory()&&
-currentStats.nlink!==0&&existingStat.mtimeMs===currentStats.mtimeMs){
-return;
-}
+},(eventType,filename)=>{
+constexistingStat=this.#files.get(file);
+constcurrentStats=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);
 }elseif(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(constfofthis.#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)=>{
-constonAbort=()=>reject(newAbortError(undefined,{cause: signal.reason}));
+constonAbort=()=>{
+this.close();
+reject(newAbortError(undefined,{cause: signal.reason}));
+};
 if(signal.aborted)returnonAbort();
 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} :
 newPromise(promiseExecutor)),
+return: ()=>{
+this.close();
+return{__proto__: null,done: true};
+},
 [SymbolAsyncIterator](){returnthis;},
 };
 }

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

@@ -1,7 +1,6 @@
 'use strict';
 
 constcommon=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');
 consttestDir=tmpdir.path;
 tmpdir.refresh();
 
-(async()=>{
-// Add a file to subfolder of a watching folder
+// Add a file to subfolder of a watching folder
 
-constrootDirectory=fs.mkdtempSync(testDir+path.sep);
-consttestDirectory=path.join(rootDirectory,'test-4');
-fs.mkdirSync(testDirectory);
+constrootDirectory=fs.mkdtempSync(testDir+path.sep);
+consttestDirectory=path.join(rootDirectory,'test-4');
+fs.mkdirSync(testDirectory);
 
-constfile='folder-5';
-constfilePath=path.join(testDirectory,file);
-fs.mkdirSync(filePath);
+constfile='folder-5';
+constfilePath=path.join(testDirectory,file);
+fs.mkdirSync(filePath);
 
-constsubfolderPath=path.join(filePath,'subfolder-6');
-fs.mkdirSync(subfolderPath);
+constsubfolderPath=path.join(filePath,'subfolder-6');
+fs.mkdirSync(subfolderPath);
 
-constchildrenFile='file-7.txt';
-constchildrenAbsolutePath=path.join(subfolderPath,childrenFile);
-constrelativePath=path.join(file,path.basename(subfolderPath),childrenFile);
+constchildrenFile='file-7.txt';
+constchildrenAbsolutePath=path.join(subfolderPath,childrenFile);
+constrelativePath=path.join(file,path.basename(subfolderPath),childrenFile);
 
-constwatcher=fs.watch(testDirectory,{recursive: true});
-letwatcherClosed=false;
-watcher.on('change',function(event,filename){
-assert.strictEqual(event,'rename');
+constwatcher=fs.watch(testDirectory,{recursive: true});
+letwatcherClosed=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;
+}
+});
 
-awaitsetTimeout(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');
+});

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

@@ -1,7 +1,6 @@
 'use strict';
 
 constcommon=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');
 consttestDir=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
 
-constrootDirectory=fs.mkdtempSync(testDir+path.sep);
-consttestDirectory=path.join(rootDirectory,'test-3');
-fs.mkdirSync(testDirectory);
+constrootDirectory=fs.mkdtempSync(testDir+path.sep);
+consttestDirectory=path.join(rootDirectory,'test-3');
+fs.mkdirSync(testDirectory);
 
-constfilePath=path.join(testDirectory,'folder-3');
+constfilePath=path.join(testDirectory,'folder-3');
 
-constchildrenFile='file-4.txt';
-constchildrenAbsolutePath=path.join(filePath,childrenFile);
-constchildrenRelativePath=path.join(path.basename(filePath),childrenFile);
+constchildrenFile='file-4.txt';
+constchildrenAbsolutePath=path.join(filePath,childrenFile);
+constchildrenRelativePath=path.join(path.basename(filePath),childrenFile);
 
-constwatcher=fs.watch(testDirectory,{recursive: true});
-letwatcherClosed=false;
-watcher.on('change',function(event,filename){
-assert.strictEqual(event,'rename');
-assert.ok(filename===path.basename(filePath)||filename===childrenRelativePath);
+constwatcher=fs.watch(testDirectory,{recursive: true});
+letwatcherClosed=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;
+}
+});
 
-awaitsetTimeout(common.platformTimeout(100));
-fs.mkdirSync(filePath);
-awaitsetTimeout(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');
+});