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

fs: fix busy loop in recursive rm for windows #36815

Closed
wants to merge 3 commits into from
Closed

fs: fix busy loop in recursive rm for windows #36815

wants to merge 3 commits into from

Conversation

RaisinTen
Copy link
Contributor

Fixes: #34580

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 6, 2021
BridgeAR
BridgeAR reviewed Jan 6, 2021
View reviewed changes
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
@RaisinTen
Copy link
Contributor Author

cc @nodejs/fs

@RaisinTen RaisinTen added the review wanted PRs that need reviews. label Jan 17, 2021
aduh95
aduh95 approved these changes Jan 18, 2021
View reviewed changes
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Jan 20, 2021

Test is failing on Windows CI:

not ok 520 parallel/test-rmdirSync-busy-loop-windows
  ---
  duration_ms: 0.167
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:119
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Missing expected exception.
        at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-rmdirSync-busy-loop-windows.js:25:8)
        at Module._compile (node:internal/modules/cjs/loader:1108:14)
        at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
        at Module.load (node:internal/modules/cjs/loader:973:32)
        at Function.Module._load (node:internal/modules/cjs/loader:813:14)
        at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
        at node:internal/main/run_main_module:17:47 {
      generatedMessage: false,
      code: 'ERR_ASSERTION',
      actual: undefined,
      expected: /EACCES/,
      operator: 'throws'
    }

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2021
@richardlau richardlau mentioned this pull request Jan 21, 2021
Closed
richardlau
richardlau previously requested changes Jan 21, 2021
View reviewed changes
test/parallel/test-rmdirSync-busy-loop-windows.js Outdated
const middle = join(root, 'middle');
fs.mkdirSync(middle);
fs.mkdirSync(join(middle, 'leaf')); // Make `middle` non-empty
fs.chmodSync(middle, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Stripping access permissions appears to have broken our Windows CI on the host the test ran on as subsequent jobs are unable to clean the workspace (as the user doesn't have permissions to remove this directory): nodejs/build#2526

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that makes sense. tmpdir.refresh() uses the rimraf code that's fixed here, and there's not usually a reason for tests to clean up their temp directories. I guess the problem could be solved by having this test do a tmpdir.refresh() before exiting. And if we forget to ever remove that workaround, it's not a big deal.

Copy link
Member

Choose a reason for hiding this comment

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

Is tmpdir.refresh() able to remove a directory where write permissions have been removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it locally and it does not seem to remove the directory. I think I'll have to change the permission before exiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to change the permission for the directory to 0o777 before removing it and I don't see any temporary directories after this is run locally. PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

If the earlier assertion fails (e.g. as in #36815 (comment)) then the reset of the permissions won't occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it does not reset the permissions.

@nodejs-github-bot
Copy link
Collaborator

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

RaisinTen
RaisinTen commented Jan 22, 2021
View reviewed changes
test/parallel/test-rmdirSync-busy-loop-windows.js Outdated
Comment on lines 23 to 41
try {
assert.throws(() => {
fs.rmdirSync(root, { recursive: true });
}, /EACCES/);
} finally {
fs.chmodSync(middle, 0o777);
fs.rmdirSync(root, { recursive: true });
}
Copy link
Contributor Author

@RaisinTen RaisinTen Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@richardlau This does reset the permissions regardless of the assertion.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen RaisinTen force-pushed the fs/fix-busy-loop-in-rmdirSync-for-windows branch from 9654955 to 28a50fd Compare January 27, 2021 16:06
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen RaisinTen added wip Issues and PRs that are still a work in progress. and removed review wanted PRs that need reviews. labels Feb 10, 2021
@RaisinTen
Copy link
Contributor Author

@aduh95 since you're working on deprecating rmdir's recursive option here: #37302,
I was thinking if I should change this PR to test rm instead. Also, could you please help
me find out why this fails?

@aduh95
Copy link
Contributor

aduh95 commented Feb 12, 2021

I was thinking if I should change this PR to test rm instead

If the same issue exists with rm, I'd say yes, you should that instead.

Also, could you please help me find out why this fails?

So the issue is that Windows doesn't throw a EACCES error (or any error), and you'd expect it to throw because of the previous fs.chmod call, correct? Shouldn't you expect a EPERM error every time? I don't have a Windows machine to investigate very deeply, sorry.

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Feb 13, 2021 •
edited
Loading

If the same issue exists with rm, I'd say yes, you should that instead.

Sure, I'll do that.

So the issue is that Windows doesn't throw a EACCES error (or any error), and you'd expect it to throw because of the previous fs.chmod call, correct? Shouldn't you expect a EPERM error every time? I don't have a Windows machine to investigate very deeply, sorry.

Correct.
Both EACCES and EPERM seems pretty appropriate to me in this case (please correct me if I'm wrong):

  • EACCES because the current user should not have the permission to access the file
  • EPERM because only the root should be able to remove the file

Oddly enough, Windows doesn't throw any of them.
I don't have Windows either. ;)

@aduh95 aduh95 added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. windows Issues and PRs related to the Windows platform. labels Feb 13, 2021
@RaisinTen RaisinTen force-pushed the fs/fix-busy-loop-in-rmdirSync-for-windows branch from d71ec14 to 7f0eccd Compare March 1, 2021 15:54
@RaisinTen RaisinTen changed the title fs: fix busy loop in rmdirSync for windows fs: fix busy loop in recursive rm for windows Mar 1, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen RaisinTen force-pushed the fs/fix-busy-loop-in-rmdirSync-for-windows branch from 5155d42 to a474fcd Compare March 26, 2021 14:33
@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen RaisinTen mentioned this pull request May 15, 2021
Closed
@RaisinTen RaisinTen force-pushed the fs/fix-busy-loop-in-rmdirSync-for-windows branch from a474fcd to 5572faa Compare May 18, 2021 14:00
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen
Copy link
Contributor Author

Closing in favour of #38684

@RaisinTen RaisinTen closed this May 20, 2021
@RaisinTen RaisinTen deleted the fs/fix-busy-loop-in-rmdirSync-for-windows branch May 20, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@richardlau richardlau richardlau left review comments

@BridgeAR BridgeAR BridgeAR left review comments

@ronag ronag ronag approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. windows Issues and PRs related to the Windows platform. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fs.rmdirSync stuck in busy-loop on Windows
7 participants
@RaisinTen @nodejs-github-bot @aduh95 @Trott @ronag @richardlau @BridgeAR