-
Notifications
You must be signed in to change notification settings - Fork 202
FoundationEssentials: try to make the file atomic behaviour more robust #1484
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any test cases we could add alongside this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add a unit test for this? Even if the unit test can't reliably cause this issue 100% of the time, the example that @bnbarham mentioned in swiftlang/swift#83606 (comment) might be good to add as a test (but rewritten to use Swift concurrency instead of dispatch)
24a6f51
to
048d4a6
Compare
How do we want to integrate the test case that @bnbarham and I were using? |
How about something like the following in
|
We have observed `ERROR_ACCESS_DENIED` in CI on `SetRenameInformationFile`. Try to make the path more robust by first performing a kernel based rename with POSIX semantics. This requires Windows 10 1809+. If that is unsuccessful, verify that attributes are not to blame. A failure may still occur if the file is on a different volume. In such a case, fallback to the `MoveFileExW` operation to perform a copy + delete operation. Hopefully this should make the implementation more robust to failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, thanks! @bnbarham mentioned wanted to get this into release/6.2
, is that still the desire? If so, right now we're currently automerging forward from release/6.2
to main
so I think we'd want to re-target this PR to release/6.2
if that's where we want to land this (@parkera is that still our preferred process?)
I'd actually like to get this into release/6.2.0 if we think it's safe enough for the initial 6.2 release. But definitely release/6.2 if not. The issue this fixes in swift-driver is actually very likely to happen, assuming that any command in a build ends up needing response files. |
Let me cherry-pick this up to the release/6.2 branch. |
#1485 is the 6.2 cherry-pick; I don't have a strong opinion on which way we handle this (merge this and merge the other one or merge the other one and let the auto-merger handle the rest). |
@swift-ci please test |
We have observed
ERROR_ACCESS_DENIED
in CI onSetRenameInformationFile
. Try to make the path more robust by first performing a kernel based rename with POSIX semantics. This requires Windows 10 1809+. If that is unsuccessful, verify that attributes are not to blame.A failure may still occur if the file is on a different volume. In such a case, fallback to the
MoveFileExW
operation to perform a copy + delete operation.Hopefully this should make the implementation more robust to failures.