Skip to content

test: Fix windows_spawn tmp directory cleanup #15471

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

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

xEgoist
Copy link
Contributor

@xEgoist xEgoist commented Apr 26, 2023

On Windows, a directory that's set as the current working directory is not allowed to be removed. This can cause error on deleteTree if the CWD is set to the handle to be removed and will result in error.FileBusy. However, due to tmp.cleanup() ignoring the errors, the folder removal error will be ignored. (Possibly Related: #15465)

The only test that I found to be violating this is windows_spawn as it sets CWD to the tmp dir then tries to remove it. I added a test to verify this occurrence.

Another option for solving is to always set the CWD to the parent in both cleanup() functions:

zig/lib/std/testing.zig

Lines 520 to 525 in 7285eed

pub fn cleanup(self: *TmpDir) void {
self.dir.close();
self.parent_dir.deleteTree(&self.sub_path) catch {};
self.parent_dir.close();
self.* = undefined;
}

I hate to take another guess, but this * might be * be the last test causing the random CI timeouts on Windows (#14948) as it was also re-enabled on #14647 . However, I believe when #15467 is merged, it could also mitigate these from happening all together.

On Windows, a directory that's set as the current working directory is
not allowed to be removed. This can cause error on `deleteTree` if the
CWD is set to the file to be removed and will cause `error.FileBusy`.
However, due to `tmp.cleanup()` ignoring the errors, the folder removal error will
be ignored. The only test violating this is `windows_spawn`. As a
solution, setting the parent directory to be the CWD before deletion
will allow the cleanup to pass.
@squeek502
Copy link
Collaborator

This particular case shouldn't have been able to cause the infinite loop in deleteTree (since FileBusy does not trigger a retry), but still good to make sure tmp dirs can be cleaned up.

@xEgoist
Copy link
Contributor Author

xEgoist commented Apr 26, 2023

This particular case shouldn't have been able to cause the infinite loop in deleteTree (since FileBusy does not trigger a retry), but still good to make sure tmp dirs can be cleaned up.

Ah, you are right, it shouldn't trigger that, My bad. Thanks for clarifying that.

@Vexu Vexu enabled auto-merge (rebase) April 26, 2023 14:21
@Vexu Vexu merged commit 1a455b2 into ziglang:master Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants