Skip to content

fs: rimraf should not recurse on failure#35566

Closed
bcoe wants to merge 5 commits intonodejs:masterfrom
bcoe:rimraf-fix
Closed

fs: rimraf should not recurse on failure#35566
bcoe wants to merge 5 commits intonodejs:masterfrom
bcoe:rimraf-fix

Conversation

@bcoe
Copy link
Contributor

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

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

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
Contributor Author

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.

@bcoe bcoe requested review from Trott and cjihrig October 11, 2020 18:46
@bcoe
Copy link
Contributor Author

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
@bcoe bcoe 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
@bcoe bcoe 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
@bcoe bcoe 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/ ✅

@bcoe bcoe 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-actions github-actions bot 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
@bcoe bcoe 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-actions github-actions bot 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
@bcoe bcoe removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 13, 2020
@bcoe
Copy link
Contributor Author

bcoe commented Oct 13, 2020

Landed in 7d66722

@bcoe bcoe 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 <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bcoe bcoe deleted the rimraf-fix branch October 13, 2020 22:06
@bcoe bcoe 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 <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins 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 <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

@cjihrig cjihrig Awaiting requested review from cjihrig

+1 more reviewer

@iansu iansu iansu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues 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