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.

Sign up for GitHub

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

test: remove custom rimraf #30074

Closed
wants to merge 1 commit into from
Closed

test: remove custom rimraf #30074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions test/async-hooks/test-pipeconnectwrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ const { checkInvocations } = require('./hook-checks');
const tmpdir = require('../common/tmpdir');
const net = require('net');

// Spawning messes up `async_hooks` state.
tmpdir.refresh({ spawn: false });
tmpdir.refresh();

const hooks = initHooks();
hooks.enable();
Expand Down
2 changes: 1 addition & 1 deletion test/async-hooks/test-statwatcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const path = require('path');
if (!common.isMainThread)
common.skip('Worker bootstrapping works differently -> different async IDs');

tmpdir.refresh({ spawn: false });
tmpdir.refresh();

const file1 = path.join(tmpdir.path, 'file1');
const file2 = path.join(tmpdir.path, 'file2');
Expand Down
6 changes: 1 addition & 5 deletions test/common/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,11 +898,7 @@ The `tmpdir` module supports the use of a temporary directory for testing.

The realpath of the testing temporary directory.

### refresh(\[opts\])

* `opts` [<Object>][] (optional) Extra options.
* `spawn` [<boolean>][] (default: `true`) Indicates that `refresh` is
allowed to optionally spawn a subprocess.
### refresh()

Deletes and recreates the testing temporary directory.

Expand Down
49 changes: 9 additions & 40 deletions test/common/tmpdir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,10 @@
/* eslint-disable node-core/require-common-first, node-core/required-modules */
'use strict';

const { execSync } = require('child_process');
const fs = require('fs');
const path = require('path');
const { debuglog } = require('util');
const { isMainThread } = require('worker_threads');

const debug = debuglog('test/tmpdir');

function rimrafSync(pathname, { spawn = true } = {}) {
const st = (() => {
try {
return fs.lstatSync(pathname);
} catch (e) {
if (fs.existsSync(pathname))
throw new Error(`Something wonky happened rimrafing ${pathname}`);
debug(e);
}
})();

// If (!st) then nothing to do.
if (!st) {
return;
}

// On Windows first try to delegate rmdir to a shell.
if (spawn && process.platform === 'win32' && st.isDirectory()) {
try {
// Try `rmdir` first.
execSync(`rmdir /q /s ${pathname}`, { timeout: 1000 });
} catch (e) {
// Attempt failed. Log and carry on.
debug(e);
}
}

fs.rmdirSync(pathname, { recursive: true, maxRetries: 5 });

if (fs.existsSync(pathname))
throw new Error(`Unable to rimraf ${pathname}`);
}

const testRoot = process.env.NODE_TEST_DIR ?
fs.realpathSync(process.env.NODE_TEST_DIR) : path.resolve(__dirname, '..');

Expand All @@ -52,8 +15,14 @@ const tmpdirName = '.tmp.' +
const tmpPath = path.join(testRoot, tmpdirName);

let firstRefresh = true;
function refresh(opts = {}) {
rimrafSync(this.path, opts);
function refresh() {
try {
fs.rmdirSync(this.path, { recursive: true });
Copy link
Contributor

@cjihrig cjihrig Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott could setting maxBusyTries here (and other call sites) help the issue with Windows in the CI?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I tried that and it seemed like maybe it was ignored in the synchronous version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It looks like all of the retry logic is in the async version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the solution to pull the retry logic into the sync version as well? @isaacs was there a reason it only exists in the async version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it turns out there is some hard coded retry logic for the sync version:

node/lib/internal/fs/rimraf.js

Lines 214 to 218 in f171112

for (let i = 0; i < numRetries; i++) {
try {
return rmdirSync(path, options);
} catch {} // Ignore errors.
}
. I'm guessing that part of the issue with synchronous retry logic is that there is no synchronous setTimeout() (Node would need a sleep API I think).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move some of the sync implementation into C and use usleep, or just expose sleep for internal use only?

I'm guessing it's an intentional design decision that Node has no sleep?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two ideas, but I think they're both problematic:

  • We do have common.busyLoop() so if we put some retry logic in common/tmpdir.js, that could be used. Seems like a suboptimal hack, though.

  • Since this is all happening on cleanup, we could also decide to remove the cleanup functionality. The upside is that if someone wants to check something about the tmp files in an exit handler, they don't have to worry about another exit handler removing the tmp files before they get a chance to do the check. The downside, of course, is that we'll have all these tmp directories left lying around.

I don't think either of those are the way to go, but if they help anyone else arrive at a better idea, cool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing it's an intentional design decision that Node has no sleep?

I don't know if it's intentional or not. libuv has a uv_sleep() function in its test runner. I can look into exposing it. I think sleep() is a reasonable thing for Node to have outside of this use case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig I think this could be great 👍 and we start to gain some benefits from pulling rimraf in to core 😄

} catch (e) {
if (e.code !== 'ENOENT') {
throw e;
}
}
fs.mkdirSync(this.path);

if (firstRefresh) {
Expand All @@ -70,7 +39,7 @@ function onexit() {
process.chdir(testRoot);

try {
rimrafSync(tmpPath, { spawn: false });
fs.rmdirSync(tmpPath, { recursive: true });
} catch (e) {
console.error('Can\'t clean tmpdir:', tmpPath);

Expand Down