Rollup merge of #96412 - ChrisDenton:remove-dir-all, r=thomcc

Windows: Iterative `remove_dir_all`

This will allow better strategies for use of memory and File handles. However, fully taking advantage of that is left to future work.

Note to reviewer: It's probably best to view the `remove_dir_all_recursive` as a new function. The diff is not very helpful (imho).

Author: matthiaskrgr

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

@@ -13,6 +13,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;
@@ -679,7 +680,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..];
@@ -688,14 +689,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 {
@@ -708,7 +711,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)),
         }
     }
 }
@@ -993,89 +996,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> {
+        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();
+                }
             }
-            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> {