ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
34.37k stars 2.51k forks source link

`fs.Dir.deleteTree` has no upper bound on retries #15465

Open squeek502 opened 1 year ago

squeek502 commented 1 year ago

Zig Version

0.11.0-dev.2777+b95cdf0ae

Steps to Reproduce and Observed Behavior

This was meant to be a follow-up issue of https://github.com/ziglang/zig/pull/13073#discussion_r987838660

This should have a default and user-configurable upper bound of tries for deletion. Otherwise, one can not detect other buggy processes continuously inserting directories or files.

As I understand it, the CI has been running into infinite loops on Windows due to DELETE_PENDING when file handles don't get closed (https://github.com/ziglang/zig/pull/15450, https://github.com/ziglang/zig/pull/15460).

Here's a minimal reproduction:

const std = @import("std");

test {
    var tmp = std.testing.tmpDir(.{});
    defer tmp.cleanup(); // infinite loop while retrying to delete the tmp dir

    var file = try tmp.dir.createFile("neverclose", .{});
    _ = file;
}

Expected Behavior

deleteTree should error with something like error.TooManyRetries after some user-configurable amount of retries.

andrewrk commented 1 year ago

I don't think a max number of retries is the correct solution here. The calling code of deleteTree should instead receive some kind of error message such as "directory contains open file handles" or something to that effect. The idea that you have to choose how many retries to delete things doesn't make any sense. You either delete something, or fail to delete something. That's the API that std lib needs to have.

squeek502 commented 1 year ago

As far as I'm aware, it's not possible to detect a condition like "directory contains open file handles," or otherwise know if the retries will ultimately be successful ahead-of-time. There are currently two main scenarios (there may be more that I'm unaware of) that can cause a retry to happen in the current deleteTree code:

  1. [Windows only] Some file within a to-be-deleted directory is in the DELETE_PENDING state. We can't know if the file will still be in the DELETE_PENDING state on the next retry or not (i.e. it could be opened by some other process that is about to exit).
  2. Some file or directory was created in a to-be-deleted directory while its children were being iterated and deleted. We can't know if more files will be created on the next retry or not.

It would be possible to treat DELETE_PENDING as an error; this is what https://github.com/ziglang/zig/issues/6452 is about. But, it's not fully clear why the second scenario should be retried but not DELETE_PENDING. Another option would be to remove retrying altogether and treat error.DirNotEmpty as an error condition within deleteTree; this would avoid the possibility of infinite loops but make deleteTree fail in scenarios where it could potentially succeed if it used retries.

squeek502 commented 1 year ago

Oh, also something worth noting: between https://github.com/ziglang/zig/pull/13073 and https://github.com/ziglang/zig/pull/15402 the retry functionality in deleteTree was fully broken (it was dead code thanks to a typo I made), so it's definitely possible that we could remove the retry logic entirely and still be fine.