From 324709400f0889c743b2cef4411f8bc1fc362d1c Mon Sep 17 00:00:00 2001 From: Daniel Stolt Date: Sun, 6 Sep 2015 15:51:26 +0200 Subject: [PATCH] Cleaned up naming, formatting and comments in distributed locks. --- .../Tasks/DistributedLockServiceTests.cs | 4 +- .../Tasks/Locking/Services/DistributedLock.cs | 14 +-- .../Services/DistributedLockService.cs | 84 ++++++------- .../Locking/Services/IDistributedLock.cs | 4 +- .../Services/IDistributedLockService.cs | 111 ++++++++++-------- 5 files changed, 110 insertions(+), 107 deletions(-) diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index 71efdc6d2..d04507b52 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -223,14 +223,14 @@ namespace Orchard.Tests.Tasks { // Create a never expiring lock. _machineNameProvider.MachineName = "Orchard Test Machine 1"; - var attempt1 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, l: out @lock); + var attempt1 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, dLock: out @lock); // Release the lock. _distributedLockService.ReleaseDistributedLock((DistributedLock)@lock); // Acquire the lock from another machine. _machineNameProvider.MachineName = "Orchard Test Machine 2"; - var attempt2 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, l: out @lock); + var attempt2 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, dLock: out @lock); // Validate the results. Assert.That(attempt1, Is.True); diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLock.cs b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs index a15da99ca..73345b795 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLock.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs @@ -9,19 +9,19 @@ namespace Orchard.Tasks.Locking.Services { private string _name; private int _count; - public string Name { - get { - return _name; - } - } - public DistributedLock(DistributedLockService service, string name) { _service = service; _name = name; _count = 1; } - public void IncreaseReferenceCount() { + public string Name { + get { + return _name; + } + } + + public void Increment() { _count++; } diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs index 34ab4332f..51d24488c 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -16,13 +16,12 @@ using Orchard.Tasks.Locking.Records; namespace Orchard.Tasks.Locking.Services { public class DistributedLockService : Component, IDistributedLockService { - private readonly IClock _clock; - private readonly IMachineNameProvider _machineNameProvider; - private readonly ShellSettings _shellSettings; - private readonly ILifetimeScope _lifetimeScope; - private readonly ConcurrentDictionary _locks = new ConcurrentDictionary(); - public bool DisableMonitorLock { get; set; } + private readonly IMachineNameProvider _machineNameProvider; + private readonly ILifetimeScope _lifetimeScope; + private readonly IClock _clock; + private readonly ShellSettings _shellSettings; + private readonly ConcurrentDictionary _locks; public DistributedLockService( IMachineNameProvider machineNameProvider, @@ -31,62 +30,58 @@ namespace Orchard.Tasks.Locking.Services { ShellSettings shellSettings) { _clock = clock; _lifetimeScope = lifetimeScope; - _shellSettings = shellSettings; _machineNameProvider = machineNameProvider; - } + _locks = new ConcurrentDictionary(); + } - public bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock l) { + public bool DisableMonitorLock { get; set; } + + public bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock dLock) { try { - l = AcquireLock(name, maxValidFor, timeout); - return l != null; + dLock = AcquireLock(name, maxValidFor, timeout); + return dLock != null; } catch { - l = null; + dLock = null; return false; } } public IDistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout) { - DistributedLock l = null; + DistributedLock dLock = null; try { + var acquired = Poll(() => (dLock = AcquireLockInternal(name, maxValidFor)) != null, timeout); - var acquired = Poll(() => (l = AcquireLockInternal(name, maxValidFor)) != null, timeout); - - if (acquired) { - Logger.Debug("Successfully acquired a lock named '{0}'.", name); - } - else { - Logger.Debug("Failed to acquire a lock named '{0}'.", name); - } + if (acquired) + Logger.Debug("Successfully acquired lock '{0}'.", name); + else + Logger.Debug("Failed to acquire lock '{0}' within the specified timeout.", name); } catch (Exception ex) { - Logger.Error(ex, "Error while trying to acquire a lock named '{0}'.", name); + Logger.Error(ex, "Error while trying to acquire lock '{0}'.", name); throw; } - if (l == null && timeout != null) { - throw new TimeoutException(String.Format("Failed to acquire a lock named '{0}' within the specified timeout ('{1}').", name, timeout)); - } + if (dLock == null && timeout != null) + throw new TimeoutException(String.Format("Failed to acquire lock '{0}' within the specified timeout ({1}).", name, timeout)); - return l; + return dLock; } - public void ReleaseDistributedLock(DistributedLock l) { - + public void ReleaseDistributedLock(DistributedLock dLock) { try { - var record = GetDistributedLockRecordByName(l.Name); - - if (record == null) { - throw new OrchardException(T("No lock record could be found for the specified lock to be released.")); - } + var record = GetDistributedLockRecordByName(dLock.Name); + if (record == null) + throw new OrchardException(T("No lock record could be found in the database for lock '{0}'.", dLock.Name)); TryCommitNewTransaction(repository => repository.Delete(record)); } 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}'.", l.Name); + if (ex.IsFatal()) + throw; + Logger.Error(ex, "An non-fatal error occurred while releasing lock '{0}'.", dLock.Name); } } @@ -105,8 +100,7 @@ namespace Orchard.Tasks.Locking.Services { DistributedLockRecord result = null; TryCommitNewTransaction(repository => { result = repository.Table.FirstOrDefault(x => - x.Name == name && - (x.ValidUntilUtc == null || x.ValidUntilUtc >= _clock.UtcNow) + x.Name == name && (x.ValidUntilUtc == null || x.ValidUntilUtc >= _clock.UtcNow) ); }); @@ -117,19 +111,18 @@ namespace Orchard.Tasks.Locking.Services { try { name = GetTenantLockName(name); - if (!DisableMonitorLock && !Monitor.TryEnter(String.Intern(name))) { + if (!DisableMonitorLock && !Monitor.TryEnter(String.Intern(name))) return null; - } DistributedLock dLock = null; - // Returns the existing lock in case of reentrancy. + // Return the existing lock in case of reentrancy. if(!DisableMonitorLock && _locks.TryGetValue(name, out dLock)) { - dLock.IncreaseReferenceCount(); + dLock.Increment(); return dLock; } - // Find an existing, active lock, if any. + // Find an existing active lock, if any. var record = GetValidDistributedLockRecordByName(name); // The current owner name (based on machine name). @@ -151,8 +144,7 @@ namespace Orchard.Tasks.Locking.Services { ValidUntilUtc = maxValidFor != null ? _clock.UtcNow + maxValidFor : null }; - - canAcquireLock = TryCommitNewTransaction( repository => { + canAcquireLock = TryCommitNewTransaction(repository => { repository.Create(record); }); } @@ -201,9 +193,8 @@ namespace Orchard.Tasks.Locking.Services { } private bool TryCommitNewTransaction(Action> action) { - if (action == null) { + if (action == null) throw new ArgumentNullException(); - } try { using (var childLifetimeScope = _lifetimeScope.BeginLifetimeScope()) { @@ -218,7 +209,6 @@ namespace Orchard.Tasks.Locking.Services { catch { return false; } - } } } \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs b/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs index 6151554ff..a687728e1 100644 --- a/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs @@ -3,7 +3,9 @@ namespace Orchard.Tasks.Locking.Services { /// - /// Represents a distributed lock. /> + /// Represents a distributed lock returned by IDistributedLockService. The owner of the + /// lock should call Dispose() on an instance of this interface to release the distributed + /// lock. /// public interface IDistributedLock : IDisposable { string Name { get; } diff --git a/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs index c638a7aca..7de40e64f 100644 --- a/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs @@ -1,71 +1,82 @@ using System; namespace Orchard.Tasks.Locking.Services { - /// - /// Provides distributed locking functionality. - /// - public interface IDistributedLockService : IDependency { - /// - /// Tries to acquire a distributed lock on a named resource for the current tenant. - /// - /// 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. 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. - /// The acquired lock. - /// Returns true if a lock was successfully acquired, false otherwise. - bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock @lock); + + /// + /// Provides functionality to acquire and release tenant-wide locks which are + /// distributed across all instances in a web farm. + /// + /// + /// Distributed locks can be used to protect critical sections that should only ever + /// be executed by a single thread of execution across a whole web farm. The distributed + /// locks returned by this service are reentrant, i.e. the owner of a lock can reacquire it + /// multiple times, and must also release it (dispose of it) as many times as it was + /// acquired for the lock to be released. + /// + public interface IDistributedLockService : IDependency { /// - /// Acquires a distributed lock on a named resource for the current tenant. + /// Tries to acquire a named distributed lock within the current tenant. /// - /// 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. 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. - IDistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout); + /// The name of the lock to acquire. + /// The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If null is specified, the lock never automatically expires. + /// The amount of time to wait for the lock to be acquired. Passing TimeSpan.Zero will cause the method to return immediately. Passing null will cause the method to block indefinitely until a lock can be acquired. + /// This out parameter will be assigned the acquired lock if successful. + /// true if the lock was successfully acquired, otherwise false. + bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock @lock); + + /// + /// Acquires a named distributed lock within the current tenant or throws if the lock cannot be acquired. + /// + /// The name of the lock to acquire. + /// The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If null is specified, the lock never automatically expires. + /// The amount of time to wait for the lock to be acquired. Passing TimeSpan.Zero will cause the method to return immediately. Passing null will cause the method to block indefinitely until a lock can be acquired. + /// The acquired lock. + /// This method throws a TimeoutException if the lock could not be acquired within the specified timeout period. + IDistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout); } 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. 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 TryAcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor, out IDistributedLock @lock) { + + /// + /// Tries to immediately acquire a named distributed lock with a given expiration time within the current tenant. + /// + /// The name of the lock to acquire. + /// The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If null is specified, the lock never automatically expires. + /// This out parameter will be assigned the acquired lock if successful. + /// true if the lock could be immediately acquired, otherwise false. + public static bool TryAcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor, out IDistributedLock @lock) { return service.TryAcquireLock(name, maxValidFor, TimeSpan.Zero, 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 TryAcquireLock(this IDistributedLockService service, string name, out IDistributedLock @lock) { + /// + /// Tries to immediately acquire a named distributed lock with no expiration time within the current tenant. + /// + /// The name of the lock to acquire. + /// This out parameter will be assigned the acquired lock if successful. + /// true if the lock could be immediately acquired, otherwise false. + public static bool TryAcquireLock(this IDistributedLockService service, string name, out IDistributedLock @lock) { return service.TryAcquireLock(name, null, TimeSpan.Zero, 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. 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 IDistributedLock AcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor) { + /// + /// Acquires a named distributed lock with a given expiration time within the current tenant. + /// + /// The name of the lock to acquire. + /// The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If null is specified, the lock never automatically expires. + /// The acquired lock. + /// This method blocks indefinitely until the lock can be acquired. + public static IDistributedLock AcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor) { return service.AcquireLock(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 IDistributedLock AcquireLock(this IDistributedLockService service, string name) { + /// + /// Acquires a named distributed lock with no expiration time within the current tenant. + /// + /// The name to use for the lock. + /// The acquired lock. + /// This method blocks indefinitely until the lock can be acquired. + public static IDistributedLock AcquireLock(this IDistributedLockService service, string name) { return service.AcquireLock(name, null, null); } }