From ab8731c568fd6fe548fe33d1e3c8322932be76aa Mon Sep 17 00:00:00 2001 From: bcoe Date: Thu, 8 Oct 2020 17:17:01 -0700 Subject: [PATCH 1/5] fs: rimraf should not recurse on failure --- lib/internal/fs/rimraf.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index e88dd9697b1938..d64feacedd0ed6 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -249,7 +249,7 @@ function _rmdirSync(path, options, originalErr) { for (let i = 1; i <= tries; i++) { try { - return rmdirSync(path, options); + return rmdirSync(path, {...options, recursive: false}); } catch (err) { // Only sleep if this is not the last try, and the delay is greater // than zero, and an error was encountered that warrants a retry. From c1f8562a5b5a8b2ce19ea49583e8bbf30661f6c4 Mon Sep 17 00:00:00 2001 From: bcoe Date: Thu, 8 Oct 2020 17:31:34 -0700 Subject: [PATCH 2/5] chore: run the linter --- lib/internal/fs/rimraf.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index d64feacedd0ed6..6988425ecbaeb5 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -249,7 +249,7 @@ function _rmdirSync(path, options, originalErr) { for (let i = 1; i <= tries; i++) { try { - return rmdirSync(path, {...options, recursive: false}); + return rmdirSync(path, { ...options, recursive: false }); } catch (err) { // Only sleep if this is not the last try, and the delay is greater // than zero, and an error was encountered that warrants a retry. From 3dfc53f79e4677398be4757c42191450977b34e1 Mon Sep 17 00:00:00 2001 From: bcoe Date: Thu, 8 Oct 2020 17:51:36 -0700 Subject: [PATCH 3/5] chore: code review --- lib/internal/fs/rimraf.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index 6988425ecbaeb5..b8eade61cf9bea 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -249,7 +249,7 @@ function _rmdirSync(path, options, originalErr) { for (let i = 1; i <= tries; i++) { try { - return rmdirSync(path, { ...options, recursive: false }); + return rmdirSync(path); } catch (err) { // Only sleep if this is not the last try, and the delay is greater // than zero, and an error was encountered that warrants a retry. From 3ef437bf994cbdc24a0a3379241eae8cb48b8342 Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 11 Oct 2020 11:45:59 -0700 Subject: [PATCH 4/5] test: added a test --- lib/internal/fs/rimraf.js | 5 +++-- test/parallel/test-fs-rmdir-recursive.js | 25 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index b8eade61cf9bea..e67b14a418c884 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -12,6 +12,7 @@ const { } = primordials; const { Buffer } = require('buffer'); +const fs = require('fs'); const { chmod, chmodSync, @@ -25,7 +26,7 @@ const { statSync, unlink, unlinkSync -} = require('fs'); +} = fs; const { sep } = require('path'); const { setTimeout } = require('timers'); const { sleep } = require('internal/util'); @@ -249,7 +250,7 @@ function _rmdirSync(path, options, originalErr) { for (let i = 1; i <= tries; i++) { try { - return rmdirSync(path); + return fs.rmdirSync(path); } catch (err) { // Only sleep if this is not the last try, and the delay is greater // than zero, and an error was encountered that warrants a retry. diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 1d2525a5ed6341..bbf89a3959e19d 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -211,3 +211,28 @@ function removeAsync(dir) { message: /^The value of "options\.maxRetries" is out of range\./ }); } + +// It should not pass recursive option to rmdirSync, when called from +// rimraf (see: #35566) +{ + // Make a non-empty directory: + const original = fs.rmdirSync; + const dir = `${nextDirPath()}/foo/bar`; + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(`${dir}/foo.txt`, 'hello world', 'utf8'); + + // When called the second time from rimraf, the recursive option should + // not be set for rmdirSync: + let callCount = 0; + let rmdirSyncOptionsFromRimraf; + fs.rmdirSync = (path, options) => { + if (callCount > 0) { + rmdirSyncOptionsFromRimraf = { ...options }; + } + callCount++; + return original(path, options); + }; + fs.rmdirSync(dir, { recursive: true }); + fs.rmdirSync = original; + assert.strictEqual(rmdirSyncOptionsFromRimraf.recursive, undefined); +} From 0c7e55d80cfaaaf2125a1423671a8319511e4e91 Mon Sep 17 00:00:00 2001 From: bcoe Date: Mon, 12 Oct 2020 10:42:13 -0700 Subject: [PATCH 5/5] test: rimraf fix seems to have fixed known issue --- test/known_issues/known_issues.status | 3 +-- test/{known_issues => parallel}/test-fs-open-no-close.js | 7 ------- 2 files changed, 1 insertion(+), 9 deletions(-) rename test/{known_issues => parallel}/test-fs-open-no-close.js (62%) diff --git a/test/known_issues/known_issues.status b/test/known_issues/known_issues.status index 01a82246c93fcb..e0f0a456089bf2 100644 --- a/test/known_issues/known_issues.status +++ b/test/known_issues/known_issues.status @@ -1,3 +1,4 @@ + prefix known_issues # If a known issue does not apply to a platform, list the test name in the @@ -27,5 +28,3 @@ test-vm-timeout-escape-queuemicrotask: SKIP # The Raspberry Pis are too slow to run this test. # See https://github.com/nodejs/build/issues/2227#issuecomment-608334574 test-crypto-authenticated-stream: SKIP -# The bug being checked is that the test never exits. -test-fs-open-no-close: TIMEOUT diff --git a/test/known_issues/test-fs-open-no-close.js b/test/parallel/test-fs-open-no-close.js similarity index 62% rename from test/known_issues/test-fs-open-no-close.js rename to test/parallel/test-fs-open-no-close.js index ef990d1a67df83..5e432dd11d8084 100644 --- a/test/known_issues/test-fs-open-no-close.js +++ b/test/parallel/test-fs-open-no-close.js @@ -4,14 +4,7 @@ // Failing to close a file should not keep the event loop open. const common = require('../common'); - -// This issue only shows up on Raspberry Pi devices in our CI. When this test is -// moved out of known_issues, this check can be removed, as the test should pass -// on all platforms at that point. const assert = require('assert'); -if (process.arch !== 'arm' || process.config.variables.arm_version > 7) { - assert.fail('This test is for Raspberry Pi devices in CI'); -} const fs = require('fs');