File.Unix: Replace: increase Windows compatibility

[tmds]

Fixes #49796

• When source and destination are not a file throw UnauthorizedAccessException.
• When source and destination are the same file throw IOException.

@danmoseley @stephentoub ptal

cc @mikernet

[danmoseley]

This would need a breaking change note if we take it...

[danmoseley]

@marklio

[marklio]

Unfortunately, we don't have great ability to look at API usage by OS (we actively discourage those publishing patterns, particularly for managed libraries). All up, File.Replace is a widely-used API. Given its popularity and its age (around since NetFx 2.0), it is a safe bet that most of the code using this expects the semantics provided by Windows implementations. Since consistency is an important principle in .NET, consolidating on that behavior makes sense.

It looks like this change affects more than exceptional scenarios (that is, things that didn't throw on Linux will now throw). If so, are there patterns that could be employed that would allow code to get the same replace semantics as the previous Linux behavior?

I also worry about the non-atomicity of the implemented checks. If these exceptional conditions are reliably atomic on Windows (the result of the underlying Win32 call), we've taken predictable (although inconsistent between OS) behavior and turned it into a potential reliability issue on Linux. I'm not a Linux filesystem expert. What would it take to provide atomicity here?

[danmoseley]

The atomicity only relates to whether you get the right exception. It doesn't affect whether the operation succeeds or fails.

We already have precedent for this:

runtime/src/libraries/System.IO.FileSystem/src/System/IO/FileSystemInfo.Unix.cs

Lines 69 to 75 in 1ee59da

	// Windows distinguishes between whether the directory or the file isn't found,
	// and throws a different exception in these cases. We attempt to approximate that
	// here; there is a race condition here, where something could change between
	// when the error occurs and our checks, but it's the best we can do, and the
	// worst case in such a race condition (which could occur if the file system is
	// being manipulated concurrently with these checks) is that we throw a
	// FileNotFoundException instead of DirectoryNotFoundException.

Unfortunately it's not avoidable I think if we want to throw the same exceptions.

[mikernet]

FYI, the inconsistencies I've found in File.Replace are documented here:

https://github.com/Singulink/Singulink.IO.FileSystem/blob/master/Source/Singulink.IO.FileSystem/IAbsoluteFilePath.Impl.cs#L187

There are also inconsistencies in File.Move:

https://github.com/Singulink/Singulink.IO.FileSystem/blob/master/Source/Singulink.IO.FileSystem/IAbsoluteFilePath.Impl.cs#L187

There are also lots of other issues documented throughout that code file. The biggest one is the nonsense UnauthorizedAccessException messages that could probably be improved in cases where the file path points to a dir even if the exception type is kept the same, though the exception type doesn't really make much sense IMO.

[tmds]

If so, are there patterns that could be employed that would allow code to get the same replace semantics as the previous Linux behavior?

File.Move and Directory.Move can be used.

What would it take to provide atomicity here?

There's no way to check this atomically. The non-atomic checks will expose the cross-platform difference that are now only revealed when running on the other platform.

[tmds]

the inconsistencies I've found
...
There are also lots of other issues

@mikernet if there are some things you'd like, can you create issues for them? That will improve visibility, and there can be some discussion on what makes sense.

[tmds]

@danmoseley @marklio is this PR good to merge?

[danmoseley]

Paging this back in. The only behavior that used to work but now does not, is File.Copy(file, directory) which currently works on Unix and after this will throw, is that a correct statement?

I think the code change is fine.

[tmds]

The only behavior that used to work but now does not, is File.Copy(file, directory) which currently works on Unix and after this will throw, is that a correct statement?

Yes, and copying from a file to the same file will now also throw.

[danmoseley]

@marklio

It looks like this change affects more than exceptional scenarios (that is, things that didn't throw on Linux will now throw). If so, are there patterns that could be employed that would allow code to get the same replace semantics as the previous Linux behavior?

The answer here is yes, on Unix you will need to replace File.Copy(file, directory) with File.Copy(file, directory\file); and (much more minor) remove attempts to do File.Copy(file, samefile). Essentially the question is how do we feel about the first one.

[marklio]

Essentially the question is how do we feel about the first one.

Agreed. It seems like one of our principles has been that carrying cross-plat behavioral differences into the future is untenable. I think we just need to be sure that the "correct" patterns are discoverable for folks moving forward that hit the exceptions. Usually, our tools there are breaking change docs. An article on the right way to author cross-plat file operations would probably be super useful.

[danmoseley]

@adamsitnik @carlossanlop @jozkee the IO crew can one of you please make a call and merge if you're comfortable.

[danmoseley]

@jkotas any objection to

File.Copy(file, directory) which currently works on Unix and after this will throw

[jkotas]

Fine with me.

It seems like one of our principles has been that carrying cross-plat behavioral differences into the future is untenable.

We are trying to do a best effort Windows emulation on Unix here. I am pretty sure that there are still number of subtle differences, in particular around what is done atomically and what are possible error patterns.

Also, Windows emulation on Unix is not free. Notice that this change adds two extra file system syscalls to this APIs.

To write robust performant cross-platform code, one may want to avoid this type of APIs.

[jeffhandley]

@carlossanlop / @jozkee / @adamsitnik -- Please target getting this reviewed so we can make a final decision before Preview 6 snaps.

[jeffhandley]

I resolved the conflicts. The Strings.resx additions needed to be manually moved over to src/libraries/System.Private.CoreLib/src/Resources/Strings.resx due to #53231. The other changes were automatically merged correctly.

[danmoseley]

[ 1171.987738] .NET BGC[17401]: segfault at 7f2d472fbca0 ip 00007f2e3ed7ae0c sp 00007f2d8f7f9f80 error 4 in libcoreclr.so[7f2e3ea2f000+36b000] running numerics tests.

Unfortunately no dump - our scripts may be faulty. Asking..

[jeffhandley]

I've restarted the checks and will check back in on them...

[danmoseley]

Thanks @tmds!

[carlossanlop]

Breaking change doc: dotnet/docs#26323