Skip to content

fs: rimraf should not recurse on failure#35566

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
Closed
bcoe wants to merge 5 commits into from
Closed

fs: rimraf should not recurse on failure #35566

bcoe wants to merge 5 commits into from

Conversation

@bcoe
Copy link
Contributor

@bcoebcoe commented Oct 9, 2020•
edited
Loading

When an error occurs while running rimrafSync, we pass the original options through to the call to rmdirSync, this in turn spawns another call to rimraf, rather than simply removing the file.

Fixes#34266

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@bcoebcoe requested a review from cjihrigOctober 9, 2020 00:21
@nodejs-github-botnodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 9, 2020
@bcoe
Copy link
ContributorAuthor

bcoe commented Oct 9, 2020

CC: @iansu

@iansu
Copy link
Contributor

iansu commented Oct 9, 2020

I'd love to see a test for this but I realize that might be tricky in this case.

cjihrig
cjihrig approved these changes Oct 9, 2020
View reviewed changes
@bcoe
Copy link
ContributorAuthor

bcoe commented Oct 9, 2020

I'd love to see a test for this but I realize that might be tricky in this case.

@iansu was trying to think of a reasonable way to test this, and an idea doesn't immediately jump out at me that's not terrible.

We lazy load rimraf, so perhaps I could mock fs in the require cache, and then delete the require cache. I'll see what I can do.

@bcoebcoe requested review from Trott and cjihrigOctober 11, 2020 18:46
@bcoe
Copy link
ContributorAuthor

bcoe commented Oct 11, 2020

@Trott@cjihrig@iansu I've added a test, had to shuffle code around a tiny bit to do so -- let me know if you have an idea for a better pattern.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bcoebcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 11, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bcoebcoe removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bcoebcoe mentioned this pull request Oct 13, 2020
Closed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 13, 2020•
edited by bcoe
Loading

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

@bcoebcoe added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 13, 2020
@github-actionsgithub-actionsbot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 13, 2020
@github-actions

This comment has been minimized.

Sign in to view
@bcoebcoe added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 13, 2020
@github-actionsgithub-actionsbot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 13, 2020
@github-actions

This comment has been minimized.

Sign in to view
@bcoebcoe removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 13, 2020
@bcoe
Copy link
ContributorAuthor

bcoe commented Oct 13, 2020

Landed in 7d66722

@bcoebcoe closed this Oct 13, 2020
bcoe pushed a commit that referenced this pull request Oct 13, 2020
fs: rimraf should not recurse on failure
7d66722
PR-URL: #35566 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@bcoebcoe deleted the rimraf-fix branch October 13, 2020 22:06
@bcoebcoe mentioned this pull request Oct 13, 2020
Closed
MylesBorins pushed a commit that referenced this pull request Oct 14, 2020
@MylesBorins
fs: rimraf should not recurse on failure
a3c7f8e
PR-URL: #35566 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Oct 14, 2020
Merged
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@joesepi
fs: rimraf should not recurse on failure
5f3b5bd
PR-URL: nodejs#35566 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes

@cjihrigcjihrigAwaiting requested review from cjihrig

+1 more reviewer

@iansuiansuiansu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fs.open() can keep the event loop open in some cases

5 participants

@bcoe@iansu@nodejs-github-bot@Trott@cjihrig