Fix #106 and #109

DistributedLock.FileSystem/DistributedLock.FileSystem.csproj

@@ -10,8 +10,8 @@
   </PropertyGroup>
 
   <PropertyGroup>
-    <Version>1.0.0</Version>
-    <AssemblyVersion>1.0.0.0</AssemblyVersion>
+    <Version>1.0.1</Version>
+    <AssemblyVersion>1.0.1.0</AssemblyVersion>
     <Authors>Michael Adelson</Authors>
     <Description>Provides a distributed lock implementation based on file locks</Description>
     <Copyright>Copyright © 2020 Michael Adelson</Copyright>

DistributedLock.FileSystem/FileDistributedLock.cs

@@ -13,6 +13,13 @@ namespace Medallion.Threading.FileSystem
     /// </summary>
     public sealed partial class FileDistributedLock : IInternalDistributedLock<FileDistributedLockHandle>
     {
+        /// <summary>
+        /// Since <see cref="UnauthorizedAccessException"/> can be thrown EITHER transiently or for permissions issues, we retry up to this many times
+        /// before we assume that the issue is non-transient. Empirically I've found 50 to work fine so I doubled it just to be safe (if there IS a problem
+        /// there's little risk to trying more times because we'll eventually be failing hard).
+        /// </summary>
+        private const int MaxUnauthorizedAccessExceptionRetries = 100;
+
         // These are not configurable currently because in the future we may want to change the implementation of FileDistributedLock
         // to leverage native methods which may allow for actual blocking. The values here reflect the idea that we expect file locks
         // to be used in cases where contention is rare
@@ -64,15 +71,13 @@ public FileDistributedLock(DirectoryInfo lockFileDirectory, string name)
 
         private FileDistributedLockHandle? TryAcquire(CancellationToken cancellationToken)
         {
+            var retryCount = 0;
+
             while (true)
             {
                 cancellationToken.ThrowIfCancellationRequested();
 
-                try { System.IO.Directory.CreateDirectory(this.Directory); }
-                catch (Exception ex)
-                {
-                    throw new InvalidOperationException($"Failed to ensure that lock file directory {this.Directory} exists", ex);
-                }
+                this.EnsureDirectoryExists();
 
                 FileStream lockFileStream;
                 try
@@ -88,9 +93,36 @@ public FileDistributedLock(DirectoryInfo lockFileDirectory, string name)
                     // this should almost never happen because we just created the directory but in a race condition it could. Just retry
                     continue;
                 }
-                catch (UnauthorizedAccessException) when (System.IO.Directory.Exists(this.Name))
+                catch (UnauthorizedAccessException)
                 {
-                    throw new InvalidOperationException($"Failed to create lock file '{this.Name}' because it is already the name of a directory");
+                    // This can happen in few cases:
+
+                    // The path is already directory, so we'll never be able to open a handle of it as a file
+                    if (System.IO.Directory.Exists(this.Name))
+                    {
+                        throw new InvalidOperationException($"Failed to create lock file '{this.Name}' because it is already the name of a directory");
+                    }
+
+                    // The file exists and is read-only
+                    FileAttributes attributes;
+                    try { attributes = File.GetAttributes(this.Name); }
+                    catch { attributes = FileAttributes.Normal; } // e. g. could fail with FileNotFoundException
+                    if (attributes.HasFlag(FileAttributes.ReadOnly))
+                    {
+                        // We could support this by eschewing DeleteOnClose once we detect that a file is read-only,
+                        // but absent interest or a use-case we'll just throw for now
+                        throw new NotSupportedException($"Locking on read-only file '{this.Name}' is not supported");
+                    }
+
+                    // Frustratingly, this error can be thrown transiently due to concurrent creation/deletion. Initially assume
+                    // that it is transient and just retry
+                    if (++retryCount <= MaxUnauthorizedAccessExceptionRetries)
+                    {
+                        continue;
+                    }
+
+                    // If we get here, we've exhausted our retries: assume that it is a legitimate permissions issue
+                    throw;
                 }
                 // this should never happen because we validate. However if it does (e. g. due to some system configuration change?), throw so that
                 // this doesn't end up in the IOException block (PathTooLongException is IOException)
@@ -104,5 +136,26 @@ public FileDistributedLock(DirectoryInfo lockFileDirectory, string name)
                 return new FileDistributedLockHandle(lockFileStream);
             }
         }
+
+        private void EnsureDirectoryExists()
+        {
+            var retryCount = 0;
+
+            while (true)
+            {
+                try
+                {
+                    System.IO.Directory.CreateDirectory(this.Directory);
+                    return;
+                }
+                // This can indicate either a transient failure during concurrent creation/deletion or a permissions issue.
+                // If we encounter it, assume it is transient unless it persists.
+                catch (UnauthorizedAccessException) when (++retryCount <= MaxUnauthorizedAccessExceptionRetries) { }
+                catch (Exception ex)
+                {
+                    throw new InvalidOperationException($"Failed to ensure that lock file directory {this.Directory} exists", ex);
+                }
+            }
+        }
     }
 }

DistributedLock.Tests/Tests/FileSystem/FileDistributedLockTest.cs

@@ -7,6 +7,7 @@
 using System.Runtime.InteropServices;
 using System.Security.Cryptography;
 using System.Text;
+using System.Threading;
 using System.Threading.Tasks;
 
 namespace Medallion.Threading.Tests.FileSystem
@@ -365,6 +366,122 @@ public void TestBase32Hashing()
             }
         }
 
+        /// <summary>
+        /// Reproduces https://github.com/madelson/DistributedLock/issues/109
+        /// 
+        /// Basically, there is a small window where concurrent file creation/deletion throws
+        /// <see cref="UnauthorizedAccessException"/> despite there being no access permission errors.
+        /// See also https://github.com/dotnet/runtime/issues/61395.
+        /// 
+        /// This test shows that we are not vulnerable to this.
+        /// </summary>
+        [Test]
+        public void TestDoesNotFailDueToUnauthorizedAccessExceptionOnFileCreation()
+        {
+            Directory.CreateDirectory(LockFileDirectory);
+            var @lock = new FileDistributedLock(LockFileDirectoryInfo, Guid.NewGuid().ToString());
+
+            const int TaskCount = 20;
+
+            using var barrier = new Barrier(TaskCount);
+
+            var tasks = Enumerable.Range(0, TaskCount)
+                .Select(_ => Task.Factory.StartNew(() =>
+                {
+                    barrier.SignalAndWait();
+
+                    for (var i = 0; i < 500; ++i)
+                    {
+                        @lock.TryAcquire()?.Dispose();
+                    }
+                }, TaskCreationOptions.LongRunning))
+                .ToArray();
+
+            Assert.DoesNotThrowAsync(() => Task.WhenAll(tasks));
+        }
+
+        /// <summary>
+        /// Reproduces https://github.com/madelson/DistributedLock/issues/106
+        /// 
+        /// Basically, there is a small window where concurrent creation/deletion of directories
+        /// throws <see cref="UnauthorizedAccessException"/> even though there are no access permission errors.
+        /// 
+        /// This test confirms that we recover from such errors.
+        /// </summary>
+        [Test]
+        public void TestDoesNotFailDueToUnauthorizedAccessExceptionOnDirectoryCreation()
+        {
+            var @lock = new FileDistributedLock(LockFileDirectoryInfo, Guid.NewGuid().ToString());
+
+            const int TaskCount = 20;
+
+            using var barrier = new Barrier(TaskCount);
+            using var cancelationTokenSource = new CancellationTokenSource();
+
+            var tasks = Enumerable.Range(0, TaskCount)
+                .Select(task => Task.Factory.StartNew(() =>
+                {
+                    for (var i = 0; i < 1000; ++i)
+                    {
+                        // line up all the threads
+                        try { barrier.SignalAndWait(cancelationTokenSource.Token); }
+                        catch when (cancelationTokenSource.Token.IsCancellationRequested) { return; }
+
+                        // have one thread clear the directory
+                        if (task == 0 && Directory.Exists(LockFileDirectory)) { Directory.Delete(LockFileDirectory, recursive: true); }
+
+                        // line up all the threads
+                        if (!barrier.SignalAndWait(TimeSpan.FromSeconds(3))) { throw new TimeoutException("should never get here"); }
+
+                        // have half the threads just create and delete the directory, catching any errors
+                        if (task % 2 == 0)
+                        {
+                            try
+                            {
+                                Directory.CreateDirectory(LockFileDirectory);
+                                Directory.Delete(LockFileDirectory);
+                            }
+                            catch { }
+                        }
+                        // the other half will attempt to acquire the lock
+                        else
+                        {
+                            try { @lock.TryAcquire()?.Dispose(); }
+                            catch
+                            {
+                                cancelationTokenSource.Cancel(); // exception found: exit
+                                throw;
+                            }
+                        }
+                    }
+                }, TaskCreationOptions.LongRunning))
+                .ToArray();
+
+            Assert.DoesNotThrowAsync(() => Task.WhenAll(tasks));
+        }
+
+        /// <summary>
+        /// Documents a limitation we've imposed for now to keep the code simpler
+        /// </summary>
+        [Test]
+        public void TestLockingReadOnlyFileIsNotSupported()
+        {
+            Directory.CreateDirectory(LockFileDirectory);
+            var @lock = new FileDistributedLock(LockFileDirectoryInfo, Guid.NewGuid().ToString());
+            File.Create(@lock.Name).Dispose();
+
+            try
+            {
+                File.SetAttributes(@lock.Name, FileAttributes.ReadOnly);
+
+                Assert.Throws<NotSupportedException>(() => @lock.TryAcquire()?.Dispose());
+            }
+            finally
+            {
+                File.SetAttributes(@lock.Name, FileAttributes.Normal);
+            }
+        }
+
         private static void AssertCanUseName(string name, DirectoryInfo? directory = null)
         {
             var @lock = new FileDistributedLock(directory ?? LockFileDirectoryInfo, name);

DistributedLock.Tests/Tests/FileSystem/FileDistributedLockWindowsTest.cs

@@ -0,0 +1,37 @@
+using Medallion.Threading.FileSystem;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Text;
+
+namespace Medallion.Threading.Tests.FileSystem
+{
+    [Category("CIWindows")]
+    public class FileDistributedLockWindowsTest
+    {
+        /// <summary>
+        /// Example of where always ignoring <see cref="UnauthorizedAccessException"/> during file creation
+        /// would be problematic.
+        /// </summary>
+        [Test]
+        public void TestThrowsUnauthorizedAccessExceptionInCaseOfFilePermissionViolation()
+        {
+            var @lock = new FileDistributedLock(new DirectoryInfo(@"C:\Windows"), Guid.NewGuid().ToString());
+            Assert.Throws<UnauthorizedAccessException>(() => @lock.TryAcquire()?.Dispose());
+        }
+
+        /// <summary>
+        /// Example of where always ignoring <see cref="UnauthorizedAccessException"/> during directory creation
+        /// would be problematic.
+        /// </summary>
+        [Test]
+        public void TestThrowsUnauthorizedAccessExceptionInCaseOfDirectoryPermissionViolation()
+        {
+            var @lock = new FileDistributedLock(new DirectoryInfo(@"C:\Windows\MedallionDistributedLock"), Guid.NewGuid().ToString());
+            var exception = Assert.Throws<InvalidOperationException>(() => @lock.TryAcquire()?.Dispose());
+            Assert.IsInstanceOf<UnauthorizedAccessException>(exception.InnerException);
+            Assert.IsFalse(Directory.Exists(Path.GetDirectoryName(@lock.Name)));
+        }
+    }
+}

README.md

@@ -134,6 +134,8 @@ public class SomeService
 Contributions are welcome! If you are interested in contributing towards a new or existing issue, please let me know via comments on the issue so that I can help you get started and avoid wasted effort on your part.
 
 ## Release notes
+- 2.3.0
+	- Made file-based locking more robust to transient `UnauthorizedAccessException`s ([#106](https://github.com/madelson/DistributedLock/issues/106) & [#109](https://github.com/madelson/DistributedLock/issues/109), DistributedLock.FileSystem 1.0.1)
 - 2.2.0
 	- Added MySQL/MariaDB-based implementation ([#95](https://github.com/madelson/DistributedLock/issues/95), DistributedLock.MySql 1.0.0)
 - 2.1.0