Skip to content

Windows: Iterative remove_dir_all #96412

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

Merged
merged 3 commits into from
Jun 25, 2022
Merged

Windows: Iterative remove_dir_all #96412

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b1f85c
Windows: Iterative `remove_dir_all`
ChrisDenton Apr 25, 2022
8dc4696
Retry deleting a directory
ChrisDenton Apr 26, 2022
d579665
Yield the thread when waiting to delete a file
ChrisDenton Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 81 additions & 75 deletions library/std/src/sys/windows/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::sys::handle::Handle;
use crate::sys::time::SystemTime;
use crate::sys::{c, cvt};
use crate::sys_common::{AsInner, FromInner, IntoInner};
use crate::thread;

use super::path::maybe_verbatim;
use super::to_u16s;
Expand Down Expand Up @@ -680,7 +681,7 @@ impl<'a> DirBuffIter<'a> {
}
}
impl<'a> Iterator for DirBuffIter<'a> {
type Item = &'a [u16];
type Item = (&'a [u16], bool);
fn next(&mut self) -> Option<Self::Item> {
use crate::mem::size_of;
let buffer = &self.buffer?[self.cursor..];
Expand All @@ -689,14 +690,16 @@ impl<'a> Iterator for DirBuffIter<'a> {
// SAFETY: The buffer contains a `FILE_ID_BOTH_DIR_INFO` struct but the
// last field (the file name) is unsized. So an offset has to be
// used to get the file name slice.
let (name, next_entry) = unsafe {
let (name, is_directory, next_entry) = unsafe {
let info = buffer.as_ptr().cast::<c::FILE_ID_BOTH_DIR_INFO>();
let next_entry = (*info).NextEntryOffset as usize;
let name = crate::slice::from_raw_parts(
(*info).FileName.as_ptr().cast::<u16>(),
(*info).FileNameLength as usize / size_of::<u16>(),
);
(name, next_entry)
let is_directory = ((*info).FileAttributes & c::FILE_ATTRIBUTE_DIRECTORY) != 0;

(name, is_directory, next_entry)
};

if next_entry == 0 {
Expand All @@ -709,7 +712,7 @@ impl<'a> Iterator for DirBuffIter<'a> {
const DOT: u16 = b'.' as u16;
match name {
[DOT] | [DOT, DOT] => self.next(),
_ => Some(name),
_ => Some((name, is_directory)),
}
}
}
Expand Down Expand Up @@ -994,89 +997,92 @@ pub fn remove_dir_all(path: &Path) -> io::Result<()> {
if (file.basic_info()?.FileAttributes & c::FILE_ATTRIBUTE_DIRECTORY) == 0 {
return Err(io::Error::from_raw_os_error(c::ERROR_DIRECTORY as _));
}
let mut delete: fn(&File) -> io::Result<()> = File::posix_delete;
let result = match delete(&file) {
Err(e) if e.kind() == io::ErrorKind::DirectoryNotEmpty => {
match remove_dir_all_recursive(&file, delete) {
// Return unexpected errors.
Err(e) if e.kind() != io::ErrorKind::DirectoryNotEmpty => return Err(e),
result => result,
}
}
// If POSIX delete is not supported for this filesystem then fallback to win32 delete.
Err(e)
if e.raw_os_error() == Some(c::ERROR_NOT_SUPPORTED as i32)
|| e.raw_os_error() == Some(c::ERROR_INVALID_PARAMETER as i32) =>
{
delete = File::win32_delete;
Err(e)
}
result => result,
};
if result.is_ok() {
Ok(())
} else {
// This is a fallback to make sure the directory is actually deleted.
// Otherwise this function is prone to failing with `DirectoryNotEmpty`
// due to possible delays between marking a file for deletion and the
// file actually being deleted from the filesystem.
//
// So we retry a few times before giving up.
for _ in 0..5 {
match remove_dir_all_recursive(&file, delete) {
Err(e) if e.kind() == io::ErrorKind::DirectoryNotEmpty => {}
result => return result,

match remove_dir_all_iterative(&file, File::posix_delete) {
Err(e) => {
if let Some(code) = e.raw_os_error() {
match code as u32 {
// If POSIX delete is not supported for this filesystem then fallback to win32 delete.
c::ERROR_NOT_SUPPORTED
| c::ERROR_INVALID_FUNCTION
| c::ERROR_INVALID_PARAMETER => {
remove_dir_all_iterative(&file, File::win32_delete)
}
_ => Err(e),
}
} else {
Err(e)
}
}
// Try one last time.
delete(&file)
ok => ok,
}
}

fn remove_dir_all_recursive(f: &File, delete: fn(&File) -> io::Result<()>) -> io::Result<()> {
fn remove_dir_all_iterative(f: &File, delete: fn(&File) -> io::Result<()>) -> io::Result<()> {
// When deleting files we may loop this many times when certain error conditions occur.
// This allows remove_dir_all to succeed when the error is temporary.
const MAX_RETRIES: u32 = 10;

let mut buffer = DirBuff::new();
let mut restart = true;
// Fill the buffer and iterate the entries.
while f.fill_dir_buff(&mut buffer, restart)? {
for name in buffer.iter() {
// Open the file without following symlinks and try deleting it.
// We try opening will all needed permissions and if that is denied
// fallback to opening without `FILE_LIST_DIRECTORY` permission.
// Note `SYNCHRONIZE` permission is needed for synchronous access.
let mut result =
open_link_no_reparse(&f, name, c::SYNCHRONIZE | c::DELETE | c::FILE_LIST_DIRECTORY);
if matches!(&result, Err(e) if e.kind() == io::ErrorKind::PermissionDenied) {
result = open_link_no_reparse(&f, name, c::SYNCHRONIZE | c::DELETE);
let mut dirlist = vec![f.duplicate()?];

// FIXME: This is a hack so we can push to the dirlist vec after borrowing from it.
fn copy_handle(f: &File) -> mem::ManuallyDrop<File> {
Copy link
Member

Choose a reason for hiding this comment

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

This is gross but I don't see an easy alternative and it's commented with a fixme, so fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I aim to do some bigger changes which I'm hoping will fix this. But my initial attempt was way too big for one PR (it touched a lot of different places and the diff was awful) so this was the compromise solution. I hope it's very temporary.

unsafe { mem::ManuallyDrop::new(File::from_raw_handle(f.as_raw_handle())) }
}

while let Some(dir) = dirlist.last() {
let dir = copy_handle(dir);

// Fill the buffer and iterate the entries.
let more_data = dir.fill_dir_buff(&mut buffer, false)?;
for (name, is_directory) in buffer.iter() {
if is_directory {
let child_dir = open_link_no_reparse(
&dir,
name,
c::SYNCHRONIZE | c::DELETE | c::FILE_LIST_DIRECTORY,
)?;
dirlist.push(child_dir);
} else {
for i in 1..=MAX_RETRIES {
let result = open_link_no_reparse(&dir, name, c::SYNCHRONIZE | c::DELETE);
match result {
Ok(f) => delete(&f)?,
// Already deleted, so skip.
Err(e) if e.kind() == io::ErrorKind::NotFound => break,
// Retry a few times if the file is locked or a delete is already in progress.
Err(e)
if i < MAX_RETRIES
&& (e.raw_os_error() == Some(c::ERROR_DELETE_PENDING as _)
|| e.raw_os_error()
== Some(c::ERROR_SHARING_VIOLATION as _)) => {}
// Otherwise return the error.
Err(e) => return Err(e),
}
thread::yield_now();
Copy link
Member

Choose a reason for hiding this comment

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

I do kind of wonder if it's worth sleeping a bit between these in order to maximize the chance that if it's a temporary failure, it goes away.

I guess in practice this might be good enough, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've had some very mixed feedback about sleeping. Some people seemed quite horrified at the prospect. Though I'm uncertain if it was just the principle of the thing, In any case, I guess it would need some limit to help avoid some possible worst case scenario where we end up sleeping for each and every file. Depending on the version of Windows and the environment, the minimum sleep time may be as much as 16ms.

}
}
match result {
Ok(file) => match delete(&file) {
Err(e) if e.kind() == io::ErrorKind::DirectoryNotEmpty => {
// Iterate the directory's files.
// Ignore `DirectoryNotEmpty` errors here. They will be
// caught when `remove_dir_all` tries to delete the top
// level directory. It can then decide if to retry or not.
match remove_dir_all_recursive(&file, delete) {
Err(e) if e.kind() == io::ErrorKind::DirectoryNotEmpty => {}
result => result?,
}
// If there were no more files then delete the directory.
if !more_data {
if let Some(dir) = dirlist.pop() {
// Retry deleting a few times in case we need to wait for a file to be deleted.
for i in 1..=MAX_RETRIES {
let result = delete(&dir);
if let Err(e) = result {
if i == MAX_RETRIES || e.kind() != io::ErrorKind::DirectoryNotEmpty {
return Err(e);
}
thread::yield_now();
} else {
break;
}
result => result?,
},
// Ignore error if a delete is already in progress or the file
// has already been deleted. It also ignores sharing violations
// (where a file is locked by another process) as these are
// usually temporary.
Err(e)
if e.raw_os_error() == Some(c::ERROR_DELETE_PENDING as _)
|| e.kind() == io::ErrorKind::NotFound
|| e.raw_os_error() == Some(c::ERROR_SHARING_VIOLATION as _) => {}
Err(e) => return Err(e),
}
}
}
// Continue reading directory entries without restarting from the beginning,
restart = false;
}
delete(&f)
Ok(())
}

pub fn readlink(path: &Path) -> io::Result<PathBuf> {
Expand Down