FileStream.Unix: improve DeleteOnClose when FileShare.None.

[tmds]

Use FileShare.None lock to detect when another Handle that has
DeleteOnClose deleted the file, and then re-open it.

@adamsitnik @stephentoub ptal.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Use FileShare.None lock to detect when another Handle that has
DeleteOnClose deleted the file, and then re-open it.

@adamsitnik @stephentoub ptal.

Author:	tmds
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[tmds]

On Linux, NuGet doesn't clean up its lock files because DeleteOnClose has concurrency issues (NuGet/NuGet.Client#431).

I was looking introducing some specific cleanup logic in NuGet/NuGet.Client#4123.
This PR should fix it in the framework instead and allow DeleteOnClose to be used for cleanup of NuGet lock files.

cc @zivkan

[stephentoub]

On Linux, NuGet doesn't clean up its lock files because DeleteOnClose has concurrency issues

What are the issues?

EDIT: nevermind, I see your description in the nuget pr.

[marcin-krystianc]

FWIW, I think it makes a lot of sense to fix it in the runtime. The intent of:

using (new FileStream(path, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None, 4096, FileOptions.DeleteOnClose))
{
}

is very clear and there is no doubt what it wants to achieve. So it is quite dangerous that currently this works on Windows but it is broken on Linux.

[tmds]

The test I've added is failing. I need to take a closer look.

[zivkan]

FWIW, the nuget.org server team coincidentally contacted me asking if it's possible for client to make duplicate HTTP requests to the same URL. This got me thinking about this PR.

Is it possible that two processes/threads opening a file with FileShare.None can BOTH successfully open the file? The CDN logs show it's happening on Linux.

edit: 90%+ of instances are on Linux, but it's also happening on Windows. NuGet guards simultaneous downloads of the same package via a lock file, hence why I thought of this PR.

[tmds]

CI failures seem unrelated now.

Is it possible that two processes/threads opening a file with FileShare.None can BOTH successfully open the file? The CDN logs show it's happening on Linux.

No. One will fail the lock and throw:

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

Lines 285 to 293 in 145e588

	// The only error we care about is EWOULDBLOCK, which indicates that the file is currently locked by someone
	// else and we would block trying to access it. Other errors, such as ENOTSUP (locking isn't supported) or
	// EACCES (the file system doesn't allow us to lock), will only hamper FileStream's usage without providing value,
	// given again that this is only advisory / best-effort.
	Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo();
	if (errorInfo.Error == Interop.Error.EWOULDBLOCK)
	{
	throw Interop.GetExceptionForIoErrno(errorInfo, path, isDirectory: false);
	}

[jeffhandley]

@adamsitnik This PR is assigned to you for follow-up/decision before the RC1 snap.

[adamsitnik]

I like the idea of calling unlink before unlocking the file. We could also let close() unlock it for us and call flock(LOCK_UN) only when close() fails.

But with the proposed solution I am not sure if we are able to guarantee that FileShare.None combined with DeleteOnClose can be used as a safe global lock.

In case of NuGet, they should rather use FileMode.CreateNew instead of FileMode.OpenOrCreate

https://github.com/NuGet/NuGet.Client/blob/0ae80f5495c8d6fad2ec6e2c708727675ff08fb6/src/NuGet.Core/NuGet.Common/ConcurrencyUtilities.cs#L217

it's mapped to O_EXCL:

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

Lines 232 to 233 in 7249ec4

	case FileMode.CreateNew:
	flags |= (Interop.Sys.OpenFlags.O_CREAT | Interop.Sys.OpenFlags.O_EXCL);

From https://man7.org/linux/man-pages/man2/open.2.html:

Ensure that this call creates the file: if this flag is
specified in conjunction with O_CREAT, and pathname
already exists, then open() fails with the error EEXIST.

Which combined with DeleteOnClose should serve as pretty good global lock. Please correct me if I am wrong.

[tmds]

Which combined with DeleteOnClose should serve as pretty good global lock.

The trouble is when the file already exists. There is no way to open it and be sure you and the other process are referring to the same file entry.

The change does this by performing Stat and comparing it with FStat after obtaining the lock.

[tmds]

@adamsitnik can you take another look?

[tmds]

ping @adamsitnik

[carlossanlop]

Thank you for the patience, @tmds . I left a few comments.