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.

Sign up for GitHub

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

Jump to bottom

test: work around ENOTEMPTY when cleaning tmp dir #30849

Closed
wants to merge 1 commit into from
Closed

test: work around ENOTEMPTY when cleaning tmp dir #30849

wants to merge 1 commit into from

Conversation

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

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@cjihrig
Copy link
Contributor

cjihrig commented Dec 8, 2019

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bnoordhuis
Copy link
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
Copy link
Contributor

cjihrig commented Dec 9, 2019

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

bcoe commented Dec 9, 2019

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

cjihrig commented Dec 9, 2019

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

cjihrig commented Dec 10, 2019

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

@cjihrig cjihrig mentioned this pull request Dec 10, 2019
Closed
4 tasks
@cjihrig
Copy link
Contributor

cjihrig commented Dec 10, 2019

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

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

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

@bnoordhuis
Copy link
Member Author

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

@Trott
Copy link
Member

Trott commented Dec 12, 2019

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

Trott
Trott approved these changes Dec 12, 2019
View reviewed changes
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 pending parallel.status question.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2019
@Trott
Copy link
Member

Trott commented Dec 12, 2019

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

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

@Trott
Copy link
Member

Trott commented Dec 12, 2019

Landed in edf654d

@Trott Trott closed this Dec 12, 2019
Trott pushed a commit that referenced this pull request Dec 12, 2019
@bnoordhuis @Trott
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
@bnoordhuis @MylesBorins
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@bnoordhuis @targos
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@bnoordhuis @BethGriggs
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
@bcoe bcoe mentioned this pull request Sep 13, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott 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 pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
5 participants
@bnoordhuis @nodejs-github-bot @cjihrig @bcoe @Trott