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: use fs rimraf to refresh tmpdir #30569

Merged
merged 3 commits into from
Dec 10, 2019
Merged

test: use fs rimraf to refresh tmpdir #30569

merged 3 commits into from
Dec 10, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 20, 2019

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 nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 20, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 23, 2019

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 25, 2019

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.

@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joaocgreis
Copy link
Member

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Dec 4, 2019
danbev pushed a commit that referenced this pull request Dec 9, 2019
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danbev pushed a commit that referenced this pull request Dec 9, 2019
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Dec 9, 2019
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Dec 9, 2019
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 9, 2019

@Trott Trott mentioned this pull request Dec 9, 2019
3 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 10, 2019

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
Copy link
Contributor Author

cjihrig commented Dec 10, 2019

Landed in 7629fb2...4a5fb74.

@cjihrig cjihrig merged commit 4a5fb74 into nodejs:master Dec 10, 2019
@cjihrig cjihrig deleted the tmpdir branch December 10, 2019 14:24
targos pushed a commit that referenced this pull request Dec 11, 2019
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]>
targos pushed a commit that referenced this pull request Dec 11, 2019
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]>
targos pushed a commit that referenced this pull request Dec 11, 2019
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]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
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]>
targos pushed a commit that referenced this pull request Jan 14, 2020
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]>
targos pushed a commit that referenced this pull request Jan 14, 2020
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]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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: #30785
Fixes: #30580
Refs: #30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants