Improved locking code.

- Moved DatabaseLock into Framework and making it the default lock provider.
- Removed unnecessary thread synchronization code.
- Allowing multiple lock acquisitions from the same machine (simulating a "lease").
- Updated unit tests.

src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj

@@ -182,7 +182,6 @@
     <Compile Include="Stubs\MessageChannelSelectorStub.cs" />
     <Compile Include="Stubs\ShapeDisplayStub.cs" />
     <Compile Include="Tags\Services\TagsServiceTests.cs" />
-    <Compile Include="TaskLease\DatabaseLockTests.cs" />
     <Compile Include="Themes\Services\ThemeServiceTests.cs" />
     <Compile Include="Users\Controllers\AccountControllerTests.cs" />
     <Compile Include="Users\Services\UserServiceTests.cs" />

src/Orchard.Tests/Orchard.Framework.Tests.csproj

@@ -292,6 +292,7 @@
     <Compile Include="Stubs\StubVirtualPathMonitor.cs" />
     <Compile Include="Stubs\StubCacheManager.cs" />
     <Compile Include="Stubs\StubWebSiteFolder.cs" />
+    <Compile Include="Tasks\DatabaseLockTests.cs" />
     <Compile Include="Tasks\DistributedLockServiceTests.cs" />
     <Compile Include="Time\TimeZoneSelectorTests.cs" />
     <Compile Include="UI\Resources\ResourceManagerTests.cs" />

src/Orchard.Tests/Stubs/StubDistributedLock.cs

@@ -1,23 +1,27 @@
 using System;
-using Orchard.Tasks.Locking;
+using Orchard.Tasks.Locking.Services;
 
 namespace Orchard.Tests.Stubs {
     public class StubDistributedLock : IDistributedLock {
         public static bool IsAcquired { get; private set; }
+        public static string AcquiredByMachineName { get; private set; }
 
         public bool IsDisposed { get; private set; }
 
-        public bool TryAcquire(string name, TimeSpan maxLifetime) {
+        public bool TryAcquire(string name, string machineName, TimeSpan maxLifetime) {
-            if (IsAcquired)
+            if (IsAcquired && machineName != AcquiredByMachineName)
                 return false;
 
-            return IsAcquired = true;
+            IsAcquired = true;
+            AcquiredByMachineName = machineName;
+            return true;
         }
 
 
         public void Dispose() {
             IsDisposed = true;
             IsAcquired = false;
+            AcquiredByMachineName = null;
         }
     }
 }

src/Orchard.Tests/Stubs/StubMachineNameProvider.cs

@@ -2,8 +2,12 @@
 
 namespace Orchard.Tests.Stubs {
     public class StubMachineNameProvider : IMachineNameProvider {
+        public StubMachineNameProvider() {
+            MachineName = "Orchard Machine";
+        }
+        public string MachineName { get; set; }
         public string GetMachineName() {
-            return "Orchard Machine";
+            return MachineName;
         }
     }
 }

src/Orchard.Tests/Tasks/DatabaseLockTests.cs

@@ -4,12 +4,15 @@ using System.Linq;
 using Autofac;
 using NUnit.Framework;
 using Orchard.Data;
-using Orchard.TaskLease.Models;
+using Orchard.Tasks.Locking.Providers;
-using Orchard.TaskLease.Services;
+using Orchard.Tasks.Locking.Records;
 
-namespace Orchard.Tests.Modules.TaskLease {
+namespace Orchard.Tests.Tasks {
     [TestFixture]
     public class DatabaseLockTests : DatabaseEnabledTestsBase {
+        private const string MachineName1 = "Orchard Testing Machine 1";
+        private const string MachineName2 = "Orchard Testing Machine 2";
+        private const string LockName = "Orchard Test Lock";
         private IRepository<DatabaseLockRecord> _databaseLockRepository;
         private DatabaseLock _lock;
 
@@ -32,7 +35,7 @@ namespace Orchard.Tests.Modules.TaskLease {
 
         [Test]
         public void AcquiringLockSucceeds() {
-            var lockAcquired = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60));
+            var lockAcquired = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60));
 
             Assert.That(lockAcquired, Is.True);
             Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(1));
@@ -40,15 +43,15 @@ namespace Orchard.Tests.Modules.TaskLease {
 
         [Test]
         public void DisposingRemovesLockRecord() {
-            _lock.TryAcquire("Test", TimeSpan.FromSeconds(60));
+            _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60));
             _lock.Dispose();
             Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(0));
         }
 
         [Test]
         public void AcquiringLockTwiceFails() {
-            var attempt1 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60));
+            var attempt1 = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60));
-            var attempt2 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60));
+            var attempt2 = _lock.TryAcquire(LockName, MachineName2, TimeSpan.FromSeconds(60));
             
             Assert.That(attempt1, Is.True);
             Assert.That(attempt2, Is.False);
@@ -56,8 +59,8 @@ namespace Orchard.Tests.Modules.TaskLease {
 
         [Test]
         public void AcquiringExpiredLockSucceeds() {
-            var attempt1 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60));
+            var attempt1 = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60));
-            var attempt2 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(-1)); // Treat the previosuly stored lock as immediately expired.
+            var attempt2 = _lock.TryAcquire(LockName, MachineName2, TimeSpan.FromSeconds(-1)); // Treat the previosuly stored lock as immediately expired.
 
             Assert.That(attempt1, Is.True);
             Assert.That(attempt2, Is.True);

src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs

@@ -5,17 +5,19 @@ using System.Threading.Tasks;
 using Autofac;
 using NUnit.Framework;
 using Orchard.Environment;
-using Orchard.Tasks.Locking;
+using Orchard.Tasks.Locking.Services;
 using Orchard.Tests.Stubs;
 
 namespace Orchard.Tests.Tasks {
     [TestFixture]
     public class DistributedLockServiceTests : ContainerTestBase {
+        private const string LockName = "Orchard Test Lock";
         private DistributedLockService _distributedLockService;
+        private StubMachineNameProvider _stubMachineNameProvider;
 
         protected override void Register(ContainerBuilder builder) {
             builder.RegisterType<StubWorkContextAccessor>().As<IWorkContextAccessor>();
-            builder.RegisterType<StubMachineNameProvider>().As<IMachineNameProvider>();
+            builder.RegisterType<StubMachineNameProvider>().As<IMachineNameProvider>().SingleInstance();
             builder.RegisterType<StubDistributedLock>().As<IDistributedLock>();
             builder.RegisterType<DistributedLockService>().AsSelf();
             builder.RegisterInstance(new Work<IDistributedLock>(resolve => _container.Resolve<IDistributedLock>())).AsSelf();
@@ -23,22 +25,35 @@ namespace Orchard.Tests.Tasks {
 
         protected override void Resolve(ILifetimeScope container) {
             _distributedLockService = container.Resolve<DistributedLockService>();
+            _stubMachineNameProvider = (StubMachineNameProvider)container.Resolve<IMachineNameProvider>();
         }
 
 
         [Test]
         public void AcquiringLockSucceeds() {
             IDistributedLock @lock;
-            var lockAcquired = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
+            var lockAcquired = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
             
             Assert.That(lockAcquired, Is.True);
         }
 
+        [Test]
+        public void AcquiringLockTwiceOnSameMachineSucceeds() {
+            IDistributedLock @lock;
+            var attempt1 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
+            var attempt2 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
+
+            Assert.That(attempt1, Is.True);
+            Assert.That(attempt2, Is.True);
+        }
+
         [Test]
         public void AcquiringLockTwiceFails() {
             IDistributedLock @lock;
-            var attempt1 = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
+            _stubMachineNameProvider.MachineName = "Orchard Test Machine 1";
-            var attempt2 = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
+            var attempt1 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
+            _stubMachineNameProvider.MachineName = "Orchard Test Machine 2";
+            var attempt2 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
 
             Assert.That(attempt1, Is.True);
             Assert.That(attempt2, Is.False);
@@ -52,7 +67,8 @@ namespace Orchard.Tests.Tasks {
             foreach (var index in Enumerable.Range(0, 10).AsParallel()) {
                 var task = Task.Factory.StartNew(() => {
                     IDistributedLock @lock;
-                    var attempt = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
+                    _stubMachineNameProvider.MachineName = "Orchard Test Machine " + (index + 1);
+                    var attempt = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock);
                     attempts.Add(attempt);
                 });
 

src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs

@@ -17,14 +17,5 @@ namespace Orchard.TaskLease {
 
             return 1;
         }
-
-        public int UpdateFrom1() {
-            SchemaBuilder.CreateTable("DatabaseLockRecord", table => table
-                .Column<int>("Id", column => column.PrimaryKey().Identity())
-                .Column<string>("Name", column => column.NotNull().WithLength(256))
-                .Column<DateTime>("AcquiredUtc"));
-
-            return 2;
-        }
     }
 }

src/Orchard.Web/Modules/Orchard.TaskLease/Module.txt

@@ -4,5 +4,6 @@ Author: The Orchard Team
 Website: http://orchardtasklease.codeplex.com
 Version: 1.9.1
 OrchardVersion: 1.9
-Description: Provides services to synchronize tasks in a farm environment
+LifecycleStatus: Deprecated
+Description: Provides services to synchronize tasks in a farm environment.
 Category: Hosting

src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj

@@ -76,7 +76,6 @@
   </ItemGroup>
   <ItemGroup>
     <Content Include="Web.config" />
-    <Compile Include="Models\DatabaseLockRecord.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
     <Content Include="Module.txt" />
   </ItemGroup>
@@ -86,7 +85,6 @@
     </Compile>
     <Compile Include="Models\TaskLeaseRecord.cs" />
     <Compile Include="Services\ITaskLeaseService.cs" />
-    <Compile Include="Services\DatabaseLock.cs" />
     <Compile Include="Services\TaskLeaseService.cs" />
   </ItemGroup>
   <ItemGroup />

src/Orchard.sln

@@ -1133,7 +1133,7 @@ Global
 		{642A49D7-8752-4177-80D6-BFBBCFAD3DE0} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5}
 		{966EC390-3C7F-4D98-92A6-F0F30D02E9B1} = {902528F6-1444-42A3-8B75-A54B775B539C}
 		{3158C928-888C-4A84-8BC1-4A8257489538} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5}
-		{3F72A4E9-7B72-4260-B010-C16EC54F9BAF} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5}
+		{3F72A4E9-7B72-4260-B010-C16EC54F9BAF} = {902528F6-1444-42A3-8B75-A54B775B539C}
 		{E07AFA7E-7B36-44C3-A537-AFCCAA93EA7A} = {74E681ED-FECC-4034-B9BD-01B0BB1BDECA}
 		{5531E894-D259-45A3-AA61-26DBE720C1CE} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5}
 		{2969635F-D9C3-4D01-890D-437B46659690} = {74E681ED-FECC-4034-B9BD-01B0BB1BDECA}

src/Orchard/Data/Migration/AutomaticDataMigrations.cs

@@ -3,7 +3,7 @@ using System.Linq;
 using Orchard.Environment;
 using Orchard.Environment.Features;
 using Orchard.Logging;
-using Orchard.Tasks.Locking;
+using Orchard.Tasks.Locking.Services;
 
 namespace Orchard.Data.Migration {
     /// <summary>

src/Orchard/Orchard.Framework.csproj

@@ -397,10 +397,12 @@
     <Compile Include="Services\IJsonConverter.cs" />
     <Compile Include="Settings\CurrentSiteWorkContext.cs" />
     <Compile Include="Settings\ResourceDebugMode.cs" />
-    <Compile Include="Tasks\Locking\DefaultLock.cs" />
+    <Compile Include="Tasks\Locking\Migrations\FrameworkMigrations.cs" />
-    <Compile Include="Tasks\Locking\IDistributedLockService.cs" />
+    <Compile Include="Tasks\Locking\Providers\DatabaseLock.cs" />
-    <Compile Include="Tasks\Locking\IDistributedLock.cs" />
+    <Compile Include="Tasks\Locking\Services\IDistributedLockService.cs" />
-    <Compile Include="Tasks\Locking\DistributedLockService.cs" />
+    <Compile Include="Tasks\Locking\Services\IDistributedLock.cs" />
+    <Compile Include="Tasks\Locking\Services\DistributedLockService.cs" />
+    <Compile Include="Tasks\Locking\Records\DatabaseLockRecord.cs" />
     <Compile Include="Themes\CurrentThemeWorkContext.cs" />
     <Compile Include="Themes\ThemeManager.cs" />
     <Compile Include="Time\CurrentTimeZoneWorkContext.cs" />

src/Orchard/Tasks/Locking/DefaultLock.cs

@@ -1,31 +0,0 @@
-using System;
-
-namespace Orchard.Tasks.Locking {
-
-    public class DefaultLock : IDistributedLock {
-        private readonly IStaticLockSemaphore _semaphore;
-
-        public DefaultLock(IStaticLockSemaphore semaphore) {
-            _semaphore = semaphore;
-        }
-
-        public bool TryAcquire(string name, TimeSpan maxLifetime) {
-            if (_semaphore.IsAcquired)
-                return false;
-
-            return _semaphore.IsAcquired = true;
-        }
-
-        public void Dispose() {
-            _semaphore.IsAcquired = false;
-        }
-
-        public interface IStaticLockSemaphore : ISingletonDependency {
-            bool IsAcquired { get; set; }
-        }
-
-        public class StaticLockSemaphore : IStaticLockSemaphore {
-            public bool IsAcquired { get; set; }
-        }
-    }
-}

src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs

@@ -0,0 +1,17 @@
+using System;
+using Orchard.Data.Migration;
+
+namespace Orchard.Tasks.Locking.Migrations {
+    public class FrameworkMigrations : DataMigrationImpl {
+
+        public int Create() {
+            SchemaBuilder.CreateTable("DatabaseLockRecord", table => table
+                .Column<int>("Id", column => column.PrimaryKey().Identity())
+                .Column<string>("Name", column => column.NotNull().WithLength(256))
+                .Column<string>("MachineName", column => column.WithLength(256))
+                .Column<DateTime>("AcquiredUtc"));
+
+            return 1;
+        }
+    }
+}

src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs

@@ -3,20 +3,18 @@ using System.Data;
 using System.Linq;
 using Autofac;
 using Orchard.Data;
-using Orchard.Environment.Extensions;
 using Orchard.Exceptions;
 using Orchard.Logging;
 using Orchard.Services;
-using Orchard.TaskLease.Models;
+using Orchard.Tasks.Locking.Records;
-using Orchard.Tasks.Locking;
+using Orchard.Tasks.Locking.Services;
 using Orchard.Validation;
 
-namespace Orchard.TaskLease.Services {
+namespace Orchard.Tasks.Locking.Providers {
 
     /// <summary>
     /// Provides a database driven implementation of <see cref="IDistributedLock" />
     /// </summary>
-    [OrchardSuppressDependency("Orchard.Tasks.Locking.DefaultLock")]
     public class DatabaseLock : IDistributedLock {
         private readonly ILifetimeScope _lifetimeScope;
         private readonly IClock _clock;
@@ -34,7 +32,7 @@ namespace Orchard.TaskLease.Services {
 
         public ILogger Logger { get; set; }
 
-        public bool TryAcquire(string name, TimeSpan maxLifetime) {
+        public bool TryAcquire(string name, string machineName, TimeSpan maxLifetime) {
             if(_isDisposed)
                 throw new ObjectDisposedException("DatabaseLock");
 
@@ -53,9 +51,10 @@ namespace Orchard.TaskLease.Services {
                 var record = repository.Table.FirstOrDefault(x => x.Name == name);
 
                 if (record != null) {
-                    // There is a nexisting lock, but check if it has expired.
+                    // There is an existing lock, but check if it has expired or if the current machine is requesting the lock.
                     var isExpired = record.AcquiredUtc + maxLifetime < _clock.UtcNow;
-                    if (isExpired) {
+                    var isCurrentMachine = record.MachineName == machineName;
+                    if (isExpired || isCurrentMachine) {
                         repository.Delete(record);
                         record = null;
                     }
@@ -66,6 +65,7 @@ namespace Orchard.TaskLease.Services {
                 if (canAcquire) {
                     record = new DatabaseLockRecord {
                         Name = name,
+                        MachineName = machineName,
                         AcquiredUtc = _clock.UtcNow
                     };
                     repository.Create(record);

src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs

@@ -1,9 +1,10 @@
 using System;
 
-namespace Orchard.TaskLease.Models {
+namespace Orchard.Tasks.Locking.Records {
     public class DatabaseLockRecord {
         public virtual int Id { get; set; }
         public virtual string Name { get; set; }
+        public virtual string MachineName { get; set; }
         public virtual DateTime? AcquiredUtc { get; set; }
     }
 }

src/Orchard/Tasks/Locking/Services/DistributedLockService.cs

@@ -4,9 +4,8 @@ using System.Threading.Tasks;
 using Orchard.Environment;
 using Orchard.Logging;
 
-namespace Orchard.Tasks.Locking {
+namespace Orchard.Tasks.Locking.Services {
     public class DistributedLockService : IDistributedLockService {
-        private readonly ReaderWriterLockSlim _rwl = new ReaderWriterLockSlim();
         private readonly IWorkContextAccessor _wca;
         private readonly IMachineNameProvider _machineNameProvider;
 
@@ -22,31 +21,26 @@ namespace Orchard.Tasks.Locking {
             var machineName = _machineNameProvider.GetMachineName();
             @lock = Resolve<IDistributedLock>();
 
-            if (_rwl.TryEnterWriteLock(0)) {
+            try {
-                try {
+                var waitedTime = TimeSpan.Zero;
-                    var waitedTime = TimeSpan.Zero;
+                var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10);
-                    var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10);
+                bool acquired;
-                    bool acquired;
 
-                    while (!(acquired = @lock.TryAcquire(name, maxLifetime)) && waitedTime < timeout) {
+                while (!(acquired = @lock.TryAcquire(name, machineName, maxLifetime)) && waitedTime < timeout) {
-                        Task.Delay(timeout).ContinueWith(t => {
+                    Task.Delay(timeout).ContinueWith(t => {
-                            waitedTime += waitTime;
+                        waitedTime += waitTime;
-                        }).Wait();
+                    }).Wait();
-                    }
+                }
 
-                    if (acquired) {
+                if (acquired) {
-                        Logger.Debug("Successfully acquired a lock named {0} on machine {1}.", name, machineName);
+                    Logger.Debug("Successfully acquired a lock named {0} on machine {1}.", name, machineName);
-                        return true;
+                    return true;
-                    }
-                }
-                catch (Exception ex) {
-                    Logger.Error(ex, "Error while trying to acquire a lock named {0} on machine {1}.", name, machineName);
-                    throw;
-                }
-                finally {
-                    _rwl.ExitWriteLock();
                 }
             }
+            catch (Exception ex) {
+                Logger.Error(ex, "Error while trying to acquire a lock named {0} on machine {1}.", name, machineName);
+                throw;
+            }
 
             Logger.Debug("Could not acquire a lock named {0} on machine {1}.", name, machineName);
             return false;

src/Orchard/Tasks/Locking/Services/IDistributedLock.cs

@@ -1,6 +1,6 @@
 using System;
 
-namespace Orchard.Tasks.Locking {
+namespace Orchard.Tasks.Locking.Services {
     /// <summary>
     /// Provides a lock on a provided name.
     /// </summary>
@@ -9,8 +9,9 @@ namespace Orchard.Tasks.Locking {
         /// Tries to acquire a lock on the specified name.
         /// </summary>
         /// <param name="name">The name to use for the lock.</param>
+        /// <param name="machineName">The machine name trying to acquire a lock.</param>
         /// <param name="maxLifetime">The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock.</param>
         /// <returns>Returns true if a lock was acquired, false otherwise.</returns>
-        bool TryAcquire(string name, TimeSpan maxLifetime);
+        bool TryAcquire(string name, string machineName, TimeSpan maxLifetime);
     }
 }

src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs

@@ -1,6 +1,6 @@
 using System;
 
-namespace Orchard.Tasks.Locking {
+namespace Orchard.Tasks.Locking.Services {
     /// <summary>
     /// Provides distributed locking functionality.
     /// </summary>