Uh oh!
There was an error while loading. Please reload this page.
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
CI: https://ci.nodejs.org/job/node-test-pull-request/26643/ (other flakes failed) |
| } | ||
| }); | ||
| 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). |
Fishrock123
commented
Nov 19, 2019
Fwiw, there is a PR to attempt to remove this test-only rimraf entirely: #30074 |
lundibundi
commented
Nov 19, 2019
@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. |
nodejs-github-bot
commented
Dec 7, 2019
Trott
commented
Dec 7, 2019
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. |
lundibundi
commented
Dec 8, 2019
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