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.

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

fs: rimraf should not recurse on failure #35566

Closed
wants to merge 5 commits into from
Closed

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Oct 9, 2020

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.

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.

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

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 13, 2020

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

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

@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
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
MylesBorins pushed a commit that referenced this pull request Oct 14, 2020
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
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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
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
Development

Successfully merging this pull request may close these issues.

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