test: remove custom rimraf

[Trott]

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]

CI: https://ci.nodejs.org/job/node-test-pull-request/26155/

[richardlau]

Overlap with #29235.

[richardlau]

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]

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.

I was afraid this would happen on Windows....

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26219/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26220/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26221/

[bcoe]

@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.

[cjihrig]

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

[Trott]

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

[cjihrig]

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

[bcoe]

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?

[cjihrig]

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).

[bcoe]

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?

[Trott]

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.

[cjihrig]

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.

[bcoe]

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/27524/

[Trott]

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