Skip to content

Commit 736f773

Browse files
lolbinarycatlolbinarycat
committed
fix: fs::remove_dir_all: treat ENOENT as success
fixes #127576 windows implementation still needs some work
1 parent 5367673 commit 736f773

File tree

8 files changed

+153
-36
lines changed

8 files changed

+153
-36
lines changed

‎library/std/src/fs.rs

+2
Original file line numberDiff line numberDiff line change
@@ -2491,6 +2491,8 @@ pub fn remove_dir<P: AsRef<Path>>(path: P) -> io::Result<()> {
24912491
///
24922492
/// Consider ignoring the error if validating the removal is not required for your use case.
24932493
///
2494+
/// [`io::ErrorKind::NotFound`] is only returned if no removal occurs.
2495+
///
24942496
/// [`fs::remove_file`]: remove_file
24952497
/// [`fs::remove_dir`]: remove_dir
24962498
///

‎library/std/src/sys/pal/solid/fs.rs

+16-7
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use crate::sync::Arc;
1010
use crate::sys::time::SystemTime;
1111
use crate::sys::unsupported;
1212
pub use crate::sys_common::fs::exists;
13+
use crate::sys_common::ignore_notfound;
1314

1415
/// A file descriptor.
1516
#[derive(Clone, Copy)]
@@ -527,15 +528,23 @@ pub fn rmdir(p: &Path) -> io::Result<()> {
527528

528529
pub fn remove_dir_all(path: &Path) -> io::Result<()> {
529530
for child in readdir(path)? {
530-
let child = child?;
531-
let child_type = child.file_type()?;
532-
if child_type.is_dir() {
533-
remove_dir_all(&child.path())?;
534-
} else {
535-
unlink(&child.path())?;
531+
let result: io::Result<()> = try {
532+
let child = child?;
533+
let child_type = child.file_type()?;
534+
if child_type.is_dir() {
535+
remove_dir_all(&child.path())?;
536+
} else {
537+
unlink(&child.path())?;
538+
}
539+
};
540+
// ignore internal NotFound errors
541+
if let Err(err) = result
542+
&& err.kind() != io::ErrorKind::NotFound
543+
{
544+
return result;
536545
}
537546
}
538-
rmdir(path)
547+
ignore_notfound(rmdir(path))
539548
}
540549

541550
pub fn readlink(p: &Path) -> io::Result<PathBuf> {

‎library/std/src/sys/pal/unix/fs.rs

+34-15
Original file line numberDiff line numberDiff line change
@@ -2029,6 +2029,7 @@ mod remove_dir_impl {
20292029
use crate::path::{Path, PathBuf};
20302030
use crate::sys::common::small_c_string::run_path_with_cstr;
20312031
use crate::sys::{cvt, cvt_r};
2032+
use crate::sys_common::ignore_notfound;
20322033

20332034
pub fn openat_nofollow_dironly(parent_fd: Option<RawFd>, p: &CStr) -> io::Result<OwnedFd> {
20342035
let fd = cvt_r(|| unsafe {
@@ -2082,6 +2083,16 @@ mod remove_dir_impl {
20822083
}
20832084
}
20842085

2086+
fn is_enoent(result: &io::Result<()>) -> bool {
2087+
if let Err(err) = result
2088+
&& matches!(err.raw_os_error(), Some(libc::ENOENT))
2089+
{
2090+
true
2091+
} else {
2092+
false
2093+
}
2094+
}
2095+
20852096
fn remove_dir_all_recursive(parent_fd: Option<RawFd>, path: &CStr) -> io::Result<()> {
20862097
// try opening as directory
20872098
let fd = match openat_nofollow_dironly(parent_fd, &path) {
@@ -2105,27 +2116,35 @@ mod remove_dir_impl {
21052116
for child in dir {
21062117
let child = child?;
21072118
let child_name = child.name_cstr();
2108-
match is_dir(&child) {
2109-
Some(true) => {
2110-
remove_dir_all_recursive(Some(fd), child_name)?;
2111-
}
2112-
Some(false) => {
2113-
cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;
2114-
}
2115-
None => {
2116-
// POSIX specifies that calling unlink()/unlinkat(..., 0) on a directory can succeed
2117-
// if the process has the appropriate privileges. This however can causing orphaned
2118-
// directories requiring an fsck e.g. on Solaris and Illumos. So we try recursing
2119-
// into it first instead of trying to unlink() it.
2120-
remove_dir_all_recursive(Some(fd), child_name)?;
2119+
// we need an inner try block, because if one of these
2120+
// directories has already been deleted, then we need to
2121+
// continue the loop, not return ok.
2122+
let result: io::Result<()> = try {
2123+
match is_dir(&child) {
2124+
Some(true) => {
2125+
remove_dir_all_recursive(Some(fd), child_name)?;
2126+
}
2127+
Some(false) => {
2128+
cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;
2129+
}
2130+
None => {
2131+
// POSIX specifies that calling unlink()/unlinkat(..., 0) on a directory can succeed
2132+
// if the process has the appropriate privileges. This however can causing orphaned
2133+
// directories requiring an fsck e.g. on Solaris and Illumos. So we try recursing
2134+
// into it first instead of trying to unlink() it.
2135+
remove_dir_all_recursive(Some(fd), child_name)?;
2136+
}
21212137
}
2138+
};
2139+
if result.is_err() && !is_enoent(&result) {
2140+
return result;
21222141
}
21232142
}
21242143

21252144
// unlink the directory after removing its contents
2126-
cvt(unsafe {
2145+
ignore_notfound(cvt(unsafe {
21272146
unlinkat(parent_fd.unwrap_or(libc::AT_FDCWD), path.as_ptr(), libc::AT_REMOVEDIR)
2128-
})?;
2147+
}))?;
21292148
Ok(())
21302149
}
21312150

‎library/std/src/sys/pal/wasi/fs.rs

+14-6
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use crate::sys::common::small_c_string::run_path_with_cstr;
1313
use crate::sys::time::SystemTime;
1414
use crate::sys::unsupported;
1515
pub use crate::sys_common::fs::exists;
16-
use crate::sys_common::{AsInner, FromInner, IntoInner};
16+
use crate::sys_common::{ignore_notfound, AsInner, FromInner, IntoInner};
1717
use crate::{fmt, iter, ptr};
1818

1919
pub struct File {
@@ -794,14 +794,22 @@ fn remove_dir_all_recursive(parent: &WasiFd, path: &Path) -> io::Result<()> {
794794
io::const_io_error!(io::ErrorKind::Uncategorized, "invalid utf-8 file name found")
795795
})?;
796796

797-
if entry.file_type()?.is_dir() {
798-
remove_dir_all_recursive(&entry.inner.dir.fd, path.as_ref())?;
799-
} else {
800-
entry.inner.dir.fd.unlink_file(path)?;
797+
let result: io::Result<()> = try {
798+
if entry.file_type()?.is_dir() {
799+
remove_dir_all_recursive(&entry.inner.dir.fd, path.as_ref())?;
800+
} else {
801+
entry.inner.dir.fd.unlink_file(path)?;
802+
}
803+
};
804+
// ignore internal NotFound errors
805+
if let Err(err) = &result
806+
&& err.kind() != io::ErrorKind::NotFound
807+
{
808+
return result;
801809
}
802810
}
803811

804812
// Once all this directory's contents are deleted it should be safe to
805813
// delete the directory tiself.
806-
parent.remove_directory(osstr2str(path.as_ref())?)
814+
ignore_notfound(parent.remove_directory(osstr2str(path.as_ref())?))
807815
}

‎library/std/src/sys/pal/windows/fs.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use crate::sys::handle::Handle;
1414
use crate::sys::path::maybe_verbatim;
1515
use crate::sys::time::SystemTime;
1616
use crate::sys::{c, cvt, Align8};
17-
use crate::sys_common::{AsInner, FromInner, IntoInner};
17+
use crate::sys_common::{ignore_notfound, AsInner, FromInner, IntoInner};
1818
use crate::{fmt, ptr, slice, thread};
1919

2020
pub struct File {
@@ -1160,7 +1160,7 @@ pub fn remove_dir_all(path: &Path) -> io::Result<()> {
11601160
return Err(io::Error::from_raw_os_error(c::ERROR_DIRECTORY as _));
11611161
}
11621162

1163-
match remove_dir_all_iterative(&file, File::posix_delete) {
1163+
match ignore_notfound(remove_dir_all_iterative(&file, File::posix_delete)) {
11641164
Err(e) => {
11651165
if let Some(code) = e.raw_os_error() {
11661166
match code as u32 {

‎library/std/src/sys_common/fs.rs

+15-6
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
use crate::fs;
44
use crate::io::{self, Error, ErrorKind};
55
use crate::path::Path;
6+
use crate::sys_common::ignore_notfound;
67

78
pub(crate) const NOT_FILE_ERROR: Error = io::const_io_error!(
89
ErrorKind::InvalidInput,
@@ -32,14 +33,22 @@ pub fn remove_dir_all(path: &Path) -> io::Result<()> {
3233

3334
fn remove_dir_all_recursive(path: &Path) -> io::Result<()> {
3435
for child in fs::read_dir(path)? {
35-
let child = child?;
36-
if child.file_type()?.is_dir() {
37-
remove_dir_all_recursive(&child.path())?;
38-
} else {
39-
fs::remove_file(&child.path())?;
36+
let result: io::Result<()> = try {
37+
let child = child?;
38+
if child.file_type()?.is_dir() {
39+
remove_dir_all_recursive(&child.path())?;
40+
} else {
41+
fs::remove_file(&child.path())?;
42+
}
43+
};
44+
// ignore internal NotFound errors to prevent race conditions
45+
if let Err(err) = &result
46+
&& err.kind() != io::ErrorKind::NotFound
47+
{
48+
return result;
4049
}
4150
}
42-
fs::remove_dir(path)
51+
ignore_notfound(fs::remove_dir(path))
4352
}
4453

4554
pub fn exists(path: &Path) -> io::Result<bool> {

‎library/std/src/sys_common/mod.rs

+8
Original file line numberDiff line numberDiff line change
@@ -80,3 +80,11 @@ pub fn mul_div_u64(value: u64, numer: u64, denom: u64) -> u64 {
8080
// r < denom, so (denom*numer) is the upper bound of (r*numer)
8181
q * numer + r * numer / denom
8282
}
83+
84+
pub fn ignore_notfound<T>(result: crate::io::Result<T>) -> crate::io::Result<()> {
85+
match result {
86+
Err(err) if err.kind() == crate::io::ErrorKind::NotFound => Ok(()),
87+
Ok(_) => Ok(()),
88+
Err(err) => Err(err),
89+
}
90+
}

‎tests/run-make/remove-dir-all-race/rmake.rs

+62
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
//@ ignore-windows
2+
3+
// This test attempts to make sure that running `remove_dir_all`
4+
// doesn't result in a NotFound error one of the files it
5+
// is deleting is deleted concurrently.
6+
//
7+
// The windows implementation for `remove_dir_all` is significantly
8+
// more complicated, and has not yet been brought up to par with
9+
// the implementation on other platforms, so this test is marked as
10+
// `ignore-windows` until someone more expirenced with windows can
11+
// sort that out.
12+
13+
use std::fs::remove_dir_all;
14+
use std::path::Path;
15+
use std::thread;
16+
use std::time::Duration;
17+
18+
use run_make_support::rfs::{create_dir, write};
19+
use run_make_support::run_in_tmpdir;
20+
21+
fn main() {
22+
let mut race_happened = false;
23+
run_in_tmpdir(|| {
24+
for i in 0..150 {
25+
create_dir("outer");
26+
create_dir("outer/inner");
27+
write("outer/inner.txt", b"sometext");
28+
29+
thread::scope(|scope| {
30+
let t1 = scope.spawn(|| {
31+
thread::sleep(Duration::from_nanos(i));
32+
remove_dir_all("outer").unwrap();
33+
});
34+
35+
let race_happened_ref = &race_happened;
36+
let t2 = scope.spawn(|| {
37+
let r1 = remove_dir_all("outer/inner");
38+
let r2 = remove_dir_all("outer/inner.txt");
39+
if r1.is_ok() && r2.is_err() {
40+
race_happened = true;
41+
}
42+
});
43+
});
44+
45+
assert!(!Path::new("outer").exists());
46+
47+
// trying to remove a nonexistant top-level directory should
48+
// still result in an error.
49+
let Err(err) = remove_dir_all("outer") else {
50+
panic!("removing nonexistant dir did not result in an error");
51+
};
52+
assert_eq!(err.kind(), std::io::ErrorKind::NotFound);
53+
}
54+
});
55+
if !race_happened {
56+
eprintln!(
57+
"WARNING: multithreaded deletion never raced, \
58+
try increasing the number of attempts or \
59+
adjusting the sleep timing"
60+
);
61+
}
62+
}

0 commit comments

Comments
 (0)