FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: rimraf should not recurse on failure by bcoe · Pull Request #35566 · nodejs/node · GitHub

/ node Public

fs: rimraf should not recurse on failure - #35566

Closed
bcoe wants to merge 5 commits into
nodejs:masterfrom
bcoe:rimraf-fix
ConversationCommits (5)ChecksFiles changed
Closed

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

Conversation

bcoe commented Oct 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 requested a review from cjihrig October 9, 2020 00:21
nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 9, 2020

bcoe commented Oct 9, 2020

Copy link
Copy Markdown
Contributor Author

CC: @iansu

iansu commented Oct 9, 2020

Copy link
Copy Markdown
Contributor

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

cjihrig approved these changes Oct 9, 2020
View reviewed changes
Comment thread lib/internal/fs/rimraf.js Outdated

bcoe commented Oct 9, 2020

Copy link
Copy Markdown
Contributor Author

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 requested review from Trott and cjihrig October 11, 2020 18:46

bcoe commented Oct 11, 2020

Copy link
Copy Markdown
Contributor Author

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

This comment has been minimized.

Sign in to view
bcoe added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 11, 2020

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
bcoe removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 13, 2020

This comment has been minimized.

Sign in to view
bcoe mentioned this pull request Oct 13, 2020
Closed

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

Copy link
Copy Markdown
Collaborator

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

bcoe added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 13, 2020
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

This comment has been minimized.

Sign in to view
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 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

This comment has been minimized.

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

bcoe commented Oct 13, 2020

Copy link
Copy Markdown
Contributor Author

Landed in 7d66722

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 deleted the rimraf-fix branch October 13, 2020 22:06
bcoe mentioned this pull request Oct 13, 2020
Closed
MylesBorins pushed a commit that referenced this pull request Oct 14, 2020
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 mentioned this pull request Oct 14, 2020
Merged
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
cjihrig Awaiting requested review from cjihrig
+1 more reviewer
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 outstanding review comments, 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

Back | FazBrowse Home | New Git URL