fs: fix busy loop in recursive rm for windows by RaisinTen · Pull Request #36815 · nodejs/node · GitHub

cc @nodejs/fs

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

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'
    }

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

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.

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

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

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.

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

No, it does not reset the permissions.

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

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

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

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

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

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

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

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

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.

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. ;)

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

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

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

Stale

Fixes: nodejs#34580

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

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

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

Closing in favour of #38684

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 6, 2021

richardlau reviewed Jan 6, 2021

View reviewed changes

Comment thread test/parallel/test-rmdirSync-busy-loop-windows.js Outdated

richardlau reviewed Jan 6, 2021

View reviewed changes

Comment thread test/parallel/test-rmdirSync-busy-loop-windows.js Outdated

BridgeAR reviewed Jan 6, 2021

View reviewed changes

Comment thread lib/internal/fs/rimraf.js Outdated

RaisinTen added the review wanted PRs that need reviews. label Jan 17, 2021

aduh95 approved these changes Jan 18, 2021

View reviewed changes

Comment thread lib/internal/fs/rimraf.js Outdated

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 20, 2021

ronag approved these changes Jan 20, 2021

View reviewed changes

aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 20, 2021

richardlau mentioned this pull request Jan 21, 2021

Builds failing on test-azure_msft-win10_vs2019-x64-4 nodejs/build#2526

Closed

richardlau previously requested changes Jan 21, 2021

View reviewed changes

Trott reviewed Jan 21, 2021

View reviewed changes

Comment thread test/parallel/test-rmdirSync-busy-loop-windows.js Outdated

RaisinTen commented Jan 22, 2021

View reviewed changes

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

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

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 force-pushed the fs/fix-busy-loop-in-rmdirSync-for-windows branch from d71ec14 to 7f0eccd Compare March 1, 2021 15:54

RaisinTen changed the title fs: fix busy loop in rmdirSync for windows fs: fix busy loop in recursive rm for windows Mar 1, 2021

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

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

RaisinTen mentioned this pull request May 15, 2021

fs: fix rmsync error swallowing #38684

Closed

fs: fix busy loop in recursive rm for windows

5572faa

Fixes: nodejs#34580

RaisinTen force-pushed the fs/fix-busy-loop-in-rmdirSync-for-windows branch from a474fcd to 5572faa Compare May 18, 2021 14:00

fixup! fs: fix busy loop in recursive rm for windows

27e573a

fixup! fixup! fs: fix busy loop in recursive rm for windows

c3c99a5

RaisinTen closed this May 20, 2021

RaisinTen deleted the fs/fix-busy-loop-in-rmdirSync-for-windows branch May 20, 2021 16:09

fs: fix busy loop in recursive rm for windows by RaisinTen · Pull Request #36815 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

fs: fix busy loop in recursive rm for windows#36815RaisinTen wants to merge 3 commits intonodejs:masternodejs/node:masterfrom RaisinTen:fs/fix-busy-loop-in-rmdirSync-for-windowsRaisinTen/node:fs/fix-busy-loop-in-rmdirSync-for-windowsCopy head branch name to clipboard

Conversation

RaisinTen commented Jan 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RaisinTen commented Jan 9, 2021

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 20, 2021

Uh oh!

aduh95 commented Jan 20, 2021

Uh oh!

richardlau Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

Trott Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

richardlau Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

richardlau Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 21, 2021

Uh oh!

RaisinTen Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 22, 2021

Uh oh!

nodejs-github-bot commented Jan 22, 2021

Uh oh!

nodejs-github-bot commented Jan 28, 2021

Uh oh!

nodejs-github-bot commented Jan 28, 2021

Uh oh!

nodejs-github-bot commented Jan 28, 2021

Uh oh!

RaisinTen commented Feb 10, 2021

Uh oh!

aduh95 commented Feb 12, 2021

Uh oh!

RaisinTen commented Feb 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 1, 2021

Uh oh!

nodejs-github-bot commented Mar 1, 2021

Uh oh!

nodejs-github-bot commented Mar 26, 2021

Uh oh!

nodejs-github-bot commented May 18, 2021

Uh oh!

nodejs-github-bot commented May 18, 2021

Uh oh!

nodejs-github-bot commented May 18, 2021

Uh oh!

RaisinTen commented May 20, 2021

Uh oh!

Reviewers

Assignees

Labels

fs: fix busy loop in recursive rm for windows#36815
RaisinTen wants to merge 3 commits into
nodejs:masterfrom
RaisinTen:fs/fix-busy-loop-in-rmdirSync-for-windows

RaisinTen Jan 22, 2021 •
edited
Loading

RaisinTen commented Feb 13, 2021 •
edited
Loading