diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index d04507b52..ff32e9fbe 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -87,7 +87,6 @@ namespace Orchard.Tests.Tasks { [Test] public void TryAcquiringLockTwiceFails() { - _distributedLockService.DisableMonitorLock = true; IDistributedLock @lock; _machineNameProvider.MachineName = "Orchard Test Machine 1"; var attempt1 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), out @lock); @@ -152,11 +151,10 @@ namespace Orchard.Tests.Tasks { [Test] public void MultipleAcquisitionsFromDifferentMachinesShouldFail() { IDistributedLock @lock; - _distributedLockService.DisableMonitorLock = true; _machineNameProvider.MachineName = "Orchard Test Machine 1"; - var attempt1 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), out @lock); + var attempt1 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromMinutes(60), out @lock); _machineNameProvider.MachineName = "Orchard Test Machine 2"; - var attempt2 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), out @lock); + var attempt2 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromMinutes(60), out @lock); Assert.That(attempt1, Is.True); Assert.That(attempt2, Is.False); @@ -165,7 +163,6 @@ namespace Orchard.Tests.Tasks { [Test] public void MultipleAcquisitionsFromDifferentMachinesOnDifferentTenantShouldSucceed() { IDistributedLock @lock; - _distributedLockService.DisableMonitorLock = true; _machineNameProvider.MachineName = "Orchard Test Machine 1"; var attempt1 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), out @lock); _machineNameProvider.MachineName = "Orchard Test Machine 2"; @@ -194,8 +191,6 @@ namespace Orchard.Tests.Tasks { [Test] public void TryAcquireActiveLockWithNullTimeoutReturnsFalseImmediately() { - // Disable monitor locking to simulate concurrent requests - _distributedLockService.DisableMonitorLock = true; CreateNonExpiredActiveLock("Other Machine"); IDistributedLock @lock; @@ -206,8 +201,6 @@ namespace Orchard.Tests.Tasks { [Test] public void ActiveLockWithUndefinedValidUntilNeverExpires() { - // Disable monitor locking to simulate concurrent requests - _distributedLockService.DisableMonitorLock = true; CreateNonExpiredActiveLockThatNeverExpires("Other Machine"); _clock.Advance(DateTime.MaxValue - _clock.UtcNow); // Fast forward to the End of Time. @@ -224,9 +217,9 @@ namespace Orchard.Tests.Tasks { // Create a never expiring lock. _machineNameProvider.MachineName = "Orchard Test Machine 1"; var attempt1 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, dLock: out @lock); - + // Release the lock. - _distributedLockService.ReleaseDistributedLock((DistributedLock)@lock); + @lock.Dispose(); // Acquire the lock from another machine. _machineNameProvider.MachineName = "Orchard Test Machine 2"; @@ -239,7 +232,7 @@ namespace Orchard.Tests.Tasks { private DistributedLockRecord CreateLockRecord(DateTime createdUtc, DateTime? validUntilUtc, string machineName) { var record = new DistributedLockRecord { - Name = ShellSettings.DefaultName + ":" + LockName, + Name = String.Format("DistributedLock:{0}:{1}", ShellSettings.DefaultName, LockName), CreatedUtc = createdUtc, ValidUntilUtc = validUntilUtc, MachineName = machineName, diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLock.cs b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs index 73345b795..2069c2181 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLock.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs @@ -1,36 +1,41 @@ using System; -using System.Threading; namespace Orchard.Tasks.Locking.Services { public class DistributedLock : IDistributedLock { - private DistributedLockService _service; - private string _name; + private readonly string _name; + private readonly string _internalName; + private readonly Action _releaseLockAction; private int _count; - public DistributedLock(DistributedLockService service, string name) { - _service = service; + internal DistributedLock(string name, string internalName, Action releaseLockAction) { _name = name; + _internalName = internalName; + _releaseLockAction = releaseLockAction; _count = 1; } - public string Name { + string IDistributedLock.Name { get { return _name; } } - public void Increment() { + internal string InternalName { + get { + return _internalName; + } + } + + internal void Increment() { _count++; } public void Dispose() { _count--; - if (_count == 0) { - Monitor.Exit(String.Intern(_name)); - _service.ReleaseDistributedLock(this); - } + if (_count == 0) + _releaseLockAction(); } } } \ 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 51d24488c..e7fabf36d 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -1,5 +1,5 @@ using System; -using System.Collections.Concurrent; +using System.Collections.Generic; using System.Data; using System.Linq; using System.Threading; @@ -19,9 +19,10 @@ namespace Orchard.Tasks.Locking.Services { private readonly IMachineNameProvider _machineNameProvider; private readonly ILifetimeScope _lifetimeScope; - private readonly IClock _clock; + private readonly IClock _clock; private readonly ShellSettings _shellSettings; - private readonly ConcurrentDictionary _locks; + private readonly Dictionary _locks; + private readonly TimeSpan _defaultRepeatInterval; public DistributedLockService( IMachineNameProvider machineNameProvider, @@ -32,183 +33,182 @@ namespace Orchard.Tasks.Locking.Services { _lifetimeScope = lifetimeScope; _shellSettings = shellSettings; _machineNameProvider = machineNameProvider; - _locks = new ConcurrentDictionary(); - } + _locks = new Dictionary(); + _defaultRepeatInterval = TimeSpan.FromMilliseconds(500); + } - public bool DisableMonitorLock { get; set; } - - public bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock dLock) { + public bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock dLock) { try { - dLock = AcquireLock(name, maxValidFor, timeout); - return dLock != null; + dLock = AcquireLockInternal(name, maxValidFor, timeout, throwOnTimeout: false); + + if (dLock != null) { + Logger.Debug("Successfully acquired lock '{0}'.", name); + return true; + } + + Logger.Warning("Failed to acquire lock '{0}' within the specified timeout ({1}).", name, timeout); } - catch { - dLock = null; - return false; + catch (Exception ex) { + Logger.Error(ex, "Error while trying to acquire lock '{0}'.", name); + // TODO: Is it correct to not throw here? Should we instead ONLY swallow TimeoutException? } + + dLock = null; + return false; } public IDistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout) { - DistributedLock dLock = null; - try { - var acquired = Poll(() => (dLock = AcquireLockInternal(name, maxValidFor)) != null, timeout); - - if (acquired) - Logger.Debug("Successfully acquired lock '{0}'.", name); - else - Logger.Debug("Failed to acquire lock '{0}' within the specified timeout.", name); + DistributedLock result = AcquireLockInternal(name, maxValidFor, timeout, throwOnTimeout: true); + Logger.Debug("Successfully acquired lock '{0}'.", name); + return result; } catch (Exception ex) { Logger.Error(ex, "Error while trying to acquire lock '{0}'.", name); throw; } - - if (dLock == null && timeout != null) - throw new TimeoutException(String.Format("Failed to acquire lock '{0}' within the specified timeout ({1}).", name, timeout)); - - return dLock; } - public void ReleaseDistributedLock(DistributedLock dLock) { - try { - 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)); + private DistributedLock AcquireLockInternal(string name, TimeSpan? maxValidFor, TimeSpan? timeout, bool throwOnTimeout) { + var internalName = GetInternalLockName(name); + var monitorTimeout = timeout.HasValue ? timeout.Value : TimeSpan.FromMilliseconds(-1); // -1 ms is .NET magic number for "infinite". + var monitorObj = String.Intern(String.Format("{0}:{1}", _machineNameProvider.GetMachineName(), internalName)); - TryCommitNewTransaction(repository => repository.Delete(record)); + if (!Monitor.TryEnter(monitorObj, monitorTimeout)) { + Logger.Debug("Could not enter local monitor for lock '{0}' within the specified timeout ({1}).", internalName, timeout); + + if (throwOnTimeout) + throw new TimeoutException(String.Format("Failed to acquire lock '{0}' within the specified timeout ({1}).", internalName, timeout)); + + return null; } - catch (Exception ex) { - if (ex.IsFatal()) - throw; - Logger.Error(ex, "An non-fatal error occurred while releasing lock '{0}'.", dLock.Name); - } - } - private DistributedLockRecord GetDistributedLockRecordByName(string name) { - DistributedLockRecord result = null; - TryCommitNewTransaction(repository => { - result = repository.Table.FirstOrDefault(x => - x.Name == name - ); - }); + Logger.Debug("Successfully entered local monitor for lock '{0}'.", internalName); - return result; - } - - private DistributedLockRecord GetValidDistributedLockRecordByName(string name) { - DistributedLockRecord result = null; - TryCommitNewTransaction(repository => { - result = repository.Table.FirstOrDefault(x => - x.Name == name && (x.ValidUntilUtc == null || x.ValidUntilUtc >= _clock.UtcNow) - ); - }); - - return result; - } - - private DistributedLock AcquireLockInternal(string name, TimeSpan? maxValidFor) { try { - name = GetTenantLockName(name); - - if (!DisableMonitorLock && !Monitor.TryEnter(String.Intern(name))) - return null; - DistributedLock dLock = null; - - // Return the existing lock in case of reentrancy. - if(!DisableMonitorLock && _locks.TryGetValue(name, out dLock)) { + + // If there's already a distributed lock object in our dictionary, that means + // this acquisition is a reentrance. Use the existing lock object from the + // dictionary but increment its count. + if (_locks.TryGetValue(monitorObj, out dLock)) { + Logger.Debug("Current thread is re-entering lock '{0}'; incrementing count.", internalName); dLock.Increment(); - return dLock; - } - - // Find an existing active lock, if any. - var record = GetValidDistributedLockRecordByName(name); - - // The current owner name (based on machine name). - 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 == _machineNameProvider.GetMachineName()) { - canAcquireLock = true; - } } else { - // No one has an active lock yet, so good to go. - record = new DistributedLockRecord { - Name = name, - MachineName = _machineNameProvider.GetMachineName(), - CreatedUtc = _clock.UtcNow, - ValidUntilUtc = maxValidFor != null ? _clock.UtcNow + maxValidFor : null - }; + // No distributed lock object existed in our dictionary. Try to take ownership + // of database record until timeout expires, and if successful create a distributed + // lock object and add it to our dictionary. + var success = RepeatUntilTimeout(timeout, _defaultRepeatInterval, () => { + if (EnsureDistributedLockRecord(internalName, maxValidFor)) { + Logger.Debug("Record for lock '{0}' already owned by current machine or was successfully created; creating lock object.", internalName); - canAcquireLock = TryCommitNewTransaction(repository => { - repository.Create(record); + dLock = new DistributedLock(name, internalName, releaseLockAction: () => { + Monitor.Exit(monitorObj); + DeleteDistributedLockRecord(internalName); + }); + + _locks.Add(monitorObj, dLock); + return true; + } + + return false; }); - } - if (!canAcquireLock) { - return null; - } + if (!success) { + Logger.Debug("Record for lock '{0}' could not be created for current machine within the specified timeout ({1}).", internalName, timeout); - dLock = new DistributedLock(this, name); - if (!DisableMonitorLock) { - _locks.TryAdd(name, dLock); + if (throwOnTimeout) + throw new TimeoutException(String.Format("Failed to acquire lock '{0}' within the specified timeout ({1}).", internalName, timeout)); + + return null; + } } return dLock; } catch (Exception ex) { - Monitor.Exit(String.Intern(name)); + Monitor.Exit(monitorObj); - Logger.Error(ex, "An error occurred while trying to acquire a lock."); + Logger.Error(ex, "An error occurred while trying to acquire lock '{0}'.", internalName); throw; - } + } } - private string GetTenantLockName(string name) { - return _shellSettings.Name + ":" + name; + private bool EnsureDistributedLockRecord(string internalName, TimeSpan? maxValidFor) { + var localMachineName = _machineNameProvider.GetMachineName(); + var hasLockRecord = false; + + ExecuteOnSeparateTransaction(repository => { + // Try to find a valid lock record in the database. + var record = repository.Table.FirstOrDefault(x => x.Name == internalName && (x.ValidUntilUtc == null || x.ValidUntilUtc >= _clock.UtcNow)); + if (record == null) { + // No record existed, so we're good to create a new one. + Logger.Debug("No valid record was found for lock '{0}'; creating a new record.", internalName); + + repository.Create(new DistributedLockRecord { + Name = internalName, + MachineName = localMachineName, + CreatedUtc = _clock.UtcNow, + ValidUntilUtc = maxValidFor.HasValue ? _clock.UtcNow + maxValidFor.Value : default(DateTime?) + }); + + hasLockRecord = true; + } + else if (record.MachineName == localMachineName) { + // Existing lock was for correct machine name => lock record exists. + Logger.Debug("Found a valid record for lock '{0}' and current local machine name '{1}'.", internalName, localMachineName); + hasLockRecord = true; + } + }); + + return hasLockRecord; } - /// - /// Executes the specified function until it returns true, for the specified amount of time, or indefinitely if no timeout was given. - /// - /// The operation 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 operation, TimeSpan? timeout) { + private void DeleteDistributedLockRecord(string internalName) { + try { + ExecuteOnSeparateTransaction(repository => { + var record = repository.Table.FirstOrDefault(x => x.Name == internalName); + if (record == null) + throw new Exception(String.Format("No record could be found in the database for lock '{0}'.", internalName)); + repository.Delete(record); + Logger.Debug("Successfully deleted record for lock '{0}'.", internalName); + }); + } + catch (Exception ex) { + if (ex.IsFatal()) + throw; + Logger.Warning(ex, "An error occurred while deleting record for lock '{0}'.", internalName); + } + } + + private bool RepeatUntilTimeout(TimeSpan? timeout, TimeSpan repeatInterval, Func action) { + bool success; + var waitedTime = TimeSpan.Zero; - var waitTime = TimeSpan.FromMilliseconds(timeout.GetValueOrDefault().TotalMilliseconds / 10); - bool acquired; - - while (!(acquired = operation()) && (timeout == null || waitedTime < timeout.Value)) { - Task.Delay(waitTime).ContinueWith(t => { - waitedTime += waitTime; - }).Wait(); + while (!(success = action()) && (!timeout.HasValue || waitedTime < timeout.Value)) { + Task.Delay(repeatInterval).Wait(); + waitedTime += repeatInterval; } - return acquired; + return success; } - private bool TryCommitNewTransaction(Action> action) { + private void ExecuteOnSeparateTransaction(Action> action) { if (action == null) throw new ArgumentNullException(); - try { - using (var childLifetimeScope = _lifetimeScope.BeginLifetimeScope()) { - var repository = childLifetimeScope.Resolve>(); - var transactionManager = childLifetimeScope.Resolve(); - transactionManager.RequireNew(IsolationLevel.ReadCommitted); - action(repository); - } + using (var childLifetimeScope = _lifetimeScope.BeginLifetimeScope()) { + var repository = childLifetimeScope.Resolve>(); + var transactionManager = childLifetimeScope.Resolve(); + transactionManager.RequireNew(IsolationLevel.ReadCommitted); + action(repository); + } + } - return true; - } - catch { - return false; - } + private string GetInternalLockName(string name) { + // Prefix the requested lock name by a constant and the tenant name. + return String.Format("DistributedLock:{0}:{1}", _shellSettings.Name, name); } } } \ No newline at end of file