test: use fs rimraf to refresh tmpdir#30569

cjihrig · 2019-11-20T22:26:39Z

This PR is a revisit to #29235 (which I couldn't reopen since I had force pushed the branch). The goal of that PR was to use core's new rimraf implementation when refreshing the tmpdir in tests. However, that PR hit a snag that I didn't have time to look into:

=== release test-fs-readdir-ucs2 === Path: parallel/test-fs-readdir-ucs2 --- stderr --- Can't clean tmpdir: /home/travis/build/nodejs/node/test/.tmp.602 Files blocking: [ '=�\u0004�' ] /home/travis/build/nodejs/node/test/common/tmpdir.js:88 throw e; ^ Error: Unable to rimraf /home/travis/build/nodejs/node/test/.tmp.602 at rimrafSync (/home/travis/build/nodejs/node/test/common/tmpdir.js:42:11) at process.onexit (/home/travis/build/nodejs/node/test/common/tmpdir.js:73:5) at process.emit (events.js:214:15) Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-fs-readdir-ucs2.js

I addressed that failure in the second commit here (I copied what the rimraf in our test suite does regarding encoding). It seems to be going well in the CI: https://ci.nodejs.org/job/node-test-commit/32928/.

Another alternative to 357e233 might be to introduce an encoding option to rmdir().

cc'ing the people from #29235: @Trott @targos @gengjiawen

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2019-11-22T22:57:31Z

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

nodejs-github-bot · 2019-11-22T23:50:35Z

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

addaleax

Just curious, what goes wrong on Windows when using buffer rather than utf8?

cjihrig · 2019-11-23T14:56:40Z

Just curious, what goes wrong on Windows when using buffer rather than utf8?

I haven't tried that, but I can. It looks like the tests version of rimraf has been using Buffers on Linux and utf8 everywhere else since 060e5f0. @jasnell do you remember why you didn't use Buffers everywhere?

nodejs-github-bot · 2019-11-25T18:12:13Z

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

nodejs-github-bot · 2019-11-25T19:10:13Z

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

cjihrig · 2019-11-25T19:27:37Z

https://ci.nodejs.org/job/node-test-pull-request/26972/ (which uses Buffers across all platforms) came back green. However, that was a resume build of https://ci.nodejs.org/job/node-test-pull-request/26968/, which had one failure on Windows:

13:20:29 not ok 126 parallel/test-child-process-fork-exec-path 13:20:29 --- 13:20:29 duration_ms: 0.498 13:20:29 severity: fail 13:20:29 exitcode: 1 13:20:29 stack: |- 13:20:29 Can't clean tmpdir: c:\workspace\node-test-binary-windows-2\test\.tmp.126 13:20:29 Files blocking: [ 'node-copy.exe' ] 13:20:29 13:20:29 c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:88 13:20:29 throw e; 13:20:29 ^ 13:20:29 13:20:29 Error: EPERM: operation not permitted, unlink '\\?\c:\workspace\node-test-binary-windows-2\test\.tmp.126\node-copy.exe' 13:20:29 at unlinkSync (fs.js:1056:3) 13:20:29 at fixWinEPERMSync (internal/fs/rimraf.js:259:5) 13:20:29 at rimrafSync (internal/fs/rimraf.js:194:14) 13:20:29 at internal/fs/rimraf.js:222:9 13:20:29 at Array.forEach (<anonymous>) 13:20:29 at _rmdirSync (internal/fs/rimraf.js:219:45) 13:20:29 at fixWinEPERMSync (internal/fs/rimraf.js:257:5) 13:20:29 at rimrafSync (internal/fs/rimraf.js:194:14) 13:20:29 at Object.rmdirSync (fs.js:768:12) 13:20:29 at rimrafSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:39:6){13:20:29 errno: -4048, 13:20:29 syscall: 'unlink', 13:20:29 code: 'EPERM', 13:20:29 path: '\\\\?\\c:\\workspace\\node-test-binary-windows-2\\test\\.tmp.126\\node-copy.exe' 13:20:29 } 13:20:29 ...

This was the same failure observed in #30074. I may try to move forward with some of the proposed changes in #30580 first to see if better synchronous retry logic helps here.

nodejs-github-bot · 2019-11-30T00:48:48Z

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

nodejs-github-bot · 2019-12-03T22:44:52Z

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

nodejs-github-bot · 2019-12-03T22:44:52Z

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

nodejs-github-bot · 2019-12-04T00:03:09Z

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

joaocgreis · 2019-12-04T00:03:18Z

Windows CI rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/31958/

Trott

LGTM although I imagine this is blocked until #30785 or something similar lands, as otherwise Windows CI will be very unreliable?

This commit gives the synchronous version of rimraf the same linear retry logic as the asynchronous version. Prior to this commit, sync rimraf kept retrying the operation as soon as possible until maxRetries was reached. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

rimraf should only retry if certain errors are encountered. Additionally, there is no point sleeping if an error occurs on the last try. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

This commit gives the synchronous version of rimraf the same linear retry logic as the asynchronous version. Prior to this commit, sync rimraf kept retrying the operation as soon as possible until maxRetries was reached. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

rimraf should only retry if certain errors are encountered. Additionally, there is no point sleeping if an error occurs on the last try. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

nodejs-github-bot · 2019-12-09T15:52:27Z

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

nodejs-github-bot · 2019-12-10T03:24:45Z

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

EDIT: CI was yellow. Windows CI was green.

PR-URL: nodejs#30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This commit adds synchronous retry logic to the unlinkSync() calls in rimraf. PR-URL: nodejs#30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Now that the functionality is built into core, use it to refresh the test suite's tmpdir. PR-URL: nodejs#30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

cjihrig · 2019-12-10T14:23:50Z

Landed in 7629fb2...4a5fb74.

PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This commit adds synchronous retry logic to the unlinkSync() calls in rimraf. PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Now that the functionality is built into core, use it to refresh the test suite's tmpdir. PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This commit gives the synchronous version of rimraf the same linear retry logic as the asynchronous version. Prior to this commit, sync rimraf kept retrying the operation as soon as possible until maxRetries was reached. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

rimraf should only retry if certain errors are encountered. Additionally, there is no point sleeping if an error occurs on the last try. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This commit adds synchronous retry logic to the unlinkSync() calls in rimraf. PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Now that the functionality is built into core, use it to refresh the test suite's tmpdir. PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This commit gives the synchronous version of rimraf the same linear retry logic as the asynchronous version. Prior to this commit, sync rimraf kept retrying the operation as soon as possible until maxRetries was reached. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

rimraf should only retry if certain errors are encountered. Additionally, there is no point sleeping if an error occurs on the last try. PR-URL: #30785Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This commit adds synchronous retry logic to the unlinkSync() calls in rimraf. PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Now that the functionality is built into core, use it to refresh the test suite's tmpdir. PR-URL: #30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 20, 2019

gengjiawen approved these changes Nov 22, 2019
View reviewed changes

cjihrig force-pushed the tmpdir branch from 357e233 to eb2a148Compare November 22, 2019 22:05

addaleax approved these changes Nov 23, 2019
View reviewed changes

cjihrig force-pushed the tmpdir branch from eb2a148 to 466abd7Compare November 25, 2019 18:09

cjihrig force-pushed the tmpdir branch from 466abd7 to 70c74e3Compare November 28, 2019 02:03

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2019

cjihrig mentioned this pull request Dec 4, 2019
fs: add synchronous retries to rimraf #30785
Closed
2 tasks

Trott approved these changes Dec 4, 2019
View reviewed changes

Trott added the blocked PRs that are blocked by other issues or PRs. label Dec 4, 2019

Trott mentioned this pull request Dec 7, 2019
test: harden tmpdir/rmdirSync error handling #30517
Closed
2 tasks

cjihrig mentioned this pull request Dec 8, 2019
test: work around ENOTEMPTY when cleaning tmp dir #30849
Closed

cjihrig force-pushed the tmpdir branch from 70c74e3 to 7bc376fCompare December 9, 2019 15:50

Trott mentioned this pull request Dec 9, 2019
test: remove custom rimraf #30074
Closed
3 tasks

Trott approved these changes Dec 10, 2019
View reviewed changes

cjihrig added 3 commits December 10, 2019 09:22

fs: only operate on buffers in rimraf
152014a
PR-URL: nodejs#30569 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

cjihrig force-pushed the tmpdir branch from 5c5153e to 4a5fb74Compare December 10, 2019 14:23

cjihrig merged commit 4a5fb74 into nodejs:masterDec 10, 2019

cjihrig deleted the tmpdir branch December 10, 2019 14:24

MylesBorins mentioned this pull request Dec 13, 2019
v13.4.0 proposal #30937
Merged

targos mentioned this pull request Jan 15, 2020
v12.15.0 release proposal #31368
Closed

MylesBorins mentioned this pull request Feb 8, 2020
v12.16.0 proposal #31691
Merged

Uh oh!

test: use fs rimraf to refresh tmpdir#30569

test: use fs rimraf to refresh tmpdir #30569

Conversation

cjihrig commented Nov 20, 2019

Checklist

Uh oh!

nodejs-github-bot commented Nov 22, 2019

Uh oh!

nodejs-github-bot commented Nov 22, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Nov 23, 2019

Uh oh!

nodejs-github-bot commented Nov 25, 2019

Uh oh!

nodejs-github-bot commented Nov 25, 2019

Uh oh!

cjihrig commented Nov 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

nodejs-github-bot commented Dec 3, 2019

Uh oh!

nodejs-github-bot commented Dec 3, 2019

Uh oh!

nodejs-github-bot commented Dec 4, 2019

Uh oh!

joaocgreis commented Dec 4, 2019

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 9, 2019• edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 10, 2019• edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Dec 10, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

cjihrig commented Nov 25, 2019•
edited
Loading

nodejs-github-bot commented Dec 9, 2019•
edited by cjihrig
Loading

nodejs-github-bot commented Dec 10, 2019•
edited by cjihrig
Loading

cjihrig commented Dec 10, 2019•
edited
Loading