From 6c712a984027f8f3f143a499705e3133d11ca1c9 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Fri, 21 Aug 2015 15:22:32 +0100 Subject: [PATCH] Fixed bugs, refactored and polished distributed lock service. - Added machine and thread scoped locking. - Added support for blocking acquisitions. - Added thread synchronization when creating and committing nested transactions for lock record creation and updates. --- .../Orchard.Framework.Tests.csproj | 2 +- .../Stubs/StubDistributedLock.cs | 15 - src/Orchard.Tests/Stubs/StubThreadProvider.cs | 15 + .../Tasks/DistributedLockServiceTests.cs | 202 ++++++++++---- src/Orchard.Tests/Tasks/LockTests.cs | 13 +- .../Data/Migration/AutomaticDataMigrations.cs | 38 +-- .../Environment/IMachineNameProvider.cs | 2 +- src/Orchard/Environment/IThreadProvider.cs | 13 + src/Orchard/Environment/OrchardStarter.cs | 1 + src/Orchard/Environment/ThreadProvider.cs | 9 + src/Orchard/Orchard.Framework.csproj | 7 +- .../Locking/Migrations/FrameworkMigrations.cs | 7 +- ...LockRecord.cs => DistributedLockRecord.cs} | 7 +- .../Tasks/Locking/Services/DistributedLock.cs | 46 ++++ .../Services/DistributedLockService.cs | 259 +++++++++++------- .../Locking/Services/IDistributedLock.cs | 11 - .../Services/IDistributedLockService.cs | 85 +++++- src/Orchard/Tasks/Locking/Services/Lock.cs | 31 --- 18 files changed, 507 insertions(+), 256 deletions(-) delete mode 100644 src/Orchard.Tests/Stubs/StubDistributedLock.cs create mode 100644 src/Orchard.Tests/Stubs/StubThreadProvider.cs create mode 100644 src/Orchard/Environment/IThreadProvider.cs create mode 100644 src/Orchard/Environment/ThreadProvider.cs rename src/Orchard/Tasks/Locking/Records/{LockRecord.cs => DistributedLockRecord.cs} (58%) create mode 100644 src/Orchard/Tasks/Locking/Services/DistributedLock.cs delete mode 100644 src/Orchard/Tasks/Locking/Services/IDistributedLock.cs delete mode 100644 src/Orchard/Tasks/Locking/Services/Lock.cs diff --git a/src/Orchard.Tests/Orchard.Framework.Tests.csproj b/src/Orchard.Tests/Orchard.Framework.Tests.csproj index 7f39003cf..ef222e44b 100644 --- a/src/Orchard.Tests/Orchard.Framework.Tests.csproj +++ b/src/Orchard.Tests/Orchard.Framework.Tests.csproj @@ -269,7 +269,7 @@ - + diff --git a/src/Orchard.Tests/Stubs/StubDistributedLock.cs b/src/Orchard.Tests/Stubs/StubDistributedLock.cs deleted file mode 100644 index e02d7883d..000000000 --- a/src/Orchard.Tests/Stubs/StubDistributedLock.cs +++ /dev/null @@ -1,15 +0,0 @@ -using System; -using Orchard.Tasks.Locking.Services; - -namespace Orchard.Tests.Stubs { - public class StubDistributedLock : IDistributedLock { - public bool IsDisposed { get; private set; } - - public void Dispose() { - IsDisposed = true; - } - - public int Id { get; set; } - public string Name { get; set; } - } -} diff --git a/src/Orchard.Tests/Stubs/StubThreadProvider.cs b/src/Orchard.Tests/Stubs/StubThreadProvider.cs new file mode 100644 index 000000000..73d6ebcaf --- /dev/null +++ b/src/Orchard.Tests/Stubs/StubThreadProvider.cs @@ -0,0 +1,15 @@ +using Orchard.Environment; + +namespace Orchard.Tests.Stubs { + public class StubThreadProvider : IThreadProvider { + public StubThreadProvider() { + ManagedThreadId = 1; + } + + public int ManagedThreadId { get; set; } + + public int GetCurrentThreadId() { + return ManagedThreadId; + } + } +} diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index c7d2bed92..6f789d8e4 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Threading.Tasks; using Autofac; using NUnit.Framework; @@ -16,103 +15,208 @@ namespace Orchard.Tests.Tasks { public class DistributedLockServiceTests : DatabaseEnabledTestsBase { private const string LockName = "Orchard Test Lock"; private DistributedLockService _distributedLockService; - private StubMachineNameProvider _stubMachineNameProvider; - private IRepository _lockRepository; + private StubMachineNameProvider _machineNameProvider; + private StubThreadProvider _threadProvider; + private IRepository _distributedLockRepository; - protected override IEnumerable DatabaseTypes - { - get { yield return typeof (LockRecord); } + protected override IEnumerable DatabaseTypes { + get { yield return typeof(DistributedLockRecord); } } public override void Register(ContainerBuilder builder) { builder.RegisterType().As(); - //builder.RegisterType().As(); builder.RegisterType().As().SingleInstance(); - builder.RegisterType().As(); + builder.RegisterType().As().SingleInstance(); builder.RegisterType().AsSelf(); - builder.RegisterInstance(new Work(resolve => _container.Resolve())).AsSelf(); } public override void Init() { base.Init(); _distributedLockService = _container.Resolve(); - _stubMachineNameProvider = (StubMachineNameProvider)_container.Resolve(); - _lockRepository = _container.Resolve>(); + _machineNameProvider = (StubMachineNameProvider)_container.Resolve(); + _threadProvider = (StubThreadProvider)_container.Resolve(); + _distributedLockRepository = _container.Resolve>(); } - [Test] - public void AcquiringLockSucceeds() { - IDistributedLock @lock; - var lockAcquired = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); - + public void TryAcquiringLockSucceeds() { + DistributedLock @lock; + var lockAcquired = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, 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); + public void TryAcquiringLockTwiceOnSameMachineSucceeds() { + DistributedLock @lock; + var attempt1 = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); + var attempt2 = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); Assert.That(attempt1, Is.True); Assert.That(attempt2, Is.True); } [Test] - public void AcquiringLockTwiceOnSameMachineIncreasesRefCountTwice() { - IDistributedLock @lock; - _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); - _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + public void TryAcquiringLockTwiceOnSameMachineIncreasesLockCountTwice() { + DistributedLock @lock; + _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); + _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); - var lockRecord = _lockRepository.Get(@lock.Id); - Assert.That(lockRecord.ReferenceCount, Is.EqualTo(2)); + var lockId = Int32.Parse(@lock.Id); + var lockRecord = _distributedLockRepository.Get(lockId); + Assert.That(lockRecord.Count, Is.EqualTo(2)); } [Test] - public void DisposingLockWillDecreaseRefCount() { - IDistributedLock @lock; - _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); - _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); - var lockRecord = _lockRepository.Get(@lock.Id); + public void ReleasingLockDecreasesLockCount() { + DistributedLock @lock; + _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); + _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); - _distributedLockService.DisposeLock(@lock); - Assert.That(lockRecord.ReferenceCount, Is.EqualTo(1)); + var lockId = Int32.Parse(@lock.Id); + var lockRecord = _distributedLockRepository.Get(lockId); - _distributedLockService.DisposeLock(@lock); - Assert.That(lockRecord.ReferenceCount, Is.EqualTo(0)); + _distributedLockService.ReleaseLock(@lock); + Assert.That(lockRecord.Count, Is.EqualTo(1)); + + _distributedLockService.ReleaseLock(@lock); + Assert.That(lockRecord.Count, Is.EqualTo(0)); } [Test] - public void AcquiringLockTwiceFails() { - IDistributedLock @lock; - _stubMachineNameProvider.MachineName = "Orchard Test Machine 1"; - 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); + public void TryAcquiringLockTwiceFails() { + DistributedLock @lock; + _machineNameProvider.MachineName = "Orchard Test Machine 1"; + var attempt1 = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); + _machineNameProvider.MachineName = "Orchard Test Machine 2"; + var attempt2 = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); Assert.That(attempt1, Is.True); Assert.That(attempt2, Is.False); } [Test] - public void MultipleSimultaneousAcquisitionsShouldAllowOneLock() { - var attempts = new List(); + public void TryAcquiringNonExpiredActiveLockFails() { + DistributedLock @lock; + CreateNonExpiredActiveLock("Other Machine", threadId: null); + var success = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromHours(1), null, out @lock); + + Assert.That(success, Is.False); + } + + [Test] + public void TryAcquiringNonExpiredButInactiveLockSucceeds() { + DistributedLock @lock; + CreateNonExpiredButInactiveLock("Other Machine", threadId: null); + var success = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromHours(1), null, out @lock); + + Assert.That(success, Is.True); + } + + [Test] + public void TryAcquiringExpiredButActiveLockSucceeds() { + DistributedLock @lock; + CreateExpiredButActiveLock("Other Machine", threadId: null); + var success = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromHours(1), null, out @lock); + + Assert.That(success, Is.True); + } + + [Test] + public void TryAcquiringNonExpiredAndActiveLockFromCurrentOwnerSucceeds() { + DistributedLock @lock; + CreateNonExpiredActiveLock(GetMachineName(), threadId: null); + var success = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromHours(1), null, out @lock); + + Assert.That(success, Is.True); + } + + [Test] + public void AcquiringNonExpiredAndActiveLockFromDifferentOwnerThrowsTimeoutException() { + CreateNonExpiredActiveLock("Other Machine", threadId: null); + Assert.Throws(() => _distributedLockService.AcquireLockForMachine(LockName, TimeSpan.FromHours(1), TimeSpan.Zero)); + } + + [Test] + public void MultipleAcquisitionsFromDifferentMachinesShouldFail() { + DistributedLock @lock; + _machineNameProvider.MachineName = "Orchard Test Machine 1"; + var attempt1 = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); + _machineNameProvider.MachineName = "Orchard Test Machine 2"; + var attempt2 = _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); + + Assert.That(attempt1, Is.True); + Assert.That(attempt2, Is.False); + } + + [Test] + public void MultithreadedAcquisitionsShouldNotCauseTransactionErrors() { var tasks = new List(); - foreach (var index in Enumerable.Range(0, 20)) { + for (var i = 0; i < 10; i++) { var task = Task.Factory.StartNew(() => { - IDistributedLock @lock; - _stubMachineNameProvider.MachineName = "Orchard Test Machine " + (index + 1); - var attempt = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); - attempts.Add(attempt); + DistributedLock @lock; + Assert.DoesNotThrow(() => _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromHours(1), null, out @lock)); }); tasks.Add(task); } Task.WaitAll(tasks.ToArray()); - Assert.That(attempts.Count(x => x == true), Is.EqualTo(1)); + } + + [Test] + public void MixedScopeAcquisitionsShouldThrow() { + DistributedLock @lock; + Assert.DoesNotThrow(() => _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock)); + Assert.Throws(() => _distributedLockService.TryAcquireLockForThread(LockName, TimeSpan.FromSeconds(60), null, out @lock)); + } + + [Test] + public void TryAcquireActiveLockWithNullTimeoutReturnsFalseImmediately() { + CreateNonExpiredActiveLock("Other Machine", null); + + DistributedLock @lock; + var acquired = _distributedLockService.TryAcquireLockForThread(LockName, TimeSpan.FromMinutes(1), null, out @lock); + + Assert.That(acquired, Is.False); + } + + private DistributedLockRecord CreateLockRecord(int count, DateTime createdUtc, DateTime validUntilUtc, string machineName, int? threadId) { + var record = new DistributedLockRecord { + Name = LockName, + Count = count, + CreatedUtc = createdUtc, + ValidUntilUtc = validUntilUtc, + MachineName = machineName, + ThreadId = threadId + }; + + _distributedLockRepository.Create(record); + return record; + } + + private DistributedLockRecord CreateNonExpiredActiveLock(string machineName, int? threadId) { + var now = _clock.UtcNow; + return CreateLockRecord(1, now, now + TimeSpan.FromHours(1), machineName, threadId); + } + + private DistributedLockRecord CreateNonExpiredButInactiveLock(string machineName, int? threadId) { + var now = _clock.UtcNow; + return CreateLockRecord(0, now, now + TimeSpan.FromHours(1), machineName, threadId); + } + + private DistributedLockRecord CreateExpiredButActiveLock(string machineName, int? threadId) { + var now = _clock.UtcNow; + return CreateLockRecord(1, now, now - TimeSpan.FromHours(1), machineName, threadId); + } + + private string GetMachineName() { + return _machineNameProvider.GetMachineName(); + } + + private int GetThreadId() { + return _threadProvider.GetCurrentThreadId(); } } } diff --git a/src/Orchard.Tests/Tasks/LockTests.cs b/src/Orchard.Tests/Tasks/LockTests.cs index eaa09e9e4..d94218428 100644 --- a/src/Orchard.Tests/Tasks/LockTests.cs +++ b/src/Orchard.Tests/Tasks/LockTests.cs @@ -1,5 +1,4 @@ -using System; -using Autofac; +using Autofac; using Moq; using NUnit.Framework; using Orchard.Tasks.Locking.Services; @@ -8,9 +7,9 @@ namespace Orchard.Tests.Tasks { [TestFixture] public class LockTests : ContainerTestBase { private const string LockName = "Orchard Test Lock"; - private const int LockId = 1; + private const string LockId = "1"; private Mock _distributedLockServiceMock; - private Lock _lock; + private DistributedLock _lock; protected override void Register(ContainerBuilder builder) { _distributedLockServiceMock = new Mock(); @@ -18,14 +17,14 @@ namespace Orchard.Tests.Tasks { } protected override void Resolve(ILifetimeScope container) { - _lock = new Lock(_distributedLockServiceMock.Object, LockName, LockId); + _lock = DistributedLock.ForMachine(_distributedLockServiceMock.Object, LockName, "Orchard Test Machine", LockId); } [Test] public void DisposeInvokesDistributedLockServiceDisposeLock() { _lock.Dispose(); - _distributedLockServiceMock.Verify(service => service.DisposeLock(_lock), Times.Exactly(1)); + _distributedLockServiceMock.Verify(service => service.ReleaseLock(_lock), Times.Exactly(1)); } [Test] @@ -34,7 +33,7 @@ namespace Orchard.Tests.Tasks { _lock.Dispose(); _lock.Dispose(); - _distributedLockServiceMock.Verify(service => service.DisposeLock(_lock), Times.Exactly(1)); + _distributedLockServiceMock.Verify(service => service.ReleaseLock(_lock), Times.Exactly(1)); } } } \ No newline at end of file diff --git a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs index 26cd8bc21..45b5b6bb6 100644 --- a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs +++ b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs @@ -29,27 +29,27 @@ namespace Orchard.Data.Migration { public ILogger Logger { get; set; } public void Activated() { - using(var @lock = _distributedLockService.AcquireLock(GetType().FullName, TimeSpan.FromMinutes(30), TimeSpan.FromMilliseconds(250))) { - if (@lock == null) - return; + DistributedLock @lock; + if(_distributedLockService.TryAcquireLockForMachine(GetType().FullName, TimeSpan.FromMinutes(30), TimeSpan.FromMilliseconds(250), out @lock)) { + using (@lock) { + // Let's make sure that the basic set of features is enabled. If there are any that are not enabled, then let's enable them first. + var theseFeaturesShouldAlwaysBeActive = new[] { + "Common", "Containers", "Contents", "Dashboard", "Feeds", "Navigation", "Scheduling", "Settings", "Shapes", "Title" + }; - // Let's make sure that the basic set of features is enabled. If there are any that are not enabled, then let's enable them first. - var theseFeaturesShouldAlwaysBeActive = new[] { - "Common", "Containers", "Contents", "Dashboard", "Feeds", "Navigation", "Scheduling", "Settings", "Shapes", "Title" - }; - - var enabledFeatures = _featureManager.GetEnabledFeatures().Select(f => f.Id).ToList(); - var featuresToEnable = theseFeaturesShouldAlwaysBeActive.Where(shouldBeActive => !enabledFeatures.Contains(shouldBeActive)).ToList(); - if (featuresToEnable.Any()) { - _featureManager.EnableFeatures(featuresToEnable, true); - } - - foreach (var feature in _dataMigrationManager.GetFeaturesThatNeedUpdate()) { - try { - _dataMigrationManager.Update(feature); + var enabledFeatures = _featureManager.GetEnabledFeatures().Select(f => f.Id).ToList(); + var featuresToEnable = theseFeaturesShouldAlwaysBeActive.Where(shouldBeActive => !enabledFeatures.Contains(shouldBeActive)).ToList(); + if (featuresToEnable.Any()) { + _featureManager.EnableFeatures(featuresToEnable, true); } - catch (Exception e) { - Logger.Error("Could not run migrations automatically on " + feature, e); + + foreach (var feature in _dataMigrationManager.GetFeaturesThatNeedUpdate()) { + try { + _dataMigrationManager.Update(feature); + } + catch (Exception ex) { + Logger.Error(ex, "Could not run migrations automatically on {0}.", feature); + } } } } diff --git a/src/Orchard/Environment/IMachineNameProvider.cs b/src/Orchard/Environment/IMachineNameProvider.cs index a7ea31180..93391e934 100644 --- a/src/Orchard/Environment/IMachineNameProvider.cs +++ b/src/Orchard/Environment/IMachineNameProvider.cs @@ -1,7 +1,7 @@ namespace Orchard.Environment { /// - /// Describes a service which the name of the machine running the application. + /// Describes a service which returns the name of the machine running the application. /// public interface IMachineNameProvider { diff --git a/src/Orchard/Environment/IThreadProvider.cs b/src/Orchard/Environment/IThreadProvider.cs new file mode 100644 index 000000000..29458bf27 --- /dev/null +++ b/src/Orchard/Environment/IThreadProvider.cs @@ -0,0 +1,13 @@ +namespace Orchard.Environment { + + /// + /// Describes a service which returns the managed thread ID of the current thread. + /// + public interface IThreadProvider { + + /// + /// Returns the managed thread ID of the current thread. + /// + int GetCurrentThreadId(); + } +} \ No newline at end of file diff --git a/src/Orchard/Environment/OrchardStarter.cs b/src/Orchard/Environment/OrchardStarter.cs index 66ea2a066..3d3dd6958 100644 --- a/src/Orchard/Environment/OrchardStarter.cs +++ b/src/Orchard/Environment/OrchardStarter.cs @@ -67,6 +67,7 @@ namespace Orchard.Environment { builder.RegisterType().As().SingleInstance(); builder.RegisterType().As().SingleInstance(); builder.RegisterType().As().SingleInstance(); + builder.RegisterType().As().SingleInstance(); //builder.RegisterType().As().SingleInstance(); RegisterVolatileProvider(builder); diff --git a/src/Orchard/Environment/ThreadProvider.cs b/src/Orchard/Environment/ThreadProvider.cs new file mode 100644 index 000000000..d3a65d8fb --- /dev/null +++ b/src/Orchard/Environment/ThreadProvider.cs @@ -0,0 +1,9 @@ +using System.Threading; + +namespace Orchard.Environment { + public class ThreadProvider : IThreadProvider { + public int GetCurrentThreadId() { + return Thread.CurrentThread.ManagedThreadId; + } + } +} \ No newline at end of file diff --git a/src/Orchard/Orchard.Framework.csproj b/src/Orchard/Orchard.Framework.csproj index d75ad7574..716347ef5 100644 --- a/src/Orchard/Orchard.Framework.csproj +++ b/src/Orchard/Orchard.Framework.csproj @@ -151,6 +151,8 @@ + + @@ -398,11 +400,10 @@ - + - - + diff --git a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs index 925b98c86..47e8a42c9 100644 --- a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs +++ b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs @@ -5,11 +5,12 @@ namespace Orchard.Tasks.Locking.Migrations { public class FrameworkMigrations : DataMigrationImpl { public int Create() { - SchemaBuilder.CreateTable("LockRecord", table => table + SchemaBuilder.CreateTable("DistributedLockRecord", table => table .Column("Id", column => column.PrimaryKey().Identity()) .Column("Name", column => column.NotNull().WithLength(256)) - .Column("Owner", column => column.WithLength(256)) - .Column("ReferenceCount") + .Column("MachineName", column => column.WithLength(256)) + .Column("ThreadId", column => column.Nullable()) + .Column("Count") .Column("CreatedUtc") .Column("ValidUntilUtc")); diff --git a/src/Orchard/Tasks/Locking/Records/LockRecord.cs b/src/Orchard/Tasks/Locking/Records/DistributedLockRecord.cs similarity index 58% rename from src/Orchard/Tasks/Locking/Records/LockRecord.cs rename to src/Orchard/Tasks/Locking/Records/DistributedLockRecord.cs index 9cb44d3de..586d1ddd7 100644 --- a/src/Orchard/Tasks/Locking/Records/LockRecord.cs +++ b/src/Orchard/Tasks/Locking/Records/DistributedLockRecord.cs @@ -1,11 +1,12 @@ using System; namespace Orchard.Tasks.Locking.Records { - public class LockRecord { + public class DistributedLockRecord { public virtual int Id { get; set; } public virtual string Name { get; set; } - public virtual string Owner { get; set; } - public virtual int ReferenceCount { get; set; } + public virtual string MachineName { get; set; } + public virtual int? ThreadId { get; set; } + public virtual int Count { get; set; } public virtual DateTime CreatedUtc { get; set; } public virtual DateTime ValidUntilUtc { get; set; } } diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLock.cs b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs new file mode 100644 index 000000000..cd7143fc1 --- /dev/null +++ b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs @@ -0,0 +1,46 @@ +using System; +using System.Threading; + +namespace Orchard.Tasks.Locking.Services { + + /// + /// Represents a distributed lock. /> + /// + public class DistributedLock : IDisposable { + private IDistributedLockService _service; + private int _isDisposed; + + private DistributedLock() { + } + + public string Id { get; private set; } + public string Name { get; private set; } + public string MachineName { get; private set; } + public int? ThreadId { get; private set; } + + // This will be called at least and at the latest by the IoC container when the request ends. + public void Dispose() { + if(Interlocked.CompareExchange(ref _isDisposed, 1, 0) == 0) + _service.ReleaseLock(this); + } + + public static DistributedLock ForMachine(IDistributedLockService service, string name, string machineName, string lockId) { + return new DistributedLock { + _service = service, + Name = name, + MachineName = machineName, + Id = lockId + }; + } + + public static DistributedLock ForThread(IDistributedLockService service, string name, string machineName, int threadId, string lockId) { + return new DistributedLock { + _service = service, + Name = name, + MachineName = machineName, + ThreadId = threadId, + Id = lockId + }; + } + } +} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs index e2211980a..1aa581123 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -1,7 +1,6 @@ using System; using System.Data; using System.Linq; -using System.Threading; using System.Threading.Tasks; using Autofac; using Orchard.Data; @@ -13,137 +12,189 @@ using Orchard.Tasks.Locking.Records; namespace Orchard.Tasks.Locking.Services { - public class DistributedLockService : IDistributedLockService { + public class DistributedLockService : Component, IDistributedLockService { private readonly IMachineNameProvider _machineNameProvider; private readonly ILifetimeScope _lifetimeScope; private readonly IClock _clock; - private readonly object _semaphore = new object(); + private readonly object _transactionManagerLock = new object(); + private readonly IThreadProvider _threadProvider; - public DistributedLockService(IMachineNameProvider machineNameProvider, ILifetimeScope lifetimeScope, IClock clock) { + public DistributedLockService(IMachineNameProvider machineNameProvider, IThreadProvider threadProvider, ILifetimeScope lifetimeScope, IClock clock) { _machineNameProvider = machineNameProvider; _lifetimeScope = lifetimeScope; _clock = clock; - Logger = NullLogger.Instance; + _threadProvider = threadProvider; } - public ILogger Logger { get; set; } + public bool TryAcquireLockForMachine(string name, TimeSpan maxValidFor, TimeSpan? timeout, out DistributedLock @lock) { + return TryAcquireLock(name, maxValidFor, timeout, GetMachineName(), null, out @lock); + } - public bool TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout, out IDistributedLock @lock) { - lock (_semaphore) { - @lock = default(IDistributedLock); + public DistributedLock AcquireLockForMachine(string name, TimeSpan maxValidFor, TimeSpan? timeout) { + return AcquireLock(name, maxValidFor, timeout, GetMachineName(), null); + } + + public bool TryAcquireLockForThread(string name, TimeSpan maxValidFor, TimeSpan? timeout, out DistributedLock @lock) { + return TryAcquireLock(name, maxValidFor, timeout, GetMachineName(), GetThreadId(), out @lock); + } + + public DistributedLock AcquireLockForThread(string name, TimeSpan maxValidFor, TimeSpan? timeout) { + return AcquireLock(name, maxValidFor, timeout, GetMachineName(), GetThreadId()); + } + + public void ReleaseLock(DistributedLock @lock) { + lock (_transactionManagerLock) { + var childLifetimeScope = CreateChildLifetimeScope(@lock.Name); try { - var waitedTime = TimeSpan.Zero; - var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); - bool acquired; + var repository = childLifetimeScope.Resolve>(); + var transactionManager = childLifetimeScope.Resolve(); + transactionManager.RequireNew(IsolationLevel.ReadCommitted); + var lockId = Int32.Parse(@lock.Id); + var record = repository.Get(lockId); - while (!(acquired = TryAcquireLockRecord(name, maxLifetime, out @lock)) && waitedTime < timeout) { - Task.Delay(timeout).ContinueWith(t => { - waitedTime += waitTime; - }).Wait(); - } + if (record == null) + throw new ObjectDisposedException("@lock", "No lock record could be found for the specified lock to be released."); - if (acquired) { - Logger.Debug("Successfully acquired a lock named {0}.", name); - return true; - } + if (record.Count <= 0) + throw new ObjectDisposedException("@lock", "The specified lock has already been released."); + + record.Count--; } catch (Exception ex) { - Logger.Error(ex, "Error while trying to acquire a lock named {0}.", name); - throw; + if (ex.IsFatal()) throw; + Logger.Error(ex, "An non-fatal error occurred while trying to dispose a distributed lock with name '{0}' and ID {1}.", @lock.Name, @lock.Id); + } + finally { + childLifetimeScope.Dispose(); } - - Logger.Debug("Could not acquire a lock named {0}.", name); - return false; } } - public IDistributedLock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout) { - IDistributedLock lockResult; - return TryAcquireLock(name, maxLifetime, timeout, out lockResult) ? lockResult : null; - } - - public void DisposeLock(IDistributedLock @lock) { - var childLifetimeScope = CreateChildLifetimeScope(@lock.Name); - - try { - var repository = childLifetimeScope.Resolve>(); - var transactionManager = childLifetimeScope.Resolve(); - transactionManager.RequireNew(IsolationLevel.ReadCommitted); - var record = repository.Get(@lock.Id); - - if (record != null) { - if (record.ReferenceCount > 0) - record.ReferenceCount--; - } - } - catch (Exception ex) { - if (ex.IsFatal()) throw; - Logger.Error(ex, "An non-fatal error occurred while trying to dispose a distributed lock with name {0} and ID {1}.", @lock.Name, @lock.Id); - } - finally { - childLifetimeScope.Dispose(); - } - } - - protected virtual bool TryAcquireLockRecord(string name, TimeSpan maxLifetime, out IDistributedLock @lock) { - @lock = null; - var childLifetimeScope = CreateChildLifetimeScope(name); - - try { - var transactionManager = childLifetimeScope.Resolve(); - transactionManager.RequireNew(IsolationLevel.ReadCommitted); - - // This way we can create a nested transaction scope instead of having the unwanted effect - // of manipulating the transaction of the caller. - var repository = childLifetimeScope.Resolve>(); - - // Find an existing, active lock, if any. - var record = repository.Table.FirstOrDefault(x => x.Name == name && (x.ValidUntilUtc >= _clock.UtcNow || x.ReferenceCount > 0)); - - // The current owner name (based on machine name and current thread ID). - var currentOwnerName = GetOwnerName(); - var canAcquireLock = false; - - // Check if there's already an active lock. - if (record != null) { - // Check if the owner of the lock is the one trying to acquire the lock. - if (record.Owner == currentOwnerName) { - record.ReferenceCount++; - canAcquireLock = true; - } - } - else { - // No one has an active lock yet, so good to go. - record = new LockRecord { - Name = name, - Owner = currentOwnerName, - ReferenceCount = 1, - CreatedUtc = _clock.UtcNow, - ValidUntilUtc = _clock.UtcNow + maxLifetime - }; - repository.Create(record); - repository.Flush(); - canAcquireLock = true; - } - - if (!canAcquireLock) - return false; - - @lock = new Lock(this, name, record.Id); + private bool TryAcquireLock(string name, TimeSpan maxValidFor, TimeSpan? timeout, string machineName, int? threadId, out DistributedLock @lock) { + @lock = AcquireLockInternal(name, maxValidFor, machineName, threadId, timeout.GetValueOrDefault()); + if (@lock != null) return true; + + Logger.Debug("Could not acquire a lock named '{0}'.", name); + return false; + } + + private DistributedLock AcquireLock(string name, TimeSpan maxValidFor, TimeSpan? timeout, string machineName, int? threadId) { + var @lock = AcquireLockInternal(name, maxValidFor, machineName, threadId, timeout); + if (@lock != null) + return @lock; + + throw new TimeoutException("Could not acquire a lock within the specified amount of time."); + } + + private DistributedLock AcquireLockInternal(string name, TimeSpan maxValidFor, string machineName, int? threadId, TimeSpan? timeout = null) { + try { + DistributedLockRecord record = null; + var acquired = Poll(() => (record = AcquireLockRecord(name, maxValidFor, machineName, threadId)) != null, timeout); + + if (acquired) { + Logger.Debug("Successfully acquired a lock named '{0}'.", name); + return threadId != null + ? DistributedLock.ForThread(this, name, machineName, threadId.Value, record.Id.ToString()) + : DistributedLock.ForMachine(this, name, machineName, record.Id.ToString()); + } } catch (Exception ex) { - Logger.Error(ex, "An error occurred while trying to acquire a lock."); + Logger.Error(ex, "Error while trying to acquire a lock named '{0}'.", name); throw; } - finally { - childLifetimeScope.Dispose(); + + return null; + } + + private DistributedLockRecord AcquireLockRecord(string name, TimeSpan maxValidFor, string machineName, int? threadId) { + lock (_transactionManagerLock) { + var childLifetimeScope = CreateChildLifetimeScope(name); + + try { + var transactionManager = childLifetimeScope.Resolve(); + transactionManager.RequireNew(IsolationLevel.ReadCommitted); + + // This way we can create a nested transaction scope instead of having the unwanted effect + // of manipulating the transaction of the caller. + var repository = childLifetimeScope.Resolve>(); + + // Find an existing, active lock, if any. + var record = repository.Table.FirstOrDefault(x => x.Name == name && x.ValidUntilUtc >= _clock.UtcNow && x.Count > 0); + + // The current owner name (based on machine name and current thread ID). + var canAcquireLock = false; + + // Check if there's already an active lock. + if (record != null) { + // Check if the machine name assigned to the lock is the one trying to acquire it. + if (record.MachineName == machineName) { + if(record.ThreadId != threadId) + throw new InvalidOperationException( + threadId == null + ? "An attempt to acquire a lock for a machine was detected while the requested lock is already assigned to a specific thread." + : "An attempt to acquire a lock for a thread was detected while the requested lock is already assigned to a machine."); + + record.Count++; + canAcquireLock = true; + } + } + else { + // No one has an active lock yet, so good to go. + record = new DistributedLockRecord { + Name = name, + MachineName = machineName, + ThreadId = threadId, + Count = 1, + CreatedUtc = _clock.UtcNow, + ValidUntilUtc = _clock.UtcNow + maxValidFor + }; + repository.Create(record); + canAcquireLock = true; + } + + if (!canAcquireLock) + return null; + + return record; + } + catch (Exception ex) { + Logger.Error(ex, "An error occurred while trying to acquire a lock."); + throw; + } + finally { + childLifetimeScope.Dispose(); + } } } - private string GetOwnerName() { - return String.Format("{0}_{1}", _machineNameProvider.GetMachineName(), Thread.CurrentThread.ManagedThreadId); + /// + /// Executes the specified function until it returns true, for the specified amount of time, or indefinitely if no timeout was given. + /// + /// The function to repeatedly execute until it returns true. + /// The amount of time to retry executing the function. If null is specified, the specified function is executed indefinitely until it returns true. + /// Returns true if the specified function returned true within the specified timeout, false otherwise. + private bool Poll(Func pollFunc, TimeSpan? timeout) { + var waitedTime = TimeSpan.Zero; + var waitTime = TimeSpan.FromMilliseconds(timeout.GetValueOrDefault().TotalMilliseconds / 10); + bool acquired; + + while (!(acquired = pollFunc()) && (timeout == null || waitedTime < timeout.Value)) { + Task.Delay(waitTime).ContinueWith(t => { + waitedTime += waitTime; + }).Wait(); + } + + return acquired; + } + + private string GetMachineName() { + return _machineNameProvider.GetMachineName(); + } + + private int GetThreadId() { + return _threadProvider.GetCurrentThreadId(); } private ILifetimeScope CreateChildLifetimeScope(string name) { diff --git a/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs b/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs deleted file mode 100644 index e38fe5da2..000000000 --- a/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs +++ /dev/null @@ -1,11 +0,0 @@ -using System; - -namespace Orchard.Tasks.Locking.Services { - /// - /// Represents a distributed lock. - /// - public interface IDistributedLock : IDisposable { - int Id { get; } - string Name { get; } - } -} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs index 1ba6d9060..7755ad598 100644 --- a/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs @@ -6,27 +6,94 @@ namespace Orchard.Tasks.Locking.Services { /// public interface IDistributedLockService : ISingletonDependency { /// - /// Tries to acquire a lock on the specified name. + /// Tries to acquire a lock on the specified name for the current machine. /// /// The name to use for the lock. - /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. - /// The amount of time to wait for the lock to be acquired before timing out. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. + /// The amount of time to wait for the lock to be acquired before timing out. A null value will cause the method to return immedieately if no lock could be acquired. /// The acquired lock. /// Returns true if a lock was successfully acquired, false otherwise. - bool TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout, out IDistributedLock @lock); + bool TryAcquireLockForMachine(string name, TimeSpan maxValidFor, TimeSpan? timeout, out DistributedLock @lock); /// - /// Acquires a lock with the specified parameters. + /// Acquires a lock with the specified parameters for the current machine. /// /// The name to use for the lock. /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. - /// The amount of time to wait for the lock to be acquired before timing out. - /// Returns a lock if one was successfully acquired, null otherwise. - IDistributedLock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout); + /// The amount of time to wait for the lock to be acquired before timing out. A null value will cause the method to block indefinitely until a lock can be acquired. + /// Returns a lock. + /// Throws a TimeoutException if no lock could be acquired in time. + DistributedLock AcquireLockForMachine(string name, TimeSpan maxLifetime, TimeSpan? timeout); + + /// + /// Tries to acquire a lock on the specified name for the current thread. + /// + /// The name to use for the lock. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. + /// The amount of time to wait for the lock to be acquired before timing out. A null value will cause the method to return immedieately if no lock could be acquired. + /// The acquired lock. + /// Returns true if a lock was successfully acquired, false otherwise. + bool TryAcquireLockForThread(string name, TimeSpan maxValidFor, TimeSpan? timeout, out DistributedLock @lock); + + /// + /// Acquires a lock with the specified parameters for the current thread. + /// + /// The name to use for the lock. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. + /// The amount of time to wait for the lock to be acquired before timing out. A null value will cause the method to block indefinitely until a lock can be acquired. + /// Returns a lock. + /// Throws a TimeoutException if no lock could be acquired in time. + DistributedLock AcquireLockForThread(string name, TimeSpan maxLifetime, TimeSpan? timeout); /// /// Disposes the specified lock. /// - void DisposeLock(IDistributedLock @lock); + void ReleaseLock(DistributedLock @lock); + } + + public static class DistributedLockServiceExtensions { + /// + /// Tries to acquire a lock on the specified name for the current machine. + /// + /// The name to use for the lock. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. + /// The acquired lock. + /// Returns true if a lock was successfully acquired, false otherwise. + public static bool TryAcquireLockForMachine(this IDistributedLockService service, string name, TimeSpan maxValidFor, out DistributedLock @lock) { + return service.TryAcquireLockForMachine(name, maxValidFor, null, out @lock); + } + + /// + /// Acquires a lock with the specified parameters for the current machine. + /// + /// The name to use for the lock. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. + /// Returns a lock. + /// Throws a TimeoutException if no lock could be acquired in time. + public static DistributedLock AcquireLockForMachine(this IDistributedLockService service, string name, TimeSpan maxLifetime) { + return service.AcquireLockForMachine(name, maxLifetime, null); + } + + /// + /// Tries to acquire a lock on the specified name for the current thread. + /// + /// The name to use for the lock. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. + /// The acquired lock. + /// Returns true if a lock was successfully acquired, false otherwise. + public static bool TryAcquireLockForThread(this IDistributedLockService service, string name, TimeSpan maxValidFor, out DistributedLock @lock) { + return service.TryAcquireLockForThread(name, maxValidFor, null, out @lock); + } + + /// + /// Acquires a lock with the specified parameters for the current thread. + /// + /// The name to use for the lock. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. + /// Returns a lock. + /// Throws a TimeoutException if no lock could be acquired in time. + public static DistributedLock AcquireLockForThread(this IDistributedLockService service, string name, TimeSpan maxLifetime) { + return service.AcquireLockForThread(name, maxLifetime, null); + } } } \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Services/Lock.cs b/src/Orchard/Tasks/Locking/Services/Lock.cs deleted file mode 100644 index 21a2eca42..000000000 --- a/src/Orchard/Tasks/Locking/Services/Lock.cs +++ /dev/null @@ -1,31 +0,0 @@ -using Orchard.Logging; - -namespace Orchard.Tasks.Locking.Services { - - /// - /// Provides a database driven implementation of - /// - public class Lock : IDistributedLock { - private readonly IDistributedLockService _distributedLockService; - public string Name { get; set; } - private bool _isDisposed; - - public Lock(IDistributedLockService distributedLockService, string name, int id) { - _distributedLockService = distributedLockService; - Name = name; - Id = id; - } - - public ILogger Logger { get; set; } - public int Id { get; set; } - - // This will be called at least and at the latest by the IoC container when the request ends. - public void Dispose() { - if (!_isDisposed) { - _isDisposed = true; - - _distributedLockService.DisposeLock(this); - } - } - } -} \ No newline at end of file