diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index 7828c23fe..092585fe6 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -1,9 +1,7 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Threading.Tasks; using Autofac; -using NHibernate.Linq; using NUnit.Framework; using Orchard.Data; using Orchard.Environment; @@ -189,7 +187,37 @@ namespace Orchard.Tests.Tasks { Assert.That(acquired, Is.False); } - private DistributedLockRecord CreateLockRecord(int count, DateTime createdUtc, DateTime validUntilUtc, string machineName, int? threadId) { + [Test] + public void ActiveLockWithUndefinedValidUntilNeverExpires() { + CreateNonExpiredActiveLockThatNeverExpires("Other Machine", null); + _clock.Advance(DateTime.MaxValue - _clock.UtcNow); // Fast forward to the End of Time. + DistributedLock @lock; + var acquired = _distributedLockService.TryAcquireLockForThread(LockName, TimeSpan.FromMinutes(1), null, out @lock); + + Assert.That(acquired, Is.False); + } + + [Test] + public void ActiveLockWithUndefinedValidUntilNeverExpiresUntilReleased() { + DistributedLock @lock; + + // Create a never expiring lock. + _machineNameProvider.MachineName = "Orchard Test Machine 1"; + var attempt1 = _distributedLockService.TryAcquireLockForThread(LockName, maxValidFor: null, timeout: null, @lock: out @lock); + + // Release the lock. + _distributedLockService.ReleaseLock(@lock); + + // Acquire the lock from another machine. + _machineNameProvider.MachineName = "Orchard Test Machine 2"; + var attempt2 = _distributedLockService.TryAcquireLockForThread(LockName, maxValidFor: null, timeout: null, @lock: out @lock); + + // Validate the results. + Assert.That(attempt1, Is.True); + Assert.That(attempt2, Is.True); + } + + private DistributedLockRecord CreateLockRecord(int count, DateTime createdUtc, DateTime? validUntilUtc, string machineName, int? threadId) { var record = new DistributedLockRecord { Name = LockName, Count = count, @@ -219,6 +247,11 @@ namespace Orchard.Tests.Tasks { return CreateLockRecord(1, now, now - TimeSpan.FromHours(1), machineName, threadId); } + private DistributedLockRecord CreateNonExpiredActiveLockThatNeverExpires(string machineName, int? threadId) { + var now = _clock.UtcNow; + return CreateLockRecord(1, now, null, machineName, threadId); + } + private string GetMachineName() { return _machineNameProvider.GetMachineName(); } diff --git a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs index 952e4a554..ce27736a6 100644 --- a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs +++ b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs @@ -12,7 +12,7 @@ namespace Orchard.Tasks.Locking.Migrations { .Column("ThreadId", column => column.Nullable()) .Column("Count") .Column("CreatedUtc") - .Column("ValidUntilUtc")); + .Column("ValidUntilUtc", column => column.Nullable())); SchemaBuilder.AlterTable("DistributedLockRecord", table => { table.CreateIndex("IDX_DistributedLockRecord_Name_ValidUntilUtc_Count", "Name", "ValidUntilUtc", "Count"); diff --git a/src/Orchard/Tasks/Locking/Records/DistributedLockRecord.cs b/src/Orchard/Tasks/Locking/Records/DistributedLockRecord.cs index 586d1ddd7..ec8fbff96 100644 --- a/src/Orchard/Tasks/Locking/Records/DistributedLockRecord.cs +++ b/src/Orchard/Tasks/Locking/Records/DistributedLockRecord.cs @@ -8,6 +8,6 @@ namespace Orchard.Tasks.Locking.Records { public virtual int? ThreadId { get; set; } public virtual int Count { get; set; } public virtual DateTime CreatedUtc { get; set; } - public virtual DateTime ValidUntilUtc { get; set; } + public virtual DateTime? ValidUntilUtc { get; set; } } } \ 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 abf4f462f..f6ef2f94c 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -25,19 +25,19 @@ namespace Orchard.Tasks.Locking.Services { _threadProvider = threadProvider; } - public bool TryAcquireLockForMachine(string name, TimeSpan maxValidFor, TimeSpan? timeout, out DistributedLock @lock) { + public bool TryAcquireLockForMachine(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out DistributedLock @lock) { return TryAcquireLock(name, maxValidFor, timeout, GetMachineName(), null, out @lock); } - public DistributedLock AcquireLockForMachine(string name, TimeSpan maxValidFor, TimeSpan? timeout) { + 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) { + 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) { + public DistributedLock AcquireLockForThread(string name, TimeSpan? maxValidFor, TimeSpan? timeout) { return AcquireLock(name, maxValidFor, timeout, GetMachineName(), GetThreadId()); } @@ -68,7 +68,7 @@ namespace Orchard.Tasks.Locking.Services { } } - private bool TryAcquireLock(string name, TimeSpan maxValidFor, TimeSpan? timeout, string machineName, int? threadId, out DistributedLock @lock) { + private bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, string machineName, int? threadId, out DistributedLock @lock) { @lock = AcquireLockInternal(name, maxValidFor, machineName, threadId, timeout ?? TimeSpan.Zero); if (@lock != null) return true; @@ -76,7 +76,7 @@ namespace Orchard.Tasks.Locking.Services { return false; } - private DistributedLock AcquireLock(string name, TimeSpan maxValidFor, TimeSpan? timeout, string machineName, int? threadId) { + 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; @@ -84,7 +84,7 @@ namespace Orchard.Tasks.Locking.Services { throw new TimeoutException(String.Format("Failed to acquire a lock named '{0}' within the specified timeout ('{1}').", name, timeout)); } - private DistributedLock AcquireLockInternal(string name, TimeSpan maxValidFor, string machineName, int? threadId, TimeSpan? timeout = null) { + 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); @@ -109,65 +109,63 @@ namespace Orchard.Tasks.Locking.Services { return null; } - private DistributedLockRecord AcquireLockRecord(string name, TimeSpan maxValidFor, string machineName, int? threadId) { - //lock (_transactionManagerLock) { - var childLifetimeScope = CreateChildLifetimeScope(name); + private DistributedLockRecord AcquireLockRecord(string name, TimeSpan? maxValidFor, string machineName, int? threadId) { + var childLifetimeScope = CreateChildLifetimeScope(name); - try { - var transactionManager = childLifetimeScope.Resolve(); - transactionManager.RequireNew(IsolationLevel.ReadCommitted); + 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>(); + // 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); + // Find an existing, active lock, if any. + var record = repository.Table.FirstOrDefault(x => x.Name == name && (x.ValidUntilUtc == null || x.ValidUntilUtc >= _clock.UtcNow) && x.Count > 0); - // The current owner name (based on machine name and current thread ID). - var canAcquireLock = false; + // 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."); + // 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); + 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 = maxValidFor != null ? _clock.UtcNow + maxValidFor : null + }; + repository.Create(record); + canAcquireLock = true; + } - if (!canAcquireLock) - return null; + 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(); - } - //} + return record; + } + catch (Exception ex) { + Logger.Error(ex, "An error occurred while trying to acquire a lock."); + throw; + } + finally { + childLifetimeScope.Dispose(); + } } /// diff --git a/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs index 7755ad598..31818f200 100644 --- a/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs @@ -13,37 +13,37 @@ namespace Orchard.Tasks.Locking.Services { /// 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 TryAcquireLockForMachine(string name, TimeSpan maxValidFor, TimeSpan? timeout, out DistributedLock @lock); + bool TryAcquireLockForMachine(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out DistributedLock @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. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If null is specified, the lock never expires until it's released by the owner. /// 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); + DistributedLock AcquireLockForMachine(string name, TimeSpan? maxValidFor, 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 maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If null is specified, the lock never expires until it's released by the owner. /// 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); + 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 maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If null is specified, the lock never expires until it's released by the owner. /// 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); + DistributedLock AcquireLockForThread(string name, TimeSpan? maxValidFor, TimeSpan? timeout); /// /// Disposes the specified lock. @@ -56,44 +56,73 @@ namespace Orchard.Tasks.Locking.Services { /// 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 maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If null is specified, the lock never expires until it's released by the owner. /// 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) { + public static bool TryAcquireLockForMachine(this IDistributedLockService service, string name, TimeSpan? maxValidFor, out DistributedLock @lock) { return service.TryAcquireLockForMachine(name, maxValidFor, null, out @lock); } + /// + /// Tries to acquire a lock on the specified name for the current machine. + /// + /// The name to use for the lock. + /// The acquired lock. + /// Returns true if a lock was successfully acquired, false otherwise. + public static bool TryAcquireLockForMachine(this IDistributedLockService service, string name, out DistributedLock @lock) { + return service.TryAcquireLockForMachine(name, null, 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. + /// The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If null is specified, the lock never expires until it's released by the owner. /// 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); + public static DistributedLock AcquireLockForMachine(this IDistributedLockService service, string name, TimeSpan? maxValidFor) { + return service.AcquireLockForMachine(name, maxValidFor, null); + } + + /// + /// Acquires a lock with the specified parameters for the current machine. + /// + /// The name to use for 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) { + return service.AcquireLockForMachine(name, null, 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 maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If null is specified, the lock never expires until it's released by the owner. /// 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) { + public static bool TryAcquireLockForThread(this IDistributedLockService service, string name, TimeSpan? maxValidFor, out DistributedLock @lock) { return service.TryAcquireLockForThread(name, maxValidFor, null, out @lock); } + /// + /// Tries to acquire a lock on the specified name for the current thread. + /// + /// The name to use for the lock. + /// The acquired lock. + /// Returns true if a lock was successfully acquired, false otherwise. + public static bool TryAcquireLockForThread(this IDistributedLockService service, string name, out DistributedLock @lock) { + return service.TryAcquireLockForThread(name, null, 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); + public static DistributedLock AcquireLockForThread(this IDistributedLockService service, string name) { + return service.AcquireLockForThread(name, null, null); } } } \ No newline at end of file