From 5059384b570a4cb554215c8091ff00d77a0ebadf Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Tue, 4 Oct 2022 23:24:19 -0700 Subject: [PATCH 1/9] Introduce IterableDir.iterateAssumeFirstIteration This allows for avoiding an unnecessary lseek (or equivalent) call in places where it can be known that the fd has not had its cursor modified yet. --- lib/std/fs.zig | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index b1e88d2e0109..effa07a2ac55 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -811,6 +811,17 @@ pub const IterableDir = struct { }; pub fn iterate(self: IterableDir) Iterator { + return self.iterateImpl(true); + } + + /// Like `iterate`, but will not reset the directory cursor before the first + /// iteration. This should only be used in cases where it is known that the + /// `IterableDir` has not had its cursor modified yet (e.g. it was just opened). + pub fn iterateAssumeFirstIteration(self: IterableDir) Iterator { + return self.iterateImpl(false); + } + + fn iterateImpl(self: IterableDir, first_iter_start_value: bool) Iterator { switch (builtin.os.tag) { .macos, .ios, @@ -825,20 +836,20 @@ pub const IterableDir = struct { .index = 0, .end_index = 0, .buf = undefined, - .first_iter = true, + .first_iter = first_iter_start_value, }, .linux, .haiku => return Iterator{ .dir = self.dir, .index = 0, .end_index = 0, .buf = undefined, - .first_iter = true, + .first_iter = first_iter_start_value, }, .windows => return Iterator{ .dir = self.dir, .index = 0, .end_index = 0, - .first_iter = true, + .first_iter = first_iter_start_value, .buf = undefined, .name_data = undefined, }, From 8cec8f6dddeec46f609afd42c6024c55e5c126d9 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Tue, 4 Oct 2022 23:30:15 -0700 Subject: [PATCH 2/9] fs.Dir.deleteTree: Reduce the number of failing deleteFile calls There are two parts to this: 1. The deleteFile call on the sub_path has been moved outside the loop, since if the first call fails with `IsDir` then it's very likely that all the subsequent calls will do the same. Instead, if the `openIterableDir` call ever hits `NotDir` after the `deleteFile` hit `IsDir`, then we assume that the tree was deleted at some point and can consider the deleteTree a success. 2. Inside the `dir_it.next()` loop, we look at entry.kind and only try doing the relevant (deleteFile/openIterableDir) operation, but always fall back to the other if we get the relevant error (NotDir/IsDir). --- lib/std/fs.zig | 171 +++++++++++++++++++++++++------------------------ 1 file changed, 87 insertions(+), 84 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index effa07a2ac55..c8a88cd90e4f 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2046,41 +2046,38 @@ pub const Dir = struct { /// this function recursively removes its entries and then tries again. /// This operation is not atomic on most file systems. pub fn deleteTree(self: Dir, sub_path: []const u8) DeleteTreeError!void { - start_over: while (true) { - var got_access_denied = false; - - // First, try deleting the item as a file. This way we don't follow sym links. - if (self.deleteFile(sub_path)) { - return; - } else |err| switch (err) { - error.FileNotFound => return, - error.IsDir => {}, - error.AccessDenied => got_access_denied = true, + // First, try deleting the item as a file. This way we don't follow sym links. + if (self.deleteFile(sub_path)) { + return; + } else |err| switch (err) { + error.FileNotFound => return, + error.IsDir => {}, + error.AccessDenied, + error.InvalidUtf8, + error.SymLinkLoop, + error.NameTooLong, + error.SystemResources, + error.ReadOnlyFileSystem, + error.NotDir, + error.FileSystem, + error.FileBusy, + error.BadPathName, + error.Unexpected, + => |e| return e, + } - error.InvalidUtf8, - error.SymLinkLoop, - error.NameTooLong, - error.SystemResources, - error.ReadOnlyFileSystem, - error.NotDir, - error.FileSystem, - error.FileBusy, - error.BadPathName, - error.Unexpected, - => |e| return e, - } + start_over: while (true) { var iterable_dir = self.openIterableDir(sub_path, .{ .no_follow = true }) catch |err| switch (err) { error.NotDir => { - if (got_access_denied) { - return error.AccessDenied; - } - continue :start_over; + // Somehow the sub_path got changed into a file while we were trying to delete the tree. + // This implies that the dir at the sub_path was deleted at some point so we consider this + // as a successful delete and return. + return; }, error.FileNotFound => { // That's fine, we were trying to remove this directory anyway. - continue :start_over; + return; }, - error.InvalidHandle, error.AccessDenied, error.SymLinkLoop, @@ -2112,63 +2109,69 @@ pub const Dir = struct { // open it, and close the original directory. Repeat. Then start the entire operation over. scan_dir: while (true) { - var dir_it = iterable_dir.iterate(); - while (try dir_it.next()) |entry| { - if (iterable_dir.dir.deleteFile(entry.name)) { - continue; - } else |err| switch (err) { - error.FileNotFound => continue, - - // Impossible because we do not pass any path separators. - error.NotDir => unreachable, - - error.IsDir => {}, - error.AccessDenied => got_access_denied = true, - - error.InvalidUtf8, - error.SymLinkLoop, - error.NameTooLong, - error.SystemResources, - error.ReadOnlyFileSystem, - error.FileSystem, - error.FileBusy, - error.BadPathName, - error.Unexpected, - => |e| return e, - } - - const new_dir = iterable_dir.dir.openIterableDir(entry.name, .{ .no_follow = true }) catch |err| switch (err) { - error.NotDir => { - if (got_access_denied) { - return error.AccessDenied; - } + var dir_it = iterable_dir.iterateAssumeFirstIteration(); + dir_it: while (try dir_it.next()) |entry| { + var treat_as_dir = entry.kind == .Directory; + handle_entry: while (true) { + if (treat_as_dir) { + const new_dir = iterable_dir.dir.openIterableDir(entry.name, .{ .no_follow = true }) catch |err| switch (err) { + error.NotDir => { + treat_as_dir = false; + continue :handle_entry; + }, + error.FileNotFound => { + // That's fine, we were trying to remove this directory anyway. + continue :dir_it; + }, + + error.InvalidHandle, + error.AccessDenied, + error.SymLinkLoop, + error.ProcessFdQuotaExceeded, + error.NameTooLong, + error.SystemFdQuotaExceeded, + error.NoDevice, + error.SystemResources, + error.Unexpected, + error.InvalidUtf8, + error.BadPathName, + error.DeviceBusy, + => |e| return e, + }; + if (cleanup_dir_parent) |*d| d.close(); + cleanup_dir_parent = iterable_dir; + iterable_dir = new_dir; + mem.copy(u8, &dir_name_buf, entry.name); + dir_name = dir_name_buf[0..entry.name.len]; continue :scan_dir; - }, - error.FileNotFound => { - // That's fine, we were trying to remove this directory anyway. - continue :scan_dir; - }, - - error.InvalidHandle, - error.AccessDenied, - error.SymLinkLoop, - error.ProcessFdQuotaExceeded, - error.NameTooLong, - error.SystemFdQuotaExceeded, - error.NoDevice, - error.SystemResources, - error.Unexpected, - error.InvalidUtf8, - error.BadPathName, - error.DeviceBusy, - => |e| return e, - }; - if (cleanup_dir_parent) |*d| d.close(); - cleanup_dir_parent = iterable_dir; - iterable_dir = new_dir; - mem.copy(u8, &dir_name_buf, entry.name); - dir_name = dir_name_buf[0..entry.name.len]; - continue :scan_dir; + } else { + if (iterable_dir.dir.deleteFile(entry.name)) { + continue :dir_it; + } else |err| switch (err) { + error.FileNotFound => continue :dir_it, + + // Impossible because we do not pass any path separators. + error.NotDir => unreachable, + + error.IsDir => { + treat_as_dir = true; + continue :handle_entry; + }, + + error.AccessDenied, + error.InvalidUtf8, + error.SymLinkLoop, + error.NameTooLong, + error.SystemResources, + error.ReadOnlyFileSystem, + error.FileSystem, + error.FileBusy, + error.BadPathName, + error.Unexpected, + => |e| return e, + } + } + } } // Reached the end of the directory entries, which means we successfully deleted all of them. // Now to remove the directory itself. From e9889cd25ff219f2f5ac7a336c4f99a577b19d44 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 5 Oct 2022 03:13:21 -0700 Subject: [PATCH 3/9] fs: Add IterableDir.Iterator.reset --- lib/std/fs.zig | 30 ++++++++++++++++++++++++++++++ lib/std/fs/test.zig | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index c8a88cd90e4f..533aa82d3fc4 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -490,6 +490,12 @@ pub const IterableDir = struct { }; } } + + pub fn reset(self: *Self) void { + self.index = 0; + self.end_index = 0; + self.first_iter = true; + } }, .haiku => struct { dir: Dir, @@ -577,6 +583,12 @@ pub const IterableDir = struct { }; } } + + pub fn reset(self: *Self) void { + self.index = 0; + self.end_index = 0; + self.first_iter = true; + } }, .linux => struct { dir: Dir, @@ -655,6 +667,12 @@ pub const IterableDir = struct { }; } } + + pub fn reset(self: *Self) void { + self.index = 0; + self.end_index = 0; + self.first_iter = true; + } }, .windows => struct { dir: Dir, @@ -727,6 +745,12 @@ pub const IterableDir = struct { }; } } + + pub fn reset(self: *Self) void { + self.index = 0; + self.end_index = 0; + self.first_iter = true; + } }, .wasi => struct { dir: Dir, @@ -806,6 +830,12 @@ pub const IterableDir = struct { }; } } + + pub fn reset(self: *Self) void { + self.index = 0; + self.end_index = 0; + self.cookie = os.wasi.DIRCOOKIE_START; + } }, else => @compileError("unimplemented"), }; diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index a7686080c150..f0fb3e01cc2a 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -219,6 +219,42 @@ test "Dir.Iterator twice" { } } +test "Dir.Iterator reset" { + var tmp_dir = tmpIterableDir(.{}); + defer tmp_dir.cleanup(); + + // First, create a couple of entries to iterate over. + const file = try tmp_dir.iterable_dir.dir.createFile("some_file", .{}); + file.close(); + + try tmp_dir.iterable_dir.dir.makeDir("some_dir"); + + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const allocator = arena.allocator(); + + // Create iterator. + var iter = tmp_dir.iterable_dir.iterate(); + + var i: u8 = 0; + while (i < 2) : (i += 1) { + var entries = std.ArrayList(IterableDir.Entry).init(allocator); + + while (try iter.next()) |entry| { + // We cannot just store `entry` as on Windows, we're re-using the name buffer + // which means we'll actually share the `name` pointer between entries! + const name = try allocator.dupe(u8, entry.name); + try entries.append(.{ .name = name, .kind = entry.kind }); + } + + try testing.expect(entries.items.len == 2); // note that the Iterator skips '.' and '..' + try testing.expect(contains(&entries, .{ .name = "some_file", .kind = .File })); + try testing.expect(contains(&entries, .{ .name = "some_dir", .kind = .Directory })); + + iter.reset(); + } +} + test "Dir.Iterator but dir is deleted during iteration" { var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); From 274d19575ea1ebaea593cdca7c5afa8303153cb4 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 5 Oct 2022 03:17:52 -0700 Subject: [PATCH 4/9] fs: Optimize Dir.deleteTree for non-deeply-nested directories `deleteTree` now uses a stack-allocated stack for the first 16 nested directories, and then falls back to the previous implementation (which only keeps 1 directory open at a time) when it runs out of room in its stack. This allows the function to perform as well as a recursive implementation for most use-cases without needing allocation or introducing the possibility of stack overflow. --- lib/std/fs.zig | 214 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 189 insertions(+), 25 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 533aa82d3fc4..416f3ce3135c 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2076,7 +2076,6 @@ pub const Dir = struct { /// this function recursively removes its entries and then tries again. /// This operation is not atomic on most file systems. pub fn deleteTree(self: Dir, sub_path: []const u8) DeleteTreeError!void { - // First, try deleting the item as a file. This way we don't follow sym links. if (self.deleteFile(sub_path)) { return; } else |err| switch (err) { @@ -2096,31 +2095,196 @@ pub const Dir = struct { => |e| return e, } - start_over: while (true) { - var iterable_dir = self.openIterableDir(sub_path, .{ .no_follow = true }) catch |err| switch (err) { - error.NotDir => { - // Somehow the sub_path got changed into a file while we were trying to delete the tree. - // This implies that the dir at the sub_path was deleted at some point so we consider this - // as a successful delete and return. - return; - }, - error.FileNotFound => { - // That's fine, we were trying to remove this directory anyway. - return; + const StackItem = struct { + name: []const u8, + parent_dir: Dir, + iter: IterableDir.Iterator, + }; + + var stack = std.BoundedArray(StackItem, 16){}; + defer { + for (stack.slice()) |*item| { + item.iter.dir.close(); + } + } + + var initial_iterable_dir = self.openIterableDir(sub_path, .{ .no_follow = true }) catch |err| switch (err) { + error.NotDir => { + // Somehow the sub_path got changed into a file while we were trying to delete the tree. + // This implies that the dir at the sub_path was deleted at some point so we consider this + // as a successful delete and return. + return; + }, + error.FileNotFound => { + // That's fine, we were trying to remove this directory anyway. + return; + }, + error.InvalidHandle, + error.AccessDenied, + error.SymLinkLoop, + error.ProcessFdQuotaExceeded, + error.NameTooLong, + error.SystemFdQuotaExceeded, + error.NoDevice, + error.SystemResources, + error.Unexpected, + error.InvalidUtf8, + error.BadPathName, + error.DeviceBusy, + => |e| return e, + }; + + stack.appendAssumeCapacity(StackItem{ + .name = sub_path, + .parent_dir = self, + .iter = initial_iterable_dir.iterateAssumeFirstIteration(), + }); + + process_stack: while (stack.len != 0) { + var top = &(stack.slice()[stack.len - 1]); + while (try top.iter.next()) |entry| { + var treat_as_dir = entry.kind == .Directory; + handle_entry: while (true) { + if (treat_as_dir) { + if (stack.ensureUnusedCapacity(1)) { + var iterable_dir = top.iter.dir.openIterableDir(entry.name, .{ .no_follow = true }) catch |err| switch (err) { + error.NotDir => { + treat_as_dir = false; + continue :handle_entry; + }, + error.FileNotFound => { + // That's fine, we were trying to remove this directory anyway. + break :handle_entry; + }, + + error.InvalidHandle, + error.AccessDenied, + error.SymLinkLoop, + error.ProcessFdQuotaExceeded, + error.NameTooLong, + error.SystemFdQuotaExceeded, + error.NoDevice, + error.SystemResources, + error.Unexpected, + error.InvalidUtf8, + error.BadPathName, + error.DeviceBusy, + => |e| return e, + }; + stack.appendAssumeCapacity(StackItem{ + .name = entry.name, + .parent_dir = top.iter.dir, + .iter = iterable_dir.iterateAssumeFirstIteration(), + }); + continue :process_stack; + } else |_| { + try top.iter.dir.deleteTreeFallback(entry.name, entry.kind); + break :handle_entry; + } + } else { + if (top.iter.dir.deleteFile(entry.name)) { + break :handle_entry; + } else |err| switch (err) { + error.FileNotFound => break :handle_entry, + + error.NotDir => unreachable, + + error.IsDir => { + treat_as_dir = true; + continue :handle_entry; + }, + + error.AccessDenied, + error.InvalidUtf8, + error.SymLinkLoop, + error.NameTooLong, + error.SystemResources, + error.ReadOnlyFileSystem, + error.FileSystem, + error.FileBusy, + error.BadPathName, + error.Unexpected, + => |e| return e, + } + } + } + } + + top.parent_dir.deleteDir(top.name) catch |err| switch (err) { + error.FileNotFound => {}, + error.DirNotEmpty => { + // reset the iterator and try again + top.iter.reset(); + continue :process_stack; }, - error.InvalidHandle, - error.AccessDenied, - error.SymLinkLoop, - error.ProcessFdQuotaExceeded, - error.NameTooLong, - error.SystemFdQuotaExceeded, - error.NoDevice, - error.SystemResources, - error.Unexpected, - error.InvalidUtf8, - error.BadPathName, - error.DeviceBusy, - => |e| return e, + else => |e| return e, + }; + + top.iter.dir.close(); + _ = stack.pop(); + } + } + + /// Fallback version of deleteTree that is less efficient but works on arbitrarily + /// nested directories without needing recursion or allocation. + fn deleteTreeFallback(self: Dir, sub_path: []const u8, kind_hint: File.Kind) DeleteTreeError!void { + start_over: while (true) { + var iterable_dir = iterable_dir: { + var treat_as_dir = kind_hint == .Directory; + + handle_entry: while (true) { + if (treat_as_dir) { + break :iterable_dir self.openIterableDir(sub_path, .{ .no_follow = true }) catch |err| switch (err) { + error.NotDir => { + treat_as_dir = false; + continue :handle_entry; + }, + error.FileNotFound => { + // That's fine, we were trying to remove this directory anyway. + return; + }, + + error.InvalidHandle, + error.AccessDenied, + error.SymLinkLoop, + error.ProcessFdQuotaExceeded, + error.NameTooLong, + error.SystemFdQuotaExceeded, + error.NoDevice, + error.SystemResources, + error.Unexpected, + error.InvalidUtf8, + error.BadPathName, + error.DeviceBusy, + => |e| return e, + }; + } else { + if (self.deleteFile(sub_path)) { + return; + } else |err| switch (err) { + error.FileNotFound => return, + + error.NotDir => unreachable, + + error.IsDir => { + treat_as_dir = true; + continue :handle_entry; + }, + + error.AccessDenied, + error.InvalidUtf8, + error.SymLinkLoop, + error.NameTooLong, + error.SystemResources, + error.ReadOnlyFileSystem, + error.FileSystem, + error.FileBusy, + error.BadPathName, + error.Unexpected, + => |e| return e, + } + } + } }; var cleanup_dir_parent: ?IterableDir = null; defer if (cleanup_dir_parent) |*d| d.close(); From 39f192d54eba99ba3dfc7066476912633e694d51 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 5 Oct 2022 16:05:02 -0700 Subject: [PATCH 5/9] fs: Reduce IterableDir.Iterator `buf` size to 1024 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was sized large so that `getdents` (and other platforms' equivalents) could provide large amounts of entries per syscall, but some benchmarking seems to indicate that the larger 8192 sizing doesn't actually lead to performance gains outside of edge cases like extremely large amounts of entries within a single directory (e.g. 25,000 files in one directory), and even then the gains are minimal ('./walk-8192 dir-with-tons-of-entries' ran 1.02 ± 0.34 times faster than './walk-1024 dir-with-tons-of-entries'). Note: Sizes 1024 and 2048 had similar performance characteristics, so the smaller of the two was chosen. --- lib/std/fs.zig | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 416f3ce3135c..c75cf88b8417 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -301,7 +301,7 @@ pub const IterableDir = struct { .macos, .ios, .freebsd, .netbsd, .dragonfly, .openbsd, .solaris => struct { dir: Dir, seek: i64, - buf: [8192]u8, // TODO align(@alignOf(os.system.dirent)), + buf: [1024]u8, // TODO align(@alignOf(os.system.dirent)), index: usize, end_index: usize, first_iter: bool, @@ -499,7 +499,7 @@ pub const IterableDir = struct { }, .haiku => struct { dir: Dir, - buf: [8192]u8, // TODO align(@alignOf(os.dirent64)), + buf: [1024]u8, // TODO align(@alignOf(os.dirent64)), index: usize, end_index: usize, first_iter: bool, @@ -594,7 +594,7 @@ pub const IterableDir = struct { dir: Dir, // The if guard is solely there to prevent compile errors from missing `linux.dirent64` // definition when compiling for other OSes. It doesn't do anything when compiling for Linux. - buf: [8192]u8 align(if (builtin.os.tag != .linux) 1 else @alignOf(linux.dirent64)), + buf: [1024]u8 align(if (builtin.os.tag != .linux) 1 else @alignOf(linux.dirent64)), index: usize, end_index: usize, first_iter: bool, @@ -676,7 +676,7 @@ pub const IterableDir = struct { }, .windows => struct { dir: Dir, - buf: [8192]u8 align(@alignOf(os.windows.FILE_BOTH_DIR_INFORMATION)), + buf: [1024]u8 align(@alignOf(os.windows.FILE_BOTH_DIR_INFORMATION)), index: usize, end_index: usize, first_iter: bool, @@ -754,7 +754,7 @@ pub const IterableDir = struct { }, .wasi => struct { dir: Dir, - buf: [8192]u8, // TODO align(@alignOf(os.wasi.dirent_t)), + buf: [1024]u8, // TODO align(@alignOf(os.wasi.dirent_t)), cookie: u64, index: usize, end_index: usize, From 34f180901eaa0fcd5e3bddc6f353604da8bd7e1f Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 5 Oct 2022 16:15:25 -0700 Subject: [PATCH 6/9] fs: deleteTreeFallback -> deleteTreeMinStackSize and make it pub --- lib/std/fs.zig | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index c75cf88b8417..c220200d6a6e 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2178,7 +2178,7 @@ pub const Dir = struct { }); continue :process_stack; } else |_| { - try top.iter.dir.deleteTreeFallback(entry.name, entry.kind); + try top.iter.dir.deleteTreeMinStackSizeWithKindHint(entry.name, entry.kind); break :handle_entry; } } else { @@ -2225,9 +2225,13 @@ pub const Dir = struct { } } - /// Fallback version of deleteTree that is less efficient but works on arbitrarily - /// nested directories without needing recursion or allocation. - fn deleteTreeFallback(self: Dir, sub_path: []const u8, kind_hint: File.Kind) DeleteTreeError!void { + /// Like `deleteTree`, but only keeps one `Iterator` active at a time to minimize the function's stack size. + /// This is slower than `deleteTree` but uses less stack space. + pub fn deleteTreeMinStackSize(self: Dir, sub_path: []const u8) DeleteTreeError!void { + return self.deleteTreeMinStackWithKindHint(sub_path, .File); + } + + fn deleteTreeMinStackSizeWithKindHint(self: Dir, sub_path: []const u8, kind_hint: File.Kind) DeleteTreeError!void { start_over: while (true) { var iterable_dir = iterable_dir: { var treat_as_dir = kind_hint == .Directory; From db0829c15ab883b9efe704414fcf8149a3d83026 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 5 Oct 2022 19:51:43 -0700 Subject: [PATCH 7/9] fs.Dir.deleteTree: Fix some handling of NotDir error in deleteFile calls We don't control sub_path so it may contain directory components; therefore, NotDir is a potential error when acting on sub_path. --- lib/std/fs.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index c220200d6a6e..fb7ae5484891 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2187,6 +2187,7 @@ pub const Dir = struct { } else |err| switch (err) { error.FileNotFound => break :handle_entry, + // Impossible because we do not pass any path separators. error.NotDir => unreachable, error.IsDir => { @@ -2268,8 +2269,6 @@ pub const Dir = struct { } else |err| switch (err) { error.FileNotFound => return, - error.NotDir => unreachable, - error.IsDir => { treat_as_dir = true; continue :handle_entry; @@ -2281,6 +2280,7 @@ pub const Dir = struct { error.NameTooLong, error.SystemResources, error.ReadOnlyFileSystem, + error.NotDir, error.FileSystem, error.FileBusy, error.BadPathName, From 063c5f43e9ff6add28ecbbcd79316ede23af3ae8 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 5 Oct 2022 19:55:23 -0700 Subject: [PATCH 8/9] fs.Dir.deleteTree: Fix FileBusy errors on Windows Windows requires the directory handle to be closed before attempting to delete the directory, so now we do that and then re-open it if we need to retry (from getting DirNotEmpty when trying to delete). --- lib/std/fs.zig | 91 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 83 insertions(+), 8 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index fb7ae5484891..a17e8123b978 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2211,18 +2211,93 @@ pub const Dir = struct { } } - top.parent_dir.deleteDir(top.name) catch |err| switch (err) { + // On Windows, we can't delete until the dir's handle has been closed, so + // close it before we try to delete. + top.iter.dir.close(); + + // In order to avoid double-closing the directory when cleaning up + // the stack in the case of an error, we save the relevant portions and + // pop the value from the stack. + const parent_dir = top.parent_dir; + const name = top.name; + _ = stack.pop(); + + var need_to_retry: bool = false; + parent_dir.deleteDir(name) catch |err| switch (err) { error.FileNotFound => {}, - error.DirNotEmpty => { - // reset the iterator and try again - top.iter.reset(); - continue :process_stack; - }, + error.DirNotEmpty => need_to_retry = false, else => |e| return e, }; - top.iter.dir.close(); - _ = stack.pop(); + if (need_to_retry) { + // Since we closed the handle that the previous iterator used, we + // need to re-open the dir and re-create the iterator. + var iterable_dir = iterable_dir: { + var treat_as_dir = true; + handle_entry: while (true) { + if (treat_as_dir) { + break :iterable_dir parent_dir.openIterableDir(name, .{ .no_follow = true }) catch |err| switch (err) { + error.NotDir => { + treat_as_dir = false; + continue :handle_entry; + }, + error.FileNotFound => { + // That's fine, we were trying to remove this directory anyway. + continue :process_stack; + }, + + error.InvalidHandle, + error.AccessDenied, + error.SymLinkLoop, + error.ProcessFdQuotaExceeded, + error.NameTooLong, + error.SystemFdQuotaExceeded, + error.NoDevice, + error.SystemResources, + error.Unexpected, + error.InvalidUtf8, + error.BadPathName, + error.DeviceBusy, + => |e| return e, + }; + } else { + if (parent_dir.deleteFile(name)) { + continue :process_stack; + } else |err| switch (err) { + error.FileNotFound => continue :process_stack, + + // Impossible because we do not pass any path separators. + error.NotDir => unreachable, + + error.IsDir => { + treat_as_dir = true; + continue :handle_entry; + }, + + error.AccessDenied, + error.InvalidUtf8, + error.SymLinkLoop, + error.NameTooLong, + error.SystemResources, + error.ReadOnlyFileSystem, + error.FileSystem, + error.FileBusy, + error.BadPathName, + error.Unexpected, + => |e| return e, + } + } + } + }; + // We know there is room on the stack since we are just re-adding + // the StackItem that we previously popped. + stack.appendAssumeCapacity(StackItem{ + .name = name, + .parent_dir = parent_dir, + .iter = iterable_dir.iterateAssumeFirstIteration(), + }); + continue :process_stack; + } } } From 1468eb12f339b09baa1155d33694272828f9617a Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Wed, 5 Oct 2022 20:08:53 -0700 Subject: [PATCH 9/9] std.fs.deleteTree: Unify how the initial sub_path is treated between deleteTree/deleteTreeMinStackSize --- lib/std/fs.zig | 163 +++++++++++++++++++------------------------------ 1 file changed, 63 insertions(+), 100 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index a17e8123b978..80e5997482b2 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2076,24 +2076,7 @@ pub const Dir = struct { /// this function recursively removes its entries and then tries again. /// This operation is not atomic on most file systems. pub fn deleteTree(self: Dir, sub_path: []const u8) DeleteTreeError!void { - if (self.deleteFile(sub_path)) { - return; - } else |err| switch (err) { - error.FileNotFound => return, - error.IsDir => {}, - error.AccessDenied, - error.InvalidUtf8, - error.SymLinkLoop, - error.NameTooLong, - error.SystemResources, - error.ReadOnlyFileSystem, - error.NotDir, - error.FileSystem, - error.FileBusy, - error.BadPathName, - error.Unexpected, - => |e| return e, - } + var initial_iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, .File)) orelse return; const StackItem = struct { name: []const u8, @@ -2108,32 +2091,6 @@ pub const Dir = struct { } } - var initial_iterable_dir = self.openIterableDir(sub_path, .{ .no_follow = true }) catch |err| switch (err) { - error.NotDir => { - // Somehow the sub_path got changed into a file while we were trying to delete the tree. - // This implies that the dir at the sub_path was deleted at some point so we consider this - // as a successful delete and return. - return; - }, - error.FileNotFound => { - // That's fine, we were trying to remove this directory anyway. - return; - }, - error.InvalidHandle, - error.AccessDenied, - error.SymLinkLoop, - error.ProcessFdQuotaExceeded, - error.NameTooLong, - error.SystemFdQuotaExceeded, - error.NoDevice, - error.SystemResources, - error.Unexpected, - error.InvalidUtf8, - error.BadPathName, - error.DeviceBusy, - => |e| return e, - }; - stack.appendAssumeCapacity(StackItem{ .name = sub_path, .parent_dir = self, @@ -2309,62 +2266,7 @@ pub const Dir = struct { fn deleteTreeMinStackSizeWithKindHint(self: Dir, sub_path: []const u8, kind_hint: File.Kind) DeleteTreeError!void { start_over: while (true) { - var iterable_dir = iterable_dir: { - var treat_as_dir = kind_hint == .Directory; - - handle_entry: while (true) { - if (treat_as_dir) { - break :iterable_dir self.openIterableDir(sub_path, .{ .no_follow = true }) catch |err| switch (err) { - error.NotDir => { - treat_as_dir = false; - continue :handle_entry; - }, - error.FileNotFound => { - // That's fine, we were trying to remove this directory anyway. - return; - }, - - error.InvalidHandle, - error.AccessDenied, - error.SymLinkLoop, - error.ProcessFdQuotaExceeded, - error.NameTooLong, - error.SystemFdQuotaExceeded, - error.NoDevice, - error.SystemResources, - error.Unexpected, - error.InvalidUtf8, - error.BadPathName, - error.DeviceBusy, - => |e| return e, - }; - } else { - if (self.deleteFile(sub_path)) { - return; - } else |err| switch (err) { - error.FileNotFound => return, - - error.IsDir => { - treat_as_dir = true; - continue :handle_entry; - }, - - error.AccessDenied, - error.InvalidUtf8, - error.SymLinkLoop, - error.NameTooLong, - error.SystemResources, - error.ReadOnlyFileSystem, - error.NotDir, - error.FileSystem, - error.FileBusy, - error.BadPathName, - error.Unexpected, - => |e| return e, - } - } - } - }; + var iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, kind_hint)) orelse return; var cleanup_dir_parent: ?IterableDir = null; defer if (cleanup_dir_parent) |*d| d.close(); @@ -2470,6 +2372,67 @@ pub const Dir = struct { } } + /// On successful delete, returns null. + fn deleteTreeOpenInitialSubpath(self: Dir, sub_path: []const u8, kind_hint: File.Kind) !?IterableDir { + return iterable_dir: { + // Treat as a file by default + var treat_as_dir = kind_hint == .Directory; + + handle_entry: while (true) { + if (treat_as_dir) { + break :iterable_dir self.openIterableDir(sub_path, .{ .no_follow = true }) catch |err| switch (err) { + error.NotDir => { + treat_as_dir = false; + continue :handle_entry; + }, + error.FileNotFound => { + // That's fine, we were trying to remove this directory anyway. + return null; + }, + + error.InvalidHandle, + error.AccessDenied, + error.SymLinkLoop, + error.ProcessFdQuotaExceeded, + error.NameTooLong, + error.SystemFdQuotaExceeded, + error.NoDevice, + error.SystemResources, + error.Unexpected, + error.InvalidUtf8, + error.BadPathName, + error.DeviceBusy, + => |e| return e, + }; + } else { + if (self.deleteFile(sub_path)) { + return null; + } else |err| switch (err) { + error.FileNotFound => return null, + + error.IsDir => { + treat_as_dir = true; + continue :handle_entry; + }, + + error.AccessDenied, + error.InvalidUtf8, + error.SymLinkLoop, + error.NameTooLong, + error.SystemResources, + error.ReadOnlyFileSystem, + error.NotDir, + error.FileSystem, + error.FileBusy, + error.BadPathName, + error.Unexpected, + => |e| return e, + } + } + } + }; + } + /// Writes content to the file system, creating a new file if it does not exist, truncating /// if it already exists. pub fn writeFile(self: Dir, sub_path: []const u8, data: []const u8) !void {