diff --git a/doc/docgen.zig b/doc/docgen.zig index 07636fd1527a..c2fef8234984 100644 --- a/doc/docgen.zig +++ b/doc/docgen.zig @@ -99,7 +99,7 @@ pub fn main() !void { var toc = try genToc(allocator, &tokenizer); try fs.cwd().makePath(tmp_dir_name); - defer fs.cwd().deleteTree(tmp_dir_name) catch {}; + defer fs.cwd().deleteTree(tmp_dir_name, .{}) catch {}; try genHtml(allocator, &tokenizer, &toc, buffered_writer.writer(), zig_exe, opt_zig_lib_dir, do_code_tests); try buffered_writer.flush(); diff --git a/lib/std/Build.zig b/lib/std/Build.zig index 34806adf488f..ca043b2c770b 100644 --- a/lib/std/Build.zig +++ b/lib/std/Build.zig @@ -776,7 +776,7 @@ fn makeUninstall(uninstall_step: *Step, prog_node: *std.Progress.Node) anyerror! if (self.verbose) { log.info("rm {s}", .{full_path}); } - fs.cwd().deleteTree(full_path) catch {}; + fs.cwd().deleteTree(full_path, .{}) catch {}; } // TODO remove empty directories diff --git a/lib/std/Build/RemoveDirStep.zig b/lib/std/Build/RemoveDirStep.zig index a5bf3c32565d..fbbf69a51238 100644 --- a/lib/std/Build/RemoveDirStep.zig +++ b/lib/std/Build/RemoveDirStep.zig @@ -28,7 +28,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void { const b = step.owner; const self = @fieldParentPtr(RemoveDirStep, "step", step); - b.build_root.handle.deleteTree(self.dir_path) catch |err| { + b.build_root.handle.deleteTree(self.dir_path, .{}) catch |err| { if (b.build_root.path) |base| { return step.fail("unable to recursively delete path '{s}/{s}': {s}", .{ base, self.dir_path, @errorName(err), diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 290eb151f7bf..ce16c698adb2 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -2089,6 +2089,10 @@ pub const Dir = struct { FileBusy, DeviceBusy, + /// The number of retries to delete thought-to-be-empty directories + /// exceeded `max_retries`. + TooManyRetries, + /// One of the path components was not a directory. /// This error is unreachable if `sub_path` does not contain a path separator. NotDir, @@ -2101,17 +2105,36 @@ pub const Dir = struct { BadPathName, } || os.UnexpectedError; + pub const DeleteTreeOptions = struct { + /// After successfully deleting all of a directories children, it is possible for + /// the directory to not actually be empty (either from a new child being added while + /// iterating or a child being in a `DELETE_PENDING` state on Windows). In this scenario, + /// the directory will be iterated again and then will be attempted to be deleted again. + /// This provides a limit to the total number of such retries. + /// The retry limit is per-directory. + /// + /// Note: A sufficiently adversarial process could still cause an effectively + /// infinite loop by doing something like continually creating deeply nested directory + /// structures within the to-be-deleted tree as it is being deleted. + max_retries: usize = 3, + + /// When initially trying to delete `sub_path`, it will first try and delete it as + /// the provided kind. + kind_hint: File.Kind = .File, + }; + /// Whether `full_path` describes a symlink, file, or directory, this function /// removes it. If it cannot be removed because it is a non-empty directory, /// 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 { - var initial_iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, .File)) orelse return; + pub fn deleteTree(self: Dir, sub_path: []const u8, options: DeleteTreeOptions) DeleteTreeError!void { + var initial_iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, options.kind_hint)) orelse return; const StackItem = struct { name: []const u8, parent_dir: Dir, iter: IterableDir.Iterator, + retries_remaining: usize, }; var stack = std.BoundedArray(StackItem, 16){}; @@ -2125,6 +2148,7 @@ pub const Dir = struct { .name = sub_path, .parent_dir = self, .iter = initial_iterable_dir.iterateAssumeFirstIteration(), + .retries_remaining = options.max_retries, }); process_stack: while (stack.len != 0) { @@ -2162,10 +2186,14 @@ pub const Dir = struct { .name = entry.name, .parent_dir = top.iter.dir, .iter = iterable_dir.iterateAssumeFirstIteration(), + .retries_remaining = options.max_retries, }); continue :process_stack; } else |_| { - try top.iter.dir.deleteTreeMinStackSizeWithKindHint(entry.name, entry.kind); + try top.iter.dir.deleteTreeMinStackSize(entry.name, .{ + .max_retries = options.max_retries, + .kind_hint = entry.kind, + }); break :handle_entry; } } else { @@ -2207,6 +2235,7 @@ pub const Dir = struct { // pop the value from the stack. const parent_dir = top.parent_dir; const name = top.name; + const retries_remaining = top.retries_remaining; _ = stack.pop(); var need_to_retry: bool = false; @@ -2217,6 +2246,7 @@ pub const Dir = struct { }; if (need_to_retry) { + if (retries_remaining == 0) return error.TooManyRetries; // 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: { @@ -2282,6 +2312,7 @@ pub const Dir = struct { .name = name, .parent_dir = parent_dir, .iter = iterable_dir.iterateAssumeFirstIteration(), + .retries_remaining = retries_remaining - 1, }); continue :process_stack; } @@ -2290,13 +2321,10 @@ pub const Dir = struct { /// 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.deleteTreeMinStackSizeWithKindHint(sub_path, .File); - } - - fn deleteTreeMinStackSizeWithKindHint(self: Dir, sub_path: []const u8, kind_hint: File.Kind) DeleteTreeError!void { + pub fn deleteTreeMinStackSize(self: Dir, sub_path: []const u8, options: DeleteTreeOptions) DeleteTreeError!void { + var retries_remaining = options.max_retries; start_over: while (true) { - var iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, kind_hint)) orelse return; + var iterable_dir = (try self.deleteTreeOpenInitialSubpath(sub_path, options.kind_hint)) orelse return; var cleanup_dir_parent: ?IterableDir = null; defer if (cleanup_dir_parent) |*d| d.close(); @@ -2386,14 +2414,25 @@ pub const Dir = struct { if (cleanup_dir_parent) |d| { d.dir.deleteDir(dir_name) catch |err| switch (err) { // These two things can happen due to file system race conditions. - error.FileNotFound, error.DirNotEmpty => continue :start_over, + error.FileNotFound, error.DirNotEmpty => { + if (retries_remaining == 0) return error.TooManyRetries; + retries_remaining -= 1; + continue :start_over; + }, else => |e| return e, }; + // Reset retries after successfully deleting a directory so that the + // retries are effectively per-directory. + retries_remaining = options.max_retries; continue :start_over; } else { self.deleteDir(sub_path) catch |err| switch (err) { error.FileNotFound => return, - error.DirNotEmpty => continue :start_over, + error.DirNotEmpty => { + if (retries_remaining == 0) return error.TooManyRetries; + retries_remaining -= 1; + continue :start_over; + }, else => |e| return e, }; return; @@ -2834,7 +2873,7 @@ pub fn deleteTreeAbsolute(absolute_path: []const u8) !void { var dir = try cwd().openDir(dirname, .{}); defer dir.close(); - return dir.deleteTree(path.basename(absolute_path)); + return dir.deleteTree(path.basename(absolute_path), .{}); } /// Same as `Dir.readLink`, except it asserts the path is absolute. diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index c953b529fa18..bd3322a1fc49 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -304,7 +304,7 @@ test "Dir.Iterator but dir is deleted during iteration" { // This is a contrived reproduction, but this could happen outside of the program, in another thread, etc. // If we get an error while trying to delete, we can skip this test (this will happen on platforms // like Windows which will give FileBusy if the directory is currently open for iteration). - tmp.dir.deleteTree("subdir") catch return error.SkipZigTest; + tmp.dir.deleteTree("subdir", .{}) catch return error.SkipZigTest; // Now, when we try to iterate, the next call should return null immediately. const entry = try iterator.next(); @@ -696,7 +696,7 @@ test "makePath, put some files in it, deleteTree" { try tmp.dir.makePath("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c" ++ fs.path.sep_str ++ "file.txt", "nonsense"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "file2.txt", "blah"); - try tmp.dir.deleteTree("os_test_tmp"); + try tmp.dir.deleteTree("os_test_tmp", .{}); if (tmp.dir.openDir("os_test_tmp", .{})) |dir| { _ = dir; @panic("expected error"); @@ -712,7 +712,7 @@ test "makePath, put some files in it, deleteTreeMinStackSize" { try tmp.dir.makePath("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "c" ++ fs.path.sep_str ++ "file.txt", "nonsense"); try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "b" ++ fs.path.sep_str ++ "file2.txt", "blah"); - try tmp.dir.deleteTreeMinStackSize("os_test_tmp"); + try tmp.dir.deleteTreeMinStackSize("os_test_tmp", .{}); if (tmp.dir.openDir("os_test_tmp", .{})) |dir| { _ = dir; @panic("expected error"); @@ -721,12 +721,48 @@ test "makePath, put some files in it, deleteTreeMinStackSize" { } } +test "deleteTree max retries" { + // While TooManyRetries is relevant for non-Windows platforms, it is trickier to + // reproduce the necessary conditions, as getting DirNotEmpty when deleting a directory + // that `deleteTree` thinks is now empty would likely mean something like a file/dir being + // created while the directory is being iterated and its children deleted. + if (builtin.os.tag != .windows) return error.SkipZigTest; + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + + // Make a subdirectory with a file in it and keep the file handle open. + var leaked_handle = blk: { + var dir = try tmp.dir.makeOpenPath("dir", .{}); + defer dir.close(); + + var file = try dir.createFile("neverclose", .{}); + break :blk file; + }; + + { + // We always want to close the file handle even on test failure, but we want it to be + // open for these expectError calls. + defer leaked_handle.close(); + + // Since the file handle was never closed, it will enter into a `DELETE_PENDING` + // state which will cause deleteTree to continually delete the file and directory, + // getting 'success' from the file delete but then DirNotEmpty from the directory + // delete. + try std.testing.expectError(error.TooManyRetries, tmp.dir.deleteTree("dir", .{})); + try std.testing.expectError(error.TooManyRetries, tmp.dir.deleteTreeMinStackSize("dir", .{})); + } + + // Now that the leaked handle has been closed, deleteTree should succeed. + try tmp.dir.deleteTree("dir", .{}); +} + test "makePath in a directory that no longer exists" { if (builtin.os.tag == .windows) return error.SkipZigTest; // Windows returns FileBusy if attempting to remove an open dir var tmp = tmpDir(.{}); defer tmp.cleanup(); - try tmp.parent_dir.deleteTree(&tmp.sub_path); + try tmp.parent_dir.deleteTree(&tmp.sub_path, .{}); try testing.expectError(error.FileNotFound, tmp.dir.makePath("sub-path")); } @@ -751,7 +787,7 @@ fn testFilenameLimits(iterable_dir: IterableDir, maxed_filename: []const u8) !vo } // ensure that we can delete the tree - try iterable_dir.dir.deleteTree(maxed_filename); + try iterable_dir.dir.deleteTree(maxed_filename, .{}); } test "max file name component lengths" { @@ -874,7 +910,7 @@ test "access file" { try tmp.dir.writeFile("os_test_tmp" ++ fs.path.sep_str ++ "file.txt", ""); try tmp.dir.access("os_test_tmp" ++ fs.path.sep_str ++ "file.txt", .{}); - try tmp.dir.deleteTree("os_test_tmp"); + try tmp.dir.deleteTree("os_test_tmp", .{}); } test "sendfile" { @@ -882,7 +918,7 @@ test "sendfile" { defer tmp.cleanup(); try tmp.dir.makePath("os_test_tmp"); - defer tmp.dir.deleteTree("os_test_tmp") catch {}; + defer tmp.dir.deleteTree("os_test_tmp", .{}) catch {}; var dir = try tmp.dir.openDir("os_test_tmp", .{}); defer dir.close(); @@ -947,7 +983,7 @@ test "copyRangeAll" { defer tmp.cleanup(); try tmp.dir.makePath("os_test_tmp"); - defer tmp.dir.deleteTree("os_test_tmp") catch {}; + defer tmp.dir.deleteTree("os_test_tmp", .{}) catch {}; var dir = try tmp.dir.openDir("os_test_tmp", .{}); defer dir.close(); diff --git a/lib/std/fs/watch.zig b/lib/std/fs/watch.zig index 469244fcd640..24174f7107c7 100644 --- a/lib/std/fs/watch.zig +++ b/lib/std/fs/watch.zig @@ -643,7 +643,7 @@ test "write a file, watch it, write it again, delete it" { if (builtin.single_threaded) return error.SkipZigTest; try std.fs.cwd().makePath(test_tmp_dir); - defer std.fs.cwd().deleteTree(test_tmp_dir) catch {}; + defer std.fs.cwd().deleteTree(test_tmp_dir, .{}) catch {}; return testWriteWatchWriteDelete(std.testing.allocator); } diff --git a/lib/std/testing.zig b/lib/std/testing.zig index 37e15ff08b26..64803e1a7062 100644 --- a/lib/std/testing.zig +++ b/lib/std/testing.zig @@ -519,7 +519,7 @@ pub const TmpDir = struct { pub fn cleanup(self: *TmpDir) void { self.dir.close(); - self.parent_dir.deleteTree(&self.sub_path) catch {}; + self.parent_dir.deleteTree(&self.sub_path, .{}) catch {}; self.parent_dir.close(); self.* = undefined; } @@ -535,7 +535,7 @@ pub const TmpIterableDir = struct { pub fn cleanup(self: *TmpIterableDir) void { self.iterable_dir.close(); - self.parent_dir.deleteTree(&self.sub_path) catch {}; + self.parent_dir.deleteTree(&self.sub_path, .{}) catch {}; self.parent_dir.close(); self.* = undefined; } diff --git a/src/Package.zig b/src/Package.zig index 7d98ddaba3b1..721a70648386 100644 --- a/src/Package.zig +++ b/src/Package.zig @@ -719,7 +719,7 @@ fn renameTmpIntoCache( }, error.PathAlreadyExists, error.AccessDenied => { // Package has been already downloaded and may already be in use on the system. - cache_dir.deleteTree(tmp_dir_sub_path) catch |del_err| { + cache_dir.deleteTree(tmp_dir_sub_path, .{}) catch |del_err| { std.log.warn("unable to delete temp directory: {s}", .{@errorName(del_err)}); }; }, diff --git a/src/link.zig b/src/link.zig index 672a53999f5b..9d7637634895 100644 --- a/src/link.zig +++ b/src/link.zig @@ -937,7 +937,7 @@ pub const File = struct { // Work around windows `renameW` can't fail with `PathAlreadyExists` // See https://github.com/ziglang/zig/issues/8362 if (cache_directory.handle.access(o_sub_path, .{})) |_| { - try cache_directory.handle.deleteTree(o_sub_path); + try cache_directory.handle.deleteTree(o_sub_path, .{}); continue; } else |err| switch (err) { error.FileNotFound => {}, @@ -961,7 +961,7 @@ pub const File = struct { o_sub_path, ) catch |err| switch (err) { error.PathAlreadyExists => { - try cache_directory.handle.deleteTree(o_sub_path); + try cache_directory.handle.deleteTree(o_sub_path, .{}); continue; }, else => |e| return e, diff --git a/tools/update_cpu_features.zig b/tools/update_cpu_features.zig index dd1b96fa7c95..fd7ab48bef7c 100644 --- a/tools/update_cpu_features.zig +++ b/tools/update_cpu_features.zig @@ -1198,7 +1198,7 @@ fn processOneTarget(job: Job) anyerror!void { if (all_features.items.len == 0) { // We represent this with an empty file. - try target_dir.deleteTree(zig_code_basename); + try target_dir.deleteTree(zig_code_basename, .{}); return; }