Removing the finalizer and implementing thread safety

--HG--
branch : indexing

src/Orchard.Tests/FileSystems/LockFile/LockFileManagerTests.cs

@@ -1,5 +1,8 @@
-using System.IO;
+using System;
+using System.Collections.Generic;
+using System.IO;
 using System.Linq;
+using System.Threading;
 using NUnit.Framework;
 using Orchard.FileSystems.AppData;
 using Orchard.FileSystems.LockFile;
@@ -91,6 +94,20 @@ namespace Orchard.Tests.FileSystems.LockFile {
             Assert.That(_appDataFolder.ListFiles("").Count(), Is.EqualTo(0));
         }
 
+        [Test]
+        public void DisposingLockShouldReleaseIt() {
+            ILockFile lockFile = null;
+            _lockFileManager.TryAcquireLock("foo.txt.lock", ref lockFile);
+
+            using (lockFile) {
+                Assert.That(_lockFileManager.IsLocked("foo.txt.lock"), Is.True);
+                Assert.That(_appDataFolder.ListFiles("").Count(), Is.EqualTo(1));
+            }
+
+            Assert.That(_lockFileManager.IsLocked("foo.txt.lock"), Is.False);
+            Assert.That(_appDataFolder.ListFiles("").Count(), Is.EqualTo(0));
+        }
+
         [Test]
         public void ExpiredLockShouldBeAvailable() {
             ILockFile lockFile = null;
@@ -112,5 +129,95 @@ namespace Orchard.Tests.FileSystems.LockFile {
             Assert.That(granted, Is.True);
             Assert.That(_appDataFolder.ListFiles("").Count(), Is.EqualTo(1));
         }
+
+        private static int _lockCount;
+        private static readonly object _synLock = new object();
+
+        [Test]
+        public void AcquiringLockShouldBeThreadSafe() {
+            var threads = new List<Thread>();
+            for(var i=0; i<10; i++) {
+                var t = new Thread(PlayWithAcquire);
+                t.Start();
+                threads.Add(t);
+            }
+
+            threads.ForEach(t => t.Join());
+            Assert.That(_lockCount, Is.EqualTo(0));
+        }
+
+        [Test]
+        public void IsLockedShouldBeThreadSafe() {
+            var threads = new List<Thread>();
+            for (var i = 0; i < 10; i++)
+            {
+                var t = new Thread(PlayWithIsLocked);
+                t.Start();
+                threads.Add(t);
+            }
+
+            threads.ForEach(t => t.Join());
+            Assert.That(_lockCount, Is.EqualTo(0));
+        }
+
+        private void PlayWithAcquire() {
+            var r = new Random(DateTime.Now.Millisecond); 
+            ILockFile lockFile = null;
+
+            // loop until the lock has been acquired
+            for (;;) {
+                if (!_lockFileManager.TryAcquireLock("foo.txt.lock", ref lockFile)) {
+                    continue;
+                }
+
+                lock (_synLock) {
+                    _lockCount++;
+                    Assert.That(_lockCount, Is.EqualTo(1));
+                }
+
+                // keep the lock for a certain time
+                Thread.Sleep(r.Next(200));
+                lock (_synLock) {
+                    _lockCount--;
+                    Assert.That(_lockCount, Is.EqualTo(0));
+                }
+
+                lockFile.Release();
+                return;
+            }
+        }
+
+        private void PlayWithIsLocked() {
+            var r = new Random(DateTime.Now.Millisecond); 
+            ILockFile lockFile = null;
+            const string path = "foo.txt.lock";
+
+            // loop until the lock has been acquired
+            for (;;) {
+                if(_lockFileManager.IsLocked(path)) {
+                    continue;
+                }
+
+                if (!_lockFileManager.TryAcquireLock(path, ref lockFile)) {
+                    continue;
+                }
+
+                lock (_synLock) {
+                    _lockCount++;
+                    Assert.That(_lockCount, Is.EqualTo(1));
+                }
+
+                // keep the lock for a certain time
+                Thread.Sleep(r.Next(200));
+                lock (_synLock) {
+                    _lockCount--;
+                    Assert.That(_lockCount, Is.EqualTo(0));
+                }
+
+                lockFile.Release();
+                return;
+            }
+        }
+
     }
 }

src/Orchard/FileSystems/LockFile/DefaultLockFileManager.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Threading;
 using Orchard.FileSystems.AppData;
 using Orchard.Services;
 
@@ -6,7 +7,7 @@ namespace Orchard.FileSystems.LockFile {
     public class DefaultLockFileManager : ILockFileManager {
         private readonly IAppDataFolder _appDataFolder;
         private readonly IClock _clock;
-
+        private readonly ReaderWriterLockSlim _rwLock = new ReaderWriterLockSlim();
         public static TimeSpan Expiration { get; private set; }
 
         public DefaultLockFileManager(IAppDataFolder appDataFolder, IClock clock) {
@@ -16,38 +17,55 @@ namespace Orchard.FileSystems.LockFile {
         }
 
         public bool TryAcquireLock(string path, ref ILockFile lockFile) {
-            try {
-                if(IsLocked(path)) {
+            if (!_rwLock.TryEnterWriteLock(0)) {
                 return false;
             }
 
-                lockFile = new LockFile(_appDataFolder, path, _clock.UtcNow.ToString());
+            try {
+                if (IsLockedImpl(path)) {
+                    return false;
+                }
+
+                lockFile = new LockFile(_appDataFolder, path, _clock.UtcNow.ToString(), _rwLock);
                 return true;
             }
             catch {
                 // an error occured while reading/creating the lock file
                 return false;
             }
+            finally {
+                _rwLock.ExitWriteLock();
+            }
         }
 
         public bool IsLocked(string path) {
+            _rwLock.EnterWriteLock();
+
             try {
+                return IsLockedImpl(path);
+            }
+            catch {
+                // an error occured while reading the file
+                return true;
+            }
+            finally {
+                _rwLock.ExitWriteLock();
+            }
+        }
+
+        private bool IsLockedImpl(string path) {
             if (_appDataFolder.FileExists(path)) {
                 var content = _appDataFolder.ReadFile(path);
 
                 DateTime creationUtc;
-                    if (DateTime.TryParse(content, out creationUtc)) {
+                if (DateTime.TryParse(content, out creationUtc))
+                {
                     // if expired the file is not removed
                     // it should be automatically as there is a finalizer in LockFile
                     // or the next taker can do it, unless it also fails, again
                     return creationUtc.Add(Expiration) > _clock.UtcNow;
                 }
             }
-            }
-            catch {
-                // an error occured while reading the file
-                return true;
-            }
 
             return false;
         }

src/Orchard/FileSystems/LockFile/LockFile.cs

@@ -1,45 +1,40 @@
-using System;
+using System.Threading;
 using Orchard.FileSystems.AppData;
 
 namespace Orchard.FileSystems.LockFile {
     /// <summary>
-    /// Represents a Lock File acquire on the file system
+    /// Represents a Lock File acquired on the file system
     /// </summary>
+    /// <remarks>
+    /// The instance needs to be disposed in order to release the lock explicitly
+    /// </remarks>
     public class LockFile : ILockFile {
         private readonly IAppDataFolder _appDataFolder;
         private readonly string _path;
         private readonly string _content;
+        private readonly ReaderWriterLockSlim _rwLock;
         private bool _released;
 
-        public LockFile(IAppDataFolder appDataFolder, string path, string content) {
+        public LockFile(IAppDataFolder appDataFolder, string path, string content, ReaderWriterLockSlim rwLock) {
             _appDataFolder = appDataFolder;
             _path = path;
             _content = content;
+            _rwLock = rwLock;
 
             // create the physical lock file
             _appDataFolder.CreateFile(path, content);
         }
 
         public void Dispose() {
-            // dispose both managed and unmanaged resources
-            Dispose(true);
-
-            // don't call the finalizer if dispose is called
-            GC.SuppressFinalize(this);
+            Release();
         }
 
         public void Release() {
-            Dispose(true);
-        }
-
-        protected virtual void Dispose(bool disposing) {
-            if(disposing) {
-                // release managed code here
-                // nothing right now, just a placeholder to preserve the pattern
-            }
+            _rwLock.EnterWriteLock();
 
+            try{
                 if (_released || !_appDataFolder.FileExists(_path)) {
-                // nothing to do, night happen if re-granted, and already released
+                    // nothing to do, might happen if re-granted, and already released
                     return;
                 }
 
@@ -51,10 +46,9 @@ namespace Orchard.FileSystems.LockFile {
                     _appDataFolder.DeleteFile(_path);
                 }
             }
-
-        ~LockFile() {
-            // dispose unmanaged resources (file)
-            Dispose(false);
+            finally {
+                _rwLock.ExitWriteLock();
+            }
         }
     }
 }