Skip to content

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

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.

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

Open
squeek502 opened this issue Apr 25, 2023 · 3 comments
Open

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

squeek502 opened this issue Apr 25, 2023 · 3 comments
Labels
bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@squeek502
Copy link
Collaborator

squeek502 commented Apr 25, 2023

Zig Version

0.11.0-dev.2777+b95cdf0ae

Steps to Reproduce and Observed Behavior

This was meant to be a follow-up issue of #13073 (comment)

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 (#15450, #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.

@squeek502 squeek502 added the bug Observed behavior contradicts documented or intended behavior label Apr 25, 2023
@Vexu Vexu added the standard library This issue involves writing Zig code for the standard library. label Apr 25, 2023
@Vexu Vexu added this to the 0.12.0 milestone Apr 25, 2023
squeek502 added a commit to squeek502/zig that referenced this issue Apr 25, 2023
Previously, it was possible for deleteTree to get into a state where it would continually retry deleting the same directory over and over infinitely, since it would think all of the children were successfully deleted but when deleting the directory it would hit `error.DirNotEmpty`. Now, there is a (user-configurable) limit to the number of times it will retry deleting directories, and return `error.TooManyRetries` if the limit is exceeded.

Closes ziglang#15465
squeek502 added a commit to squeek502/zig that referenced this issue Apr 26, 2023
Previously, it was possible for deleteTree to get into a state where it would continually retry deleting the same directory over and over infinitely, since it would think all of the children were successfully deleted but when deleting the directory it would hit `error.DirNotEmpty`. Now, there is a (user-configurable) limit to the number of times it will retry deleting directories, and return `error.TooManyRetries` if the limit is exceeded.

Closes ziglang#15465
@andrewrk
Copy link
Member

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
Copy link
Collaborator Author

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 #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
Copy link
Collaborator Author

squeek502 commented Apr 29, 2023

Oh, also something worth noting: between #13073 and #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants