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

test: remove custom rimraf #30074

Closed
wants to merge 1 commit into from
Closed

test: remove custom rimraf #30074

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 22, 2019

Use internal recursive rmdir() in the common/tmpdir module we use in
tests.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Oct 22, 2019
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

Overlap with #29235.

@richardlau
Copy link
Member

richardlau commented Oct 23, 2019

https://ci.nodejs.org/job/node-test-binary-windows-2/3646/COMPILED_BY=vs2017-x86,RUNNER=win2012r2,RUN_SUBSET=0/testReport/junit/(root)/test/parallel_test_child_process_fork_exec_path/ looks relevant.

Can't clean tmpdir: c:\workspace\node-test-binary-windows-2\test\.tmp.124
Files blocking: [ 'node-copy.exe' ]

c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:57
    throw e;
    ^

Error: EPERM: operation not permitted, unlink '\\?\c:\workspace\node-test-binary-windows-2\test\.tmp.124\node-copy.exe'
    at unlinkSync (fs.js:1051:3)
    at fixWinEPERMSync (internal/fs/rimraf.js:248:5)
    at rimrafSync (internal/fs/rimraf.js:187:14)
    at internal/fs/rimraf.js:211:9
    at Array.forEach (<anonymous>)
    at _rmdirSync (internal/fs/rimraf.js:210:25)
    at fixWinEPERMSync (internal/fs/rimraf.js:246:5)
    at rimrafSync (internal/fs/rimraf.js:187:14)
    at Object.rmdirSync (fs.js:763:12)
    at process.onexit (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:42:8) {
  errno: -4048,
  syscall: 'unlink',
  code: 'EPERM',
  path: '\\\\?\\c:\\workspace\\node-test-binary-windows-2\\test\\.tmp.124\\node-copy.exe'
}

@Trott
Copy link
Member Author

Trott commented Oct 23, 2019

@Trott Trott added wip Issues and PRs that are still a work in progress. dont-land-on-v10.x labels Oct 23, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@bcoe
Copy link
Contributor

bcoe commented Nov 19, 2019

@Trott great work on this, it would be exciting to dog-food our own rimraf, I'm fairly far along adding coverage for Windows to our windows builds, which might help us better track down the edge-cases that are breaking this build.

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

@cjihrig cjihrig Nov 20, 2019

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:

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 😄

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Dec 9, 2019

I imagine #30569 will be good to land now and that this can be closed once it does.

Use internal recursive rmdir() in the common/tmpdir module we use in
tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants