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: rimraf should not recurse on failure #35566

Closed
wants to merge 5 commits into from
Closed

fs: rimraf should not recurse on failure #35566

wants to merge 5 commits into from

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
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
@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 cjihrig and Trott 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 added a commit that referenced this pull request Oct 13, 2020
@bcoe
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
@bcoe @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
@bcoe @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

@iansu iansu iansu approved these changes

@cjihrig cjihrig Awaiting requested review from cjihrig

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