[release/6.0] SafeFileHandle.Unix: don't DeleteOnClose when lock is not successful. (#60112)

* SafeFileHandle.Unix: don't DeleteOnClose when lock is not succesful.

In #55153 DeleteOnClose handling
moved from FileStream to SafeFileHandle.

This unintentionally caused DeleteOnClose to be applied even when
FileShare locking fails. As on Windows, DeleteOnClose should not take
effect when sharing prevents the file from being opened.

This also swaps the order of unlink and LOCK_UN in Dispose as it was
prior to #55153.
Either order should be fine.

* test: add ConditionalFact IsFileLockingEnabled

Author: tmds

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

@@ -78,6 +78,18 @@ public async Task ThrowWhenHandlePositionIsChanged_async()
             await ThrowWhenHandlePositionIsChanged(useAsync: true);
         }
 
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsFileLockingEnabled))]
+        public void DeleteOnClose_FailedShareDoesNotDeleteFile()
+        {
+            string fileName = GetTestFilePath();
+
+            using SafeFileHandle handle = File.OpenHandle(fileName, FileMode.OpenOrCreate, FileAccess.Write, FileShare.None);
+
+            Assert.Throws<IOException>(() => File.OpenHandle(fileName, FileMode.Open, FileAccess.Write, FileShare.None, FileOptions.DeleteOnClose));
+
+            Assert.True(File.Exists(fileName));
+        }
+
         private async Task ThrowWhenHandlePositionIsChanged(bool useAsync)
         {
             string fileName = GetTestFilePath();

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

@@ -121,17 +121,6 @@ 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.)
             if (_deleteOnClose)
@@ -143,6 +132,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.
@@ -274,7 +274,6 @@ private static Interop.Sys.OpenFlags PreOpenConfigurationFromOptions(FileMode mo
         private void Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize)
         {
             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 +292,10 @@ private void Init(string path, FileMode mode, FileAccess access, FileShare share
                 }
             }
 
+            // 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).