FileStream.Unix: improve DeleteOnClose when FileShare.None. (#55327)

* FileStream.Unix: improve DeleteOnClose when FileShare.None.

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

* Rephrase comment

* FileStream_DeleteOnClose: handle a few special cases.

* Try avoiding test timeout on arm

* Limit check to filemodes that open an existing file, or create it when it doesn't exist.

* PR feedback

* Limit functional change to FileMode.OpenOrCreate, and increase test timeout.

* Fix comment.

Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

src/libraries/System.IO.FileSystem/tests/FileStream/DeleteOnClose.cs

@@ -0,0 +1,81 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Threading;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace System.IO.Tests
+{
+    public class FileStream_DeleteOnClose : FileSystemTest
+    {
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsFileLockingEnabled), nameof(PlatformDetection.IsThreadingSupported))]
+        public async Task OpenOrCreate_DeleteOnClose_UsableAsMutex()
+        {
+            var cts = new CancellationTokenSource();
+            int enterCount = 0;
+            int locksRemaining = int.MaxValue;
+            bool exclusive = true;
+
+            string path = GetTestFilePath();
+            Assert.False(File.Exists(path));
+
+            Func<Task> lockFile = async () =>
+            {
+                while (!cts.IsCancellationRequested)
+                {
+                    try
+                    {
+                        using (var fs = new FileStream(path, FileMode.OpenOrCreate, FileAccess.Write, FileShare.None, 4096, FileOptions.DeleteOnClose))
+                        {
+                            int counter = Interlocked.Increment(ref enterCount);
+                            if (counter != 1)
+                            {
+                                // Test failed.
+                                exclusive = false;
+                                cts.Cancel(); // Stop other Tasks.
+                                return;
+                            }
+
+                            // Hold the lock for a little bit.
+                            await Task.Delay(TimeSpan.FromMilliseconds(5));
+
+                            Interlocked.Decrement(ref enterCount);
+
+                            if (Interlocked.Decrement(ref locksRemaining) <= 0)
+                            {
+                                return;
+                            }
+                        }
+                        await Task.Delay(TimeSpan.FromMilliseconds(1));
+                    }
+                    catch (UnauthorizedAccessException)
+                    {
+                        // This can occur when the file is being deleted on Windows.
+                        await Task.Delay(TimeSpan.FromMilliseconds(1));
+                    }
+                    catch (IOException)
+                    {
+                        // The file is opened by another Task.
+                        await Task.Delay(TimeSpan.FromMilliseconds(1));
+                    }
+                }
+            };
+
+            var tasks = new Task[50];
+            for (int i = 0; i < tasks.Length; i++)
+            {
+                tasks[i] = Task.Run(lockFile);
+            }
+
+            // Wait for 1000 locks.
+            cts.CancelAfter(TimeSpan.FromSeconds(60)); // Test timeout.
+            Volatile.Write(ref locksRemaining, 500);
+            await Task.WhenAll(tasks);
+
+            Assert.True(exclusive, "Exclusive");
+            Assert.False(cts.IsCancellationRequested, "Test cancelled");
+            Assert.False(File.Exists(path), "File exists");
+        }
+    }
+}

src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj

@@ -124,6 +124,7 @@
     <Compile Include="FileStream\IsAsync.cs" />
     <Compile Include="FileStream\Name.cs" />
     <Compile Include="FileStream\CopyToAsync.cs" />
+    <Compile Include="FileStream\DeleteOnClose.cs" />
     <Compile Include="FileStream\FileStreamOptions.cs" />
     <Compile Include="FileStream\Flush.cs" />
     <Compile Include="FileStream\Dispose.cs" />

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

@@ -71,35 +71,6 @@ private static SafeFileHandle Open(string path, Interop.Sys.OpenFlags flags, int
                     errorRewriter: e => (e.Error == Interop.Error.EISDIR) ? Interop.Error.EACCES.Info() : e);
             }
 
-            // Make sure it's not a directory; we do this after opening it once we have a file descriptor
-            // to avoid race conditions.
-            //
-            // We can omit the check when write access is requested. open will have failed with EISDIR.
-            if ((flags & (Interop.Sys.OpenFlags.O_WRONLY | Interop.Sys.OpenFlags.O_RDWR)) == 0)
-            {
-                Interop.Sys.FileStatus status;
-                if (Interop.Sys.FStat(handle, out status) != 0)
-                {
-                    Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo();
-                    handle.Dispose();
-                    throw Interop.GetExceptionForIoErrno(error, path);
-                }
-                if ((status.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR)
-                {
-                    handle.Dispose();
-                    throw Interop.GetExceptionForIoErrno(Interop.Error.EACCES.Info(), path, isDirectory: true);
-                }
-
-                if ((status.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFREG)
-                {
-                    // we take advantage of the information provided by the fstat syscall
-                    // and for regular files (most common case)
-                    // avoid one extra sys call for determining whether file can be seeked
-                    handle._canSeek = NullableBool.True;
-                    Debug.Assert(Interop.Sys.LSeek(handle, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0);
-                }
-            }
-
             return handle;
         }
 
@@ -121,19 +92,9 @@ private static bool DirectoryExists(string fullPath)
 
         protected override bool ReleaseHandle()
         {
-            // When the SafeFileHandle was opened, we likely issued an flock on the created descriptor in order to add
-            // an advisory lock.  This lock should be removed via closing the file descriptor, but close can be
-            // interrupted, and we don't retry closes.  As such, we could end up leaving the file locked,
-            // which could prevent subsequent usage of the file until this process dies.  To avoid that, we proactively
-            // try to release the lock before we close the handle.
-            if (_isLocked)
-            {
-                Interop.Sys.FLock(handle, Interop.Sys.LockOperations.LOCK_UN); // ignore any errors
-                _isLocked = false;
-            }
-
             // If DeleteOnClose was requested when constructed, delete the file now.
             // (Unix doesn't directly support DeleteOnClose, so we mimic it here.)
+            // We delete the file before releasing the lock to detect the removal in Init.
             if (_deleteOnClose)
             {
                 // Since we still have the file open, this will end up deleting
@@ -143,6 +104,17 @@ protected override bool ReleaseHandle()
                 Interop.Sys.Unlink(_path); // ignore errors; it's valid that the path may no longer exist
             }
 
+            // When the SafeFileHandle was opened, we likely issued an flock on the created descriptor in order to add
+            // an advisory lock.  This lock should be removed via closing the file descriptor, but close can be
+            // interrupted, and we don't retry closes.  As such, we could end up leaving the file locked,
+            // which could prevent subsequent usage of the file until this process dies.  To avoid that, we proactively
+            // try to release the lock before we close the handle.
+            if (_isLocked)
+            {
+                Interop.Sys.FLock(handle, Interop.Sys.LockOperations.LOCK_UN); // ignore any errors
+                _isLocked = false;
+            }
+
             // Close the descriptor. Although close is documented to potentially fail with EINTR, we never want
             // to retry, as the descriptor could actually have been closed, been subsequently reassigned, and
             // be in use elsewhere in the process.  Instead, we simply check whether the call was successful.
@@ -177,16 +149,28 @@ internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess a
                 Interop.Sys.Permissions.S_IRGRP | Interop.Sys.Permissions.S_IWGRP |
                 Interop.Sys.Permissions.S_IROTH | Interop.Sys.Permissions.S_IWOTH;
 
-            SafeFileHandle safeFileHandle = Open(fullPath, openFlags, (int)OpenPermissions);
+            SafeFileHandle? safeFileHandle = null;
             try
             {
-                safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize);
+                while (true)
+                {
+                    safeFileHandle = Open(fullPath, openFlags, (int)OpenPermissions);
 
-                return safeFileHandle;
+                    // When Init return false, the path has changed to another file entry, and
+                    // we need to re-open the path to reflect that.
+                    if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize))
+                    {
+                        return safeFileHandle;
+                    }
+                    else
+                    {
+                        safeFileHandle.Dispose();
+                    }
+                }
             }
             catch (Exception)
             {
-                safeFileHandle.Dispose();
+                safeFileHandle?.Dispose();
 
                 throw;
             }
@@ -271,10 +255,34 @@ private static Interop.Sys.OpenFlags PreOpenConfigurationFromOptions(FileMode mo
             return flags;
         }
 
-        private void Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize)
+        private bool Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize)
         {
+            Interop.Sys.FileStatus status = default;
+            bool statusHasValue = false;
+
+            // Make sure our handle is not a directory.
+            // We can omit the check when write access is requested. open will have failed with EISDIR.
+            if ((access & FileAccess.Write) == 0)
+            {
+                // Stat the file descriptor to avoid race conditions.
+                FStatCheckIO(this, path, ref status, ref statusHasValue);
+
+                if ((status.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFDIR)
+                {
+                    throw Interop.GetExceptionForIoErrno(Interop.Error.EACCES.Info(), path, isDirectory: true);
+                }
+
+                if ((status.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFREG)
+                {
+                    // we take advantage of the information provided by the fstat syscall
+                    // and for regular files (most common case)
+                    // avoid one extra sys call for determining whether file can be seeked
+                    _canSeek = NullableBool.True;
+                    Debug.Assert(Interop.Sys.LSeek(this, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0);
+                }
+            }
+
             IsAsync = (options & FileOptions.Asynchronous) != 0;
-            _deleteOnClose = (options & FileOptions.DeleteOnClose) != 0;
 
             // Lock the file if requested via FileShare.  This is only advisory locking. FileShare.None implies an exclusive
             // lock on the file and all other modes use a shared lock.  While this is not as granular as Windows, not mandatory,
@@ -293,6 +301,44 @@ private void Init(string path, FileMode mode, FileAccess access, FileShare share
                 }
             }
 
+            // On Windows, DeleteOnClose happens when all kernel handles to the file are closed.
+            // Unix kernels don't have this feature, and .NET deletes the file when the Handle gets disposed.
+            // When the file is opened with an exclusive lock, we can use it to check the file at the path
+            // still matches the file we've opened.
+            // When the delete is performed by another .NET Handle, it holds the lock during the delete.
+            // Since we've just obtained the lock, the file will already be removed/replaced.
+            // We limit performing this check to cases where our file was opened with DeleteOnClose with
+            // a mode of OpenOrCreate.
+            if (_isLocked && ((options & FileOptions.DeleteOnClose) != 0) &&
+                share == FileShare.None && mode == FileMode.OpenOrCreate)
+            {
+                FStatCheckIO(this, path, ref status, ref statusHasValue);
+
+                Interop.Sys.FileStatus pathStatus;
+                if (Interop.Sys.Stat(path, out pathStatus) < 0)
+                {
+                    // If the file was removed, re-open.
+                    // Otherwise throw the error 'stat' gave us (assuming this is the
+                    // error 'open' will give us if we'd call it now).
+                    Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo();
+
+                    if (error.Error == Interop.Error.ENOENT)
+                    {
+                        return false;
+                    }
+
+                    throw Interop.GetExceptionForIoErrno(error, path);
+                }
+                if (pathStatus.Ino != status.Ino || pathStatus.Dev != status.Dev)
+                {
+                    // The file was replaced, re-open
+                    return false;
+                }
+            }
+            // Enable DeleteOnClose when we've succesfully locked the file.
+            // On Windows, the locking happens atomically as part of opening the file.
+            _deleteOnClose = (options & FileOptions.DeleteOnClose) != 0;
+
             // These provide hints around how the file will be accessed.  Specifying both RandomAccess
             // and Sequential together doesn't make sense as they are two competing options on the same spectrum,
             // so if both are specified, we prefer RandomAccess (behavior on Windows is unspecified if both are provided).
@@ -343,6 +389,8 @@ private void Init(string path, FileMode mode, FileAccess access, FileShare share
                                             path, preallocationSize));
                 }
             }
+
+            return true;
         }
 
         private bool CanLockTheFile(Interop.Sys.LockOperations lockOperation, FileAccess access)
@@ -379,6 +427,20 @@ private bool CanLockTheFile(Interop.Sys.LockOperations lockOperation, FileAccess
             }
         }
 
+        private void FStatCheckIO(SafeFileHandle handle, string path, ref Interop.Sys.FileStatus status, ref bool statusHasValue)
+        {
+            if (!statusHasValue)
+            {
+                if (Interop.Sys.FStat(this, out status) != 0)
+                {
+                    Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo();
+                    throw Interop.GetExceptionForIoErrno(error, path);
+                }
+
+                statusHasValue = true;
+            }
+        }
+
         private bool GetCanSeek()
         {
             Debug.Assert(!IsClosed);