Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fs: rimraf should not recurse on failure
PR-URL: #35566
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
bcoe authored and MylesBorins committed Oct 14, 2020
1 parent 41d7500 commit a3c7f8e
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 11 deletions.
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);
}

0 comments on commit a3c7f8e

Please sign in to comment.