fs: fix rmsync error swallowing

fix rmsync swallowing errors instead of throwing them. fixes: #38683 fixes: #34580 PR-URL: #38684 Fixes: #38683 Fixes: #34580 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · May 25, 2021 · f9447b7 · f9447b7
1 parent fa7cdd6
commit f9447b7
Show file tree

Hide file tree

Showing 9 changed files with 286 additions and 154 deletions.
diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js
@@ -219,6 +219,11 @@ function _unlinkSync(path, options) {
           i < tries &&
           options.retryDelay > 0) {
         sleep(i * options.retryDelay);
+      } else if (err.code === 'ENOENT') {
+        // The file is already gone.
+        return;
+      } else if (i === tries) {
+        throw err;
       }
     }
   }
@@ -231,8 +236,9 @@ function _rmdirSync(path, options, originalErr) {
   } catch (err) {
     if (err.code === 'ENOENT')
       return;
-    if (err.code === 'ENOTDIR')
-      throw originalErr;
+    if (err.code === 'ENOTDIR') {
+      throw originalErr || err;
+    }
 
     if (notEmptyErrorCodes.has(err.code)) {
       // Removing failed. Try removing all children and then retrying the
@@ -259,10 +265,17 @@ function _rmdirSync(path, options, originalErr) {
               i < tries &&
               options.retryDelay > 0) {
             sleep(i * options.retryDelay);
+          } else if (err.code === 'ENOENT') {
+            // The file is already gone.
+            return;
+          } else if (i === tries) {
+            throw err;
           }
         }
       }
     }
+
+    throw originalErr || err;
   }
 }
 

diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js
@@ -26,8 +26,7 @@ function makeDirectoryReadOnly(dir) {
   let accessErrorCode = 'EACCES';
   if (common.isWindows) {
     accessErrorCode = 'EPERM';
-    execSync(`icacls ${dir} /inheritance:r`);
-    execSync(`icacls ${dir} /deny "everyone":W`);
+    execSync(`icacls ${dir} /deny "everyone:(OI)(CI)(DE,DC,AD,WD)"`);
   } else {
     fs.chmodSync(dir, '444');
   }
@@ -36,7 +35,7 @@ function makeDirectoryReadOnly(dir) {
 
 function makeDirectoryWritable(dir) {
   if (common.isWindows) {
-    execSync(`icacls ${dir} /grant "everyone":W`);
+    execSync(`icacls ${dir} /remove:d "everyone"`);
   }
 }
 

diff --git a/test/parallel/test-fs-open-no-close.js b/test/parallel/test-fs-open-no-close.js
@@ -15,10 +15,17 @@ const debuglog = (arg) => {
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 
-{
-  fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => {
-    debuglog('fs open() callback');
-    assert.ifError(err);
-  }));
-  debuglog('waiting for callback');
-}
+let openFd;
+
+fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => {
+  debuglog('fs open() callback');
+  assert.ifError(err);
+  openFd = fd;
+}));
+debuglog('waiting for callback');
+
+process.on('beforeExit', common.mustCall(() => {
+  if (openFd) {
+    fs.closeSync(openFd);
+  }
+}));
diff --git a/test/parallel/test-fs-promises-file-handle-read-worker.js b/test/parallel/test-fs-promises-file-handle-read-worker.js
@@ -19,16 +19,20 @@ if (isMainThread || !workerData) {
       transferList: [handle]
     });
   });
-  fs.promises.open(file, 'r').then((handle) => {
-    fs.createReadStream(null, { fd: handle });
-    assert.throws(() => {
-      new Worker(__filename, {
-        workerData: { handle },
-        transferList: [handle]
+  fs.promises.open(file, 'r').then(async (handle) => {
+    try {
+      fs.createReadStream(null, { fd: handle });
+      assert.throws(() => {
+        new Worker(__filename, {
+          workerData: { handle },
+          transferList: [handle]
+        });
+      }, {
+        code: 25,
       });
-    }, {
-      code: 25,
-    });
+    } finally {
+      await handle.close();
+    }
   });
 } else {
   let output = '';

diff --git a/test/parallel/test-fs-promises-file-handle-readFile.js b/test/parallel/test-fs-promises-file-handle-readFile.js
@@ -56,13 +56,16 @@ async function doReadAndCancel() {
   {
     const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt');
     const fileHandle = await open(filePathForHandle, 'w+');
-    const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
-    fs.writeFileSync(filePathForHandle, buffer);
-    const signal = AbortSignal.abort();
-    await assert.rejects(readFile(fileHandle, { signal }), {
-      name: 'AbortError'
-    });
-    await fileHandle.close();
+    try {
+      const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
+      fs.writeFileSync(filePathForHandle, buffer);
+      const signal = AbortSignal.abort();
+      await assert.rejects(readFile(fileHandle, { signal }), {
+        name: 'AbortError'
+      });
+    } finally {
+      await fileHandle.close();
+    }
   }
 
   // Signal aborted on first tick

diff --git a/test/parallel/test-fs-promises-file-handle-writeFile.js b/test/parallel/test-fs-promises-file-handle-writeFile.js
@@ -30,13 +30,17 @@ async function validateWriteFile() {
 async function doWriteAndCancel() {
   const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt');
   const fileHandle = await open(filePathForHandle, 'w+');
-  const buffer = Buffer.from('dogs running'.repeat(512 * 1024), 'utf8');
-  const controller = new AbortController();
-  const { signal } = controller;
-  process.nextTick(() => controller.abort());
-  await assert.rejects(writeFile(fileHandle, buffer, { signal }), {
-    name: 'AbortError'
-  });
+  try {
+    const buffer = Buffer.from('dogs running'.repeat(512 * 1024), 'utf8');
+    const controller = new AbortController();
+    const { signal } = controller;
+    process.nextTick(() => controller.abort());
+    await assert.rejects(writeFile(fileHandle, buffer, { signal }), {
+      name: 'AbortError'
+    });
+  } finally {
+    await fileHandle.close();
+  }
 }
 
 validateWriteFile()