Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: rimraf should not recurse on failure #35566

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/internal/fs/rimraf.js
Expand Up @@ -12,6 +12,7 @@ const {
} = primordials;

const { Buffer } = require('buffer');
const fs = require('fs');
const {
chmod,
chmodSync,
Expand All @@ -25,7 +26,7 @@ const {
statSync,
unlink,
unlinkSync
} = require('fs');
} = fs;
const { sep } = require('path');
const { setTimeout } = require('timers');
const { sleep } = require('internal/util');
Expand Down Expand Up @@ -249,7 +250,7 @@ function _rmdirSync(path, options, originalErr) {

for (let i = 1; i <= tries; i++) {
try {
return rmdirSync(path, options);
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.
Expand Down
3 changes: 1 addition & 2 deletions 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
Expand Down Expand Up @@ -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
Expand Up @@ -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');

Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-fs-rmdir-recursive.js
Expand Up @@ -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);
}