FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: work around ENOTEMPTY when cleaning tmp dir by bnoordhuis · Pull Request #30849 · nodejs/node · GitHub

/ node Public

test: work around ENOTEMPTY when cleaning tmp dir - #30849

Closed
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix30620
ConversationCommits (1)ChecksFiles changed
Closed

test: work around ENOTEMPTY when cleaning tmp dir#30849
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix30620

Conversation

bnoordhuis commented Dec 8, 2019

Copy link
Copy Markdown
Member

Replace the homegrown rimrafsync implementation in test/common with
a call to fs.rmdirSync(path, { recursive: true }).

Fixes: #30620
Fixes: #30844

cc @joaocgreis

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Dec 8, 2019

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

cjihrig commented Dec 8, 2019

Copy link
Copy Markdown
Contributor

I think this will fail without the first commit from #30569 due to encodings. And without #30785 it seems (based on CI runs) that Windows will be flaky.

bnoordhuis commented Dec 8, 2019

Copy link
Copy Markdown
Member Author

It's currently failing like this so that sounds plausible.

Error: EPERM: operation not permitted, unlink '\\?\C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.63\node-copy.exe'
    at unlinkSync (fs.js:1066:3)
    at fixWinEPERMSync (internal/fs/rimraf.js:245:5)

This comment has been minimized.

Sign in to view

bnoordhuis commented Dec 9, 2019

Copy link
Copy Markdown
Member Author

Still the same error after rebasing on top of master (now that #30785 is merged):

05:53:31     Error: EPERM: operation not permitted, unlink '\\?\C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.63\node-copy.exe'
05:53:31         at unlinkSync (fs.js:1066:3)
05:53:31         at fixWinEPERMSync (internal/fs/rimraf.js:256:5)

cjihrig commented Dec 9, 2019

Copy link
Copy Markdown
Contributor

Looking at the full stack trace, I think I see what the problem is. In #30785, I updated the synchronous retry logic where the original rimraf code just retried in a loop. But it's possible that that code is never even reached if errors are encountered in the readdirSync() + rimrafSync() loop a few lines up.

bcoe commented Dec 9, 2019

Copy link
Copy Markdown
Contributor

Is there a chance we also run into a race condition on unlinkSync; it looks like we retry with backoff on rmdirSync (for directories) but not for unlinkSync for files.

Seems like using similar logic for both would be appropriate?

cjihrig commented Dec 9, 2019

Copy link
Copy Markdown
Contributor

@bcoe yep, that's exactly what I did in the most recent commit in #30569. On one CI run, it appears to have worked. I'll rerun the CI a few times to verify.

cjihrig commented Dec 10, 2019

Copy link
Copy Markdown
Contributor

@bnoordhuis if you rebase this now things should, hopefully, be working 🤞

cjihrig mentioned this pull request Dec 10, 2019
Closed
4 tasks

cjihrig commented Dec 10, 2019

Copy link
Copy Markdown
Contributor

Can you also remove the options from refresh() in test/common/README.md? See https://github.com/nodejs/node/pull/30888/files#diff-6057afb393d4ecf6a2cc0937eec56877L901

test: work around ENOTEMPTY when cleaning tmp dir
944320a 
Replace the homegrown rimrafsync implementation in test/common with
a call to `fs.rmdirSync(path, { recursive: true })`.

Fixes: nodejs#30620
Fixes: nodejs#30844

nodejs-github-bot commented Dec 12, 2019

Copy link
Copy Markdown
Collaborator

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

bnoordhuis commented Dec 12, 2019

Copy link
Copy Markdown
Member Author

Thanks, Colin. Updated + rebased: https://ci.nodejs.org/job/node-test-pull-request/27586/

Trott commented Dec 12, 2019

Copy link
Copy Markdown
Member

Are the changes to test/parallel/parallel.status intentional or unrelated?

Trott approved these changes Dec 12, 2019
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM pending parallel.status question.

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 12, 2019

Trott commented Dec 12, 2019

Copy link
Copy Markdown
Member

Are the changes to test/parallel/parallel.status intentional or unrelated?

Oh, I see, yeah, it's intentional. Ignore my question.

Trott commented Dec 12, 2019

Copy link
Copy Markdown
Member

Landed in edf654d

Trott closed this Dec 12, 2019
Trott pushed a commit that referenced this pull request Dec 12, 2019
test: work around ENOTEMPTY when cleaning tmp dir
edf654d 
Replace the homegrown rimrafsync implementation in test/common with
a call to `fs.rmdirSync(path, { recursive: true })`.

Fixes: #30620
Fixes: #30844

PR-URL: #30849
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
test: work around ENOTEMPTY when cleaning tmp dir
1b534d5 
Replace the homegrown rimrafsync implementation in test/common with
a call to `fs.rmdirSync(path, { recursive: true })`.

Fixes: #30620
Fixes: #30844

PR-URL: #30849
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Dec 13, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
test: work around ENOTEMPTY when cleaning tmp dir
5b78bf2 
Replace the homegrown rimrafsync implementation in test/common with
a call to `fs.rmdirSync(path, { recursive: true })`.

Fixes: #30620
Fixes: #30844

PR-URL: #30849
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
test: work around ENOTEMPTY when cleaning tmp dir
5fff46a 
Replace the homegrown rimrafsync implementation in test/common with
a call to `fs.rmdirSync(path, { recursive: true })`.

Fixes: #30620
Fixes: #30844

PR-URL: #30849
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
bcoe mentioned this pull request Sep 13, 2020
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky test parallel/test-module-loading-globalpaths investigate flaky test-child-process-fork-exec-path in CI

5 participants

Back | FazBrowse Home | New Git URL