From 25d4999c5ea8dc551c9505056dba2f57c856c1fa Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 5 Oct 2021 21:45:21 +0200 Subject: [PATCH] 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> --- .../tests/FileStream/DeleteOnClose.cs | 81 +++++++++ .../tests/System.IO.FileSystem.Tests.csproj | 1 + .../Win32/SafeHandles/SafeFileHandle.Unix.cs | 154 ++++++++++++------ 3 files changed, 190 insertions(+), 46 deletions(-) create mode 100644 src/libraries/System.IO.FileSystem/tests/FileStream/DeleteOnClose.cs diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/DeleteOnClose.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/DeleteOnClose.cs new file mode 100644 index 0000000000000..72150266a2ce0 --- /dev/null +++ b/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 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"); + } + } +} diff --git a/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj b/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj index ae57e79319a5f..1d523e70a1c8d 100644 --- a/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj +++ b/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj @@ -124,6 +124,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index 1edb65fe58afb..9b38d148b2693 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/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);