Skip to content
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

std.windows: use atomic rename, if possible #16717

Merged
merged 4 commits into from
Aug 24, 2023

Conversation

matu3ba
Copy link
Contributor

@matu3ba matu3ba commented Aug 6, 2023

Mitigates #14978.

@squeek502
Copy link
Collaborator

This should use the same strategy as #16499, since the same caveat (presumably) applies for non-NTFS filesystems.

lib/std/os/windows.zig Outdated Show resolved Hide resolved
@matu3ba matu3ba marked this pull request as ready for review August 19, 2023 12:49
@matu3ba
Copy link
Contributor Author

matu3ba commented Aug 19, 2023

CI passes now. Do you think further tests are needed @squeek502 ?

@squeek502
Copy link
Collaborator

Would be good to test to confirm that this will actually mitigate #14978 (see the reproduction in #14978 (comment))

@matu3ba matu3ba marked this pull request as draft August 19, 2023 23:56
@squeek502
Copy link
Collaborator

I can confirm that the fallback works--the fs tests pass when running on a FAT32 drive (and I confirmed that INVALID_PARAMETER is being hit, so it's doing the proper fallback).

lib/std/os.zig Outdated Show resolved Hide resolved
@squeek502
Copy link
Collaborator

squeek502 commented Aug 20, 2023

I can also confirm that this successfully avoids AccessDenied for the reproduction in #14978 (comment)

> 14978-master.exe
error: AccessDenied
C:\Users\Ryan\Programming\Zig\zig\lib\std\os.zig:2664:27: 0x7ff764ae6e6a in renameatW (14978-master.exe.obj)
        .ACCESS_DENIED => return error.AccessDenied,
                          ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\os.zig:2531:9: 0x7ff764ab76c7 in renameat (14978-master.exe.obj)
        return renameatW(old_dir_fd, old_path_w.span(), new_dir_fd, new_path_w.span(), windows.TRUE);
        ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\fs.zig:232:9: 0x7ff764ab1927 in finish (14978-master.exe.obj)
        try os.renameat(self.dir.fd, self.tmp_path_buf[0..], self.dir.fd, self.dest_basename);
        ^
C:\Users\Ryan\Programming\Zig\tmp\14978.zig:5:5: 0x7ff764ab1814 in main (14978-master.exe.obj)
    try af.finish();
    ^
> 14978-posix.exe

(there is a second process of the same .exe spawned at the same time for each of the above; the -master version hits AccessDenied every time, the -posix version [this branch] never hits AccessDenied)


Once #16717 (comment) is addressed, I think this will be good to go.

@matu3ba matu3ba marked this pull request as ready for review August 20, 2023 13:06
@matu3ba matu3ba marked this pull request as draft August 21, 2023 07:04
@matu3ba matu3ba marked this pull request as ready for review August 21, 2023 14:30
Jan Philipp Hafer added 4 commits August 21, 2023 20:43
@andrewrk andrewrk merged commit 7a834e2 into ziglang:master Aug 24, 2023
@matu3ba matu3ba deleted the win_atomic_mv2 branch August 25, 2023 20:46
squeek502 added a commit to squeek502/zig that referenced this pull request Jan 21, 2024
Most of these tests were made to pass by ziglang#16717, but there were a few loose ends to tie up with regards to the 'fallback' behavior (when FILE_RENAME_POSIX_SEMANTICS either isn't available or isn't supported by the underlying filesystem).

We now do a bit of extra work to get POSIX-like renaming in the fallback path.

Closes ziglang#6364
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