test: harden tmpdir/rmdirSync error handling #30517
Conversation
This makes rmdirSync call itself after handling ENOTEMPTY, EEXIST, EPERM errors instead of directly using fs.rmdirSync to handle the possibility of such errors repeating on the next try (i.e. FS finished writing new file just as we are done with our loop to delete all files). Refs: nodejs#29852
lundibundi
added
flaky-test
|
CI: https://ci.nodejs.org/job/node-test-pull-request/26643/ (other flakes failed) |
test/common/tmpdir.js
Outdated
| @@ -83,7 +83,7 @@ function rmdirSync(p, originalEr) { | |||
| rimrafSync(path.join(p, f)); | |||
| } | |||
| }); | |||
| fs.rmdirSync(p); | |||
| rmdirSync(p, null); | |||
There was a problem hiding this comment.
Does there need to be some sort of retry limit to avoid infinite loops?
There was a problem hiding this comment.
I thought about it but it should only happen if someone is infinitely (or create-check-retry) creating files in a directory we are trying to remove.
Though, I can add a retry counter as a third argument just to be sure WDYT?
|
New stress test https://ci.nodejs.org/job/node-stress-single-test/17/ Failed: looks like 100 retries is not enough under Though I still think this should decrease the flakiness of this test (and possibly other that rely on tmpdir on windows). |
|
Fwiw, there is a PR to attempt to remove this test-only rimraf entirely: #30074 |
|
@Fishrock123 yeah, but that looks like it has some issues that might not be fixed soon. And I see this issue fail windows CI quite often so I guess we might at least reinforce it. |
An alternative was opened by @cjihrig that uses some other work they landed recently, as well as #30785 (which will hopefully land soon) and might obsolete both #30074 and this PR. |
|
Closing, for now, I'd very much like to see a proper solution from @cjihrig PRs land. |
This makes rmdirSync call itself after handling ENOTEMPTY, EEXIST, EPERM
errors instead of directly using fs.rmdirSync to handle the possibility
of such errors repeating on the next try (i.e. FS finished writing new
file just as we are done with our loop to delete all files).
Refs: #29852
This should handle
test/parallel/test-child-process-fork-exec-path.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes/cc @nodejs/testing