★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/30849Nouvelle recherche | Portail wanayoo
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: work around ENOTEMPTY when cleaning tmp dir #30849

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 8, 2019

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

This comment has been hidden.

@nodejs-github-bot

This comment has been hidden.

@cjihrig
Copy link
Contributor

@cjihrig 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

@bnoordhuis bnoordhuis commented Dec 8, 2019

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 hidden.

@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented Dec 9, 2019

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 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
Member

@bcoe 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 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 cjihrig commented Dec 10, 2019

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

@cjihrig
Copy link
Contributor

@cjihrig 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

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
Contributor

@nodejs-github-bot nodejs-github-bot commented Dec 12, 2019

@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented Dec 12, 2019

@Trott
Copy link
Member

@Trott Trott commented Dec 12, 2019

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

Trott
Trott approved these changes Dec 12, 2019
Copy link
Member

@Trott Trott left a comment

LGTM pending parallel.status question.

@Trott
Copy link
Member

@Trott 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 Trott commented Dec 12, 2019

Landed in edf654d

@Trott Trott closed this Dec 12, 2019
Trott added a commit that referenced this issue Dec 12, 2019
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 added a commit that referenced this issue Dec 13, 2019
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 MylesBorins mentioned this pull request Dec 13, 2019
targos added a commit that referenced this issue Jan 14, 2020
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>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
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 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
Projects
None yet
5 participants