File.Move relies on the UnixFileSystem CopyFile method that overwrites only when it shouldn't

[ianhays]

The File.Move(source, dest) and FileInfo.MoveTo(dest) functions do not currently function as they should with regards to overwrites. The following test passes on Windows, but fails on Linux:

FileInfo testFileSource = new FileInfo(GetTestFilePath());
testFileSource.Create().Dispose();
FileInfo testFileDest = new FileInfo(GetTestFilePath());
testFileDest.Create().Dispose();
Assert.Throws<IOException>(() => File.Move(testFileSource.FullName, testFileDest.FullName)); // doesn't throw on Ubuntu

[stephentoub]

The current implementation uses rename, and it appears rename's semantics have it overwrite the target if it already exists. We'll need to implement MoveFile differently.

[ianhays]

Any way to use an overload of rename? I'm not particularly familiar with how the Interop system works, but the linux rename function has a flags overload that prevents overwriting. source

Another option would be to add in some error checking that checks if the destination file already exists before the Rename is called. This would however introduce a race condition e.g. if we check if the file doesn't exist and then someone creates the file and then we call rename then we'll overwrite it because we thought it didn't exist.

One more option would be to remove the rename entirely and just use the CopyFile function like so:

CopyFile(sourceFullPath, destFullPath, overwrite: false);
DeleteFile(sourceFullPath);

if the CopyFile fails then the sourcefile doesn't get deleted, preserving the already existing Move behavior.

[stephentoub]

Any way to use an overload of rename?

Are you referring to renameat2? It's Linux-specific, so we'd still need a solution for OS X.

This would however introduce a race condition

Yeah, I'd really like to avoid that.

One more option would be to remove the rename entirely and just use the CopyFile function like so

Another alternative: what about creating a new name with link and then removing the old name with unlink? (Or maybe we first try the link/unlink approach, and if that fails for some reason, then we fall back to the CopyFile/Delete approach.)

[ianhays]

Are you referring to renameat2? It's Linux-specific, so we'd still need a solution for OS X.

Yes, but you make a good point. Wouldn't get us too far.

Yeah, I'd really like to avoid that.

Same :)

Another alternative: what about creating a new name with link and then removing the old name with unlink?

That would work. I did not see an Interop entry for link(2) in Common, only unlink.

[stephentoub]

I did not see an Interop entry for link(2) in Common, only unlink.

We've been adding imports as we need them; we haven't had a need yet for creating hard links.

[ianhays]

Looks like our changes to the Unix File.Move will be an issue with how FileSystemWatcher reads deletions. A number of the tests use a File.Move call to test that no deletions occurred under the watch, but these tests will now fail since unlink is being called in File.Move. Unless we can get FileSystemWatcher to stop watching our filesystem, unlinking is a no-go.

Of our other options we discussed above, this restriction brings us down to two:

• Use error checking to ensure that the destFile doesn't exist and then make a call to rename. This introduces a concurrency error where a destFile may be overwritten by rename when it shouldn't be.
• Investigate platform-specific solutions to file moving. We already know that the Linux renameat2 would work, so we would need to look into an OSX equivalent.

[ianhays]

Added error checking in the recent PR to get this functioning correctly for the general case. The concurrency issue we discussed above is still present, but at least for now the function works as expected with regards to overwrites.

[stephentoub]

Unless we can get FileSystemWatcher to stop watching our filesystem, unlinking is a no-go.

If we're unable to come up with a solution that accomodates all constraints and we're forced to make an accomodation somewhere, I would prefer it be that some of the events we raise in FileSystemWatcher end up being different.

but at least for now the function works as expected with regards to overwrites

You're only treating this as a temporary band-aid and not as a real solution, though, right? We shouldn't ship a race condition like this that could result in data loss.

[ianhays]

If we're unable to come up with a solution that accomodates all constraints and we're forced to make an accomodation somewhere, I would prefer it be that some of the events we raise in FileSystemWatcher end up being different.

If we can't find an OSX equivalent to renameat2 then we may just have to do this, unfortunately. I'm looking for one now.

You're only treating this as a temporary band-aid and not as a real solution, though, right? We shouldn't ship a race condition like this that could result in data loss.

Yes, absolutely. I wanted to get the PR through since I wasn't sure how long it would take to find a better solution/rig link/unlink to work and there were a large number of changes in the PR that kept growing larger.

[ianhays]

If we can't find an OSX equivalent to renameat2 then we may just have to do this, unfortunately. I'm looking for one now.

No luck for OSX or FreeBSD. OSX has rename() of course and FreeBSD has renameat(), but neither have renameat2() which has the FLAGS parameter we need. I didn't see any other useful syscalls that would do something similar without deletion of the oldpath file. So now we can either:

• Have Linux/Windows FSW treat move not as a delete and OSX/FreeBSD treat it as a deletion
• Have Windows FSW treat move not as a delete, and AnyUnix FSW treat it as a deletion

I personally prefer keeping the Unix implementations with the same behavior. What do you think?

[stephentoub]

I personally prefer keeping the Unix implementations with the same behavior. What do you think?

I'm not sure exactly which option you're suggesting, but we should not modify the Windows behavior from what it has been and is today.

but these tests will now fail since unlink is being called in File.Move

How about this option:

• Try to link
• If we're unsuccesful in linking because the file already exists, fail.
• If we're successful in linking, then the file didn't previously exist but now does, and we can rename over it (no unlink needed).

[ianhays]

I'm not sure exactly which option you're suggesting, but we should not modify the Windows behavior from what it has been and is today.

I meant to say "I personally prefer having each Unix OS have the same implementation even if it is different from the Windows implementation". I prefer the second option.

Try to link
If we're unsuccesful in linking because the file already exists, fail.
If we're successful in linking, then the file didn't previously exist but now does, and we can rename over it (no unlink needed).

This seems fine. We'd be checking if the file exists with link and if it doesn't then we're essentially reserving the name until rename can be called. There will be a space of time in which the destination file exists but doesn't have the correct contents though.