Content-Length: 317826 | pFad | http://github.com/nodejs/node/pull/57103

F7 fs: fix rmSync error code by koplas · Pull Request #57103 · nodejs/node · GitHub
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: fix rmSync error code #57103

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fs: fix rmSync error code #57103

wants to merge 1 commit into from

Conversation

koplas
Copy link

@koplas koplas commented Feb 17, 2025

Return the correct error code, when a directory_not_empty error occurred.

Fixes: #57095

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Feb 17, 2025
Copy link
Member

@jazelly jazelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you please add a test

@koplas
Copy link
Author

koplas commented Feb 17, 2025

It doesn't seem very easy to test. This error only occurs for me if some files were added to the folder while RmSync was called. Reproducing this can result in flaky tests, as the error condition is timing-dependent.
I will take a look at how to reproduce this error reliably.

Update: There is already a test for this that is now failing. I will update the test.

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.09%. Comparing base (1d8593e) to head (55719ed).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57103      +/-   ##
==========================================
- Coverage   89.09%   89.09%   -0.01%     
==========================================
  Files         665      665              
  Lines      193249   193250       +1     
  Branches    37231    37232       +1     
==========================================
- Hits       172175   172172       -3     
- Misses      13802    13813      +11     
+ Partials     7272     7265       -7     
Files with missing lines Coverage Δ
src/node_file.cc 76.85% <0.00%> (-0.04%) ⬇️

... and 24 files with indirect coverage changes

@koplas
Copy link
Author

koplas commented Feb 18, 2025

The behavior of std::filesystem::remove_all seems to differ between operating systems. There is already a unique case for Windows. It may be necessary to introduce this for Linux and macOS.

The libcxx implementation ignores errors when deleting files that can not be deleted:
https://github.com/llvm/llvm-project/blob/251377c47d296000158347d6a1cba2e6b30132a3/libcxx/src/filesystem/operations.cpp#L886-L951
While the libstdc++ returns an error if a file cannot be deleted:
https://github.com/gcc-mirror/gcc/blob/b22f191b7c594b33fb4b4a07769dbf0ca45bc9e9/libstdc%2B%2B-v3/src/c%2B%2B17/fs_ops.cc#L1363-L1409

I will make the required changes and check how the test runs under Windows. But first, I will ask the libcxx folks if this is expected behavior or if it also needs fixing.

Return the correct error code, when a directory_not_empty error
occurred.

Fixes: nodejs#57095
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs.rmSync returns wrong error code for not empty directory
3 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/nodejs/node/pull/57103

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy