Skip to content

Mac: Update MoveAndOverwriteFile to use native API#344

Merged
wilbaker merged 1 commit intomicrosoft:masterfrom
wilbaker:mac_atomic_move_overwrite
Oct 4, 2018
Merged

Mac: Update MoveAndOverwriteFile to use native API#344
wilbaker merged 1 commit intomicrosoft:masterfrom
wilbaker:mac_atomic_move_overwrite

Conversation

@wilbaker
Copy link
Member

@wilbaker wilbaker commented Oct 4, 2018

Fixes #342.

The initial MoveAndOverwriteFile could result in a missing file (if File.Delete succeeded and File.Move failed).

Switch to the atomic rename function.

DESCRIPTION
     The rename() system call causes the link named old to be renamed as new.  If new exists, it is first removed.  Both old and new must be of the same type (that is, both must be either direc-
     tories or non-directories) and must reside on the same file system.

     The rename() system call guarantees that an instance of new will always exist, even if the system should crash in the middle of the operation.

@wilbaker wilbaker added domain: user-mode type: test-reliability Issues that contribute to test failures labels Oct 4, 2018
@wilbaker wilbaker self-assigned this Oct 4, 2018
Copy link
Contributor

@sanoursa sanoursa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

if (Rename(sourceFileName, destinationFilename) != 0)
{
File.Delete(destinationFilename);
NativeMethods.ThrowLastWin32Exception($"Failed to renname {sourceFileName} to {destinationFilename}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little weird throwing a Win32Exception from the Mac code but in searching it looks like we are already dependent on that exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep it is weird, but this is what the .NET Core APIs chose to do as well. We just need to translate "Win32Exception" to "NativeException".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep here is an example:

https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs#L44

.NET Core's I/O methods will translate native error codes into a more appropriate exception when possible (e.g. IOException), but in our code we consistently throw Win32Exceptions for all failed native calls.

@wilbaker wilbaker merged commit 9bdca0b into microsoft:master Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: user-mode type: test-reliability Issues that contribute to test failures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants