From f363ac2b5fedfa7b9267a446755e6adfee8fd9aa Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 11:56:00 +0100 Subject: [PATCH 01/28] Initial work on a distributed locking service. --- .../Modules/Orchard.TaskLease/Migrations.cs | 11 +- .../Models/DatabaseLockRecord.cs | 9 ++ .../Orchard.TaskLease.csproj | 5 + .../Services/DatabaseLock.cs | 100 ++++++++++++++++++ .../Services/ITaskLeaseService.cs | 1 + .../Services/TaskLeaseService.cs | 1 + src/Orchard/Orchard.Framework.csproj | 4 + src/Orchard/Tasks/Locking/DefaultLock.cs | 14 +++ src/Orchard/Tasks/Locking/ILock.cs | 16 +++ src/Orchard/Tasks/Locking/ILockService.cs | 27 +++++ src/Orchard/Tasks/Locking/LockService.cs | 48 +++++++++ 11 files changed, 234 insertions(+), 2 deletions(-) create mode 100644 src/Orchard.Web/Modules/Orchard.TaskLease/Models/DatabaseLockRecord.cs create mode 100644 src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs create mode 100644 src/Orchard/Tasks/Locking/DefaultLock.cs create mode 100644 src/Orchard/Tasks/Locking/ILock.cs create mode 100644 src/Orchard/Tasks/Locking/ILockService.cs create mode 100644 src/Orchard/Tasks/Locking/LockService.cs diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs index 4f43eb7a5..464894eb5 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs @@ -1,6 +1,4 @@ using System; -using Orchard.ContentManagement.MetaData; -using Orchard.Core.Contents.Extensions; using Orchard.Data.Migration; namespace Orchard.TaskLease { @@ -19,5 +17,14 @@ namespace Orchard.TaskLease { return 1; } + + public int UpdateFrom1() { + SchemaBuilder.CreateTable("DatabaseLockRecord", table => table + .Column("Id", column => column.PrimaryKey().Identity()) + .Column("Name", column => column.NotNull().WithLength(256)) + .Column("AcquiredUtc")); + + return 2; + } } } \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Models/DatabaseLockRecord.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Models/DatabaseLockRecord.cs new file mode 100644 index 000000000..039e5d0d8 --- /dev/null +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Models/DatabaseLockRecord.cs @@ -0,0 +1,9 @@ +using System; + +namespace Orchard.TaskLease.Models { + public class DatabaseLockRecord { + public virtual int Id { get; set; } + public virtual string Name { get; set; } + public virtual DateTime? AcquiredUtc { get; set; } + } +} \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj b/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj index 7273eb307..7d5361a83 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj @@ -49,6 +49,9 @@ false + + ..\..\..\..\lib\autofac\Autofac.dll + @@ -73,6 +76,7 @@ + @@ -82,6 +86,7 @@ + diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs new file mode 100644 index 000000000..3ca738c28 --- /dev/null +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs @@ -0,0 +1,100 @@ +using System; +using System.Data; +using System.Linq; +using Autofac; +using Orchard.Data; +using Orchard.Environment.Extensions; +using Orchard.Exceptions; +using Orchard.Services; +using Orchard.TaskLease.Models; +using Orchard.Tasks.Locking; +using Orchard.Validation; + +namespace Orchard.TaskLease.Services { + + /// + /// Provides a database driven implementation of + /// + [OrchardSuppressDependency("Orchard.Tasks.Locking.DefaultLock")] + public class DatabaseLock : ILock { + private readonly ILifetimeScope _lifetimeScope; + private readonly IClock _clock; + private string _name; + private bool _isAcquired; + private int _id; + private bool _isDisposed; + + public DatabaseLock(ILifetimeScope lifetimeScope, IClock clock) { + _lifetimeScope = lifetimeScope; + _clock = clock; + } + + public bool TryAcquire(string name, TimeSpan maxLifetime) { + Argument.ThrowIfNullOrEmpty(name, "name"); + + if (name.Length > 256) + throw new ArgumentException("The lock's name can't be longer than 256 characters."); + + // This way we can create a nested transaction scope instead of having the unwanted effect + // of manipulating the transaction of the caller. + using (var scope = BeginLifeTimeScope(name)) { + var repository = scope.Resolve>(); + var record = repository.Table.FirstOrDefault(x => x.Name == name); + + if (record != null) { + // There is a nexisting lock, but check if it has expired. + var isExpired = record.AcquiredUtc + maxLifetime < _clock.UtcNow; + if (isExpired) { + repository.Delete(record); + record = null; + } + } + + var canAcquire = record == null; + + if (canAcquire) { + record = new DatabaseLockRecord { + Name = name, + AcquiredUtc = _clock.UtcNow + }; + repository.Create(record); + repository.Flush(); + + _name = name; + _isAcquired = true; + _id = record.Id; + } + + return canAcquire; + } + } + + // This will be called at least by the IoC container when the request ends. + public void Dispose() { + if (_isDisposed || !_isAcquired) return; + + _isDisposed = true; + + using (var scope = BeginLifeTimeScope(_name)) { + try { + var repository = scope.Resolve>(); + var record = repository.Get(_id); + + if (record != null) { + repository.Delete(record); + repository.Flush(); + } + } + catch (Exception ex) { + if (ex.IsFatal()) throw; + } + } + } + + private ILifetimeScope BeginLifeTimeScope(string name) { + var scope = _lifetimeScope.BeginLifetimeScope("Orchard.Tasks.Locking.Database." + name); + scope.Resolve().RequireNew(IsolationLevel.ReadCommitted); + return scope; + } + } +} \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs index bf3abfb2c..d9b5effd5 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs @@ -7,6 +7,7 @@ namespace Orchard.TaskLease.Services { /// Describes a service to save and acquire task leases. A task lease can't be acquired by two different machines, /// for a specific amount of time. Optionnally a State can be saved along with the lease. /// + [Obsolete("Use Orchard.Tasks.Locking.ILockService instead.")] public interface ITaskLeaseService : IDependency { /// diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs index b8f17306c..0d58f1c28 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs @@ -9,6 +9,7 @@ namespace Orchard.TaskLease.Services { /// /// Provides a database driven implementation of /// + [Obsolete("Use Orchard.Tasks.Locking.ILockService instead.")] public class TaskLeaseService : ITaskLeaseService { private readonly IRepository _repository; diff --git a/src/Orchard/Orchard.Framework.csproj b/src/Orchard/Orchard.Framework.csproj index 216323343..9693a7400 100644 --- a/src/Orchard/Orchard.Framework.csproj +++ b/src/Orchard/Orchard.Framework.csproj @@ -397,6 +397,10 @@ + + + + diff --git a/src/Orchard/Tasks/Locking/DefaultLock.cs b/src/Orchard/Tasks/Locking/DefaultLock.cs new file mode 100644 index 000000000..9ece47036 --- /dev/null +++ b/src/Orchard/Tasks/Locking/DefaultLock.cs @@ -0,0 +1,14 @@ +using System; + +namespace Orchard.Tasks.Locking { + public class DefaultLock : ILock { + + public bool TryAcquire(string name, TimeSpan maxLifetime) { + return true; + } + + public void Dispose() { + // Noop. + } + } +} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/ILock.cs b/src/Orchard/Tasks/Locking/ILock.cs new file mode 100644 index 000000000..a80237d85 --- /dev/null +++ b/src/Orchard/Tasks/Locking/ILock.cs @@ -0,0 +1,16 @@ +using System; + +namespace Orchard.Tasks.Locking { + /// + /// Provides a lock on a provided name. + /// + public interface ILock : ITransientDependency, IDisposable { + /// + /// Tries to acquire a lock on the specified name. + /// + /// 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 true if a lock was acquired, false otherwise. + bool TryAcquire(string name, TimeSpan maxLifetime); + } +} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/ILockService.cs b/src/Orchard/Tasks/Locking/ILockService.cs new file mode 100644 index 000000000..fbb0928cb --- /dev/null +++ b/src/Orchard/Tasks/Locking/ILockService.cs @@ -0,0 +1,27 @@ +using System; + +namespace Orchard.Tasks.Locking { + /// + /// Provides distributed locking functionality. + /// + public interface ILockService : IDependency { + /// + /// Tries to acquire a lock on the specified name. + /// + /// 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. + ILock TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout); + + /// + /// Acquires a lock with the specified parameters. + /// + /// 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. + /// Thrown if the lock couldn't be acquired. + ILock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout); + } +} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/LockService.cs b/src/Orchard/Tasks/Locking/LockService.cs new file mode 100644 index 000000000..395408926 --- /dev/null +++ b/src/Orchard/Tasks/Locking/LockService.cs @@ -0,0 +1,48 @@ +using System; +using System.Threading.Tasks; +using Orchard.Environment; +using Orchard.Logging; + +namespace Orchard.Tasks.Locking { + public class LockService : ILockService { + private readonly Work _lock; + private readonly IMachineNameProvider _machineNameProvider; + + public LockService(Work @lock, IMachineNameProvider machineNameProvider) { + _lock = @lock; + _machineNameProvider = machineNameProvider; + Logger = NullLogger.Instance; + } + + public ILogger Logger { get; set; } + + public ILock TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout) { + var waitedTime = TimeSpan.Zero; + var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); + var @lock = _lock.Value; + bool acquired; + + while (!(acquired = @lock.TryAcquire(name, maxLifetime)) && waitedTime < timeout) { + Task.Delay(timeout).ContinueWith(t => { + waitedTime += waitTime; + }).Wait(); + } + + var machineName = _machineNameProvider.GetMachineName(); + + if (acquired) { + Logger.Debug(String.Format("Successfully acquired a lock named {0} on machine {1}.", name, machineName)); + return @lock; + } + + Logger.Debug(String.Format("Failed to acquire a lock named {0} on machine {1}.", name, machineName)); + return null; + } + + public ILock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout) { + var lockResult = TryAcquireLock(name, maxLifetime, timeout); + if (lockResult != null) return lockResult; + throw new TimeoutException(String.Format("No lock for \"{0}\" could not be acquired within {1} milliseconds.", name, timeout.TotalMilliseconds)); + } + } +} \ No newline at end of file From 5813647574f60aa6caf42960340a5af9ae629c5e Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 12:47:53 +0100 Subject: [PATCH 02/28] Refactor renamed ILock to IDistributedLock. Also refactored the IDistributedLockService to be more in line with general .NET patterns by return a boolean from the TryAcquire method. --- .../Services/DatabaseLock.cs | 4 ++-- .../Services/ITaskLeaseService.cs | 3 +-- .../Services/TaskLeaseService.cs | 2 +- src/Orchard/Orchard.Framework.csproj | 6 +++--- src/Orchard/Tasks/Locking/DefaultLock.cs | 2 +- ...ckService.cs => DistributedLockService.cs} | 21 +++++++++---------- .../Locking/{ILock.cs => IDistributedLock.cs} | 2 +- ...kService.cs => IDistributedLockService.cs} | 16 +++++++------- 8 files changed, 27 insertions(+), 29 deletions(-) rename src/Orchard/Tasks/Locking/{LockService.cs => DistributedLockService.cs} (60%) rename src/Orchard/Tasks/Locking/{ILock.cs => IDistributedLock.cs} (89%) rename src/Orchard/Tasks/Locking/{ILockService.cs => IDistributedLockService.cs} (66%) diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs index 3ca738c28..bc731e92e 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs @@ -13,10 +13,10 @@ using Orchard.Validation; namespace Orchard.TaskLease.Services { /// - /// Provides a database driven implementation of + /// Provides a database driven implementation of /// [OrchardSuppressDependency("Orchard.Tasks.Locking.DefaultLock")] - public class DatabaseLock : ILock { + public class DatabaseLock : IDistributedLock { private readonly ILifetimeScope _lifetimeScope; private readonly IClock _clock; private string _name; diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs index d9b5effd5..c32e3d37e 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs @@ -1,5 +1,4 @@ using System; -using Orchard.TaskLease.Models; namespace Orchard.TaskLease.Services { @@ -7,7 +6,7 @@ namespace Orchard.TaskLease.Services { /// Describes a service to save and acquire task leases. A task lease can't be acquired by two different machines, /// for a specific amount of time. Optionnally a State can be saved along with the lease. /// - [Obsolete("Use Orchard.Tasks.Locking.ILockService instead.")] + [Obsolete("Use Orchard.Tasks.Locking.IDistributedLockService instead.")] public interface ITaskLeaseService : IDependency { /// diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs index 0d58f1c28..398c1092f 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs @@ -9,7 +9,7 @@ namespace Orchard.TaskLease.Services { /// /// Provides a database driven implementation of /// - [Obsolete("Use Orchard.Tasks.Locking.ILockService instead.")] + [Obsolete("Use Orchard.Tasks.Locking.DistributedLockService instead.")] public class TaskLeaseService : ITaskLeaseService { private readonly IRepository _repository; diff --git a/src/Orchard/Orchard.Framework.csproj b/src/Orchard/Orchard.Framework.csproj index 9693a7400..d919e2a68 100644 --- a/src/Orchard/Orchard.Framework.csproj +++ b/src/Orchard/Orchard.Framework.csproj @@ -398,9 +398,9 @@ - - - + + + diff --git a/src/Orchard/Tasks/Locking/DefaultLock.cs b/src/Orchard/Tasks/Locking/DefaultLock.cs index 9ece47036..adbf1dd36 100644 --- a/src/Orchard/Tasks/Locking/DefaultLock.cs +++ b/src/Orchard/Tasks/Locking/DefaultLock.cs @@ -1,7 +1,7 @@ using System; namespace Orchard.Tasks.Locking { - public class DefaultLock : ILock { + public class DefaultLock : IDistributedLock { public bool TryAcquire(string name, TimeSpan maxLifetime) { return true; diff --git a/src/Orchard/Tasks/Locking/LockService.cs b/src/Orchard/Tasks/Locking/DistributedLockService.cs similarity index 60% rename from src/Orchard/Tasks/Locking/LockService.cs rename to src/Orchard/Tasks/Locking/DistributedLockService.cs index 395408926..0894d109b 100644 --- a/src/Orchard/Tasks/Locking/LockService.cs +++ b/src/Orchard/Tasks/Locking/DistributedLockService.cs @@ -4,11 +4,11 @@ using Orchard.Environment; using Orchard.Logging; namespace Orchard.Tasks.Locking { - public class LockService : ILockService { - private readonly Work _lock; + public class DistributedLockService : IDistributedLockService { + private readonly Work _lock; private readonly IMachineNameProvider _machineNameProvider; - public LockService(Work @lock, IMachineNameProvider machineNameProvider) { + public DistributedLockService(Work @lock, IMachineNameProvider machineNameProvider) { _lock = @lock; _machineNameProvider = machineNameProvider; Logger = NullLogger.Instance; @@ -16,10 +16,10 @@ namespace Orchard.Tasks.Locking { public ILogger Logger { get; set; } - public ILock TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout) { + public bool TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout, out IDistributedLock @lock) { var waitedTime = TimeSpan.Zero; var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); - var @lock = _lock.Value; + @lock = _lock.Value; bool acquired; while (!(acquired = @lock.TryAcquire(name, maxLifetime)) && waitedTime < timeout) { @@ -32,17 +32,16 @@ namespace Orchard.Tasks.Locking { if (acquired) { Logger.Debug(String.Format("Successfully acquired a lock named {0} on machine {1}.", name, machineName)); - return @lock; + return true; } Logger.Debug(String.Format("Failed to acquire a lock named {0} on machine {1}.", name, machineName)); - return null; + return false; } - public ILock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout) { - var lockResult = TryAcquireLock(name, maxLifetime, timeout); - if (lockResult != null) return lockResult; - throw new TimeoutException(String.Format("No lock for \"{0}\" could not be acquired within {1} milliseconds.", name, timeout.TotalMilliseconds)); + public IDistributedLock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout) { + IDistributedLock lockResult; + return TryAcquireLock(name, maxLifetime, timeout, out lockResult) ? lockResult : null; } } } \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/ILock.cs b/src/Orchard/Tasks/Locking/IDistributedLock.cs similarity index 89% rename from src/Orchard/Tasks/Locking/ILock.cs rename to src/Orchard/Tasks/Locking/IDistributedLock.cs index a80237d85..dd4655daf 100644 --- a/src/Orchard/Tasks/Locking/ILock.cs +++ b/src/Orchard/Tasks/Locking/IDistributedLock.cs @@ -4,7 +4,7 @@ namespace Orchard.Tasks.Locking { /// /// Provides a lock on a provided name. /// - public interface ILock : ITransientDependency, IDisposable { + public interface IDistributedLock : ITransientDependency, IDisposable { /// /// Tries to acquire a lock on the specified name. /// diff --git a/src/Orchard/Tasks/Locking/ILockService.cs b/src/Orchard/Tasks/Locking/IDistributedLockService.cs similarity index 66% rename from src/Orchard/Tasks/Locking/ILockService.cs rename to src/Orchard/Tasks/Locking/IDistributedLockService.cs index fbb0928cb..df6169129 100644 --- a/src/Orchard/Tasks/Locking/ILockService.cs +++ b/src/Orchard/Tasks/Locking/IDistributedLockService.cs @@ -4,24 +4,24 @@ namespace Orchard.Tasks.Locking { /// /// Provides distributed locking functionality. /// - public interface ILockService : IDependency { + public interface IDistributedLockService : IDependency { /// /// Tries to acquire a lock on the specified name. /// /// 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. - ILock TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout); + /// The amount of time to wait for the lock to be acquired before timing out. + /// The acquired lock. + /// Returns true if a lock was successfully acquired, false otherwise. + bool TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout, out IDistributedLock @lock); /// /// Acquires a lock with the specified parameters. /// /// 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. - /// Thrown if the lock couldn't be acquired. - ILock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout); + /// 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); } } \ No newline at end of file From 15e6c95740cd95eb386192ab0dd7b1d6b2829dd1 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 13:24:54 +0100 Subject: [PATCH 03/28] Added thread synchronization. This ensures that only one thread at a time can acquire a lock. --- .../Tasks/Locking/DistributedLockService.cs | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/src/Orchard/Tasks/Locking/DistributedLockService.cs b/src/Orchard/Tasks/Locking/DistributedLockService.cs index 0894d109b..042f47734 100644 --- a/src/Orchard/Tasks/Locking/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/DistributedLockService.cs @@ -1,10 +1,12 @@ using System; +using System.Threading; using System.Threading.Tasks; using Orchard.Environment; using Orchard.Logging; namespace Orchard.Tasks.Locking { public class DistributedLockService : IDistributedLockService { + private static readonly object _semaphore = new object(); private readonly Work _lock; private readonly IMachineNameProvider _machineNameProvider; @@ -17,22 +19,35 @@ namespace Orchard.Tasks.Locking { public ILogger Logger { get; set; } public bool TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout, out IDistributedLock @lock) { - var waitedTime = TimeSpan.Zero; - var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); - @lock = _lock.Value; - bool acquired; - - while (!(acquired = @lock.TryAcquire(name, maxLifetime)) && waitedTime < timeout) { - Task.Delay(timeout).ContinueWith(t => { - waitedTime += waitTime; - }).Wait(); - } - var machineName = _machineNameProvider.GetMachineName(); + @lock = _lock.Value; - if (acquired) { - Logger.Debug(String.Format("Successfully acquired a lock named {0} on machine {1}.", name, machineName)); - return true; + if (Monitor.TryEnter(_semaphore)) { + try { + var waitedTime = TimeSpan.Zero; + var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); + bool acquired; + + while (!(acquired = @lock.TryAcquire(name, maxLifetime)) && waitedTime < timeout) { + Task.Delay(timeout).ContinueWith(t => { + waitedTime += waitTime; + }).Wait(); + } + + + if (acquired) { + Logger.Debug(String.Format("Successfully acquired a lock named {0} on machine {1}.", name, machineName)); + return true; + } + } + catch (Exception ex) { + Logger.Error(ex, "Error during acquire lock."); + throw; + } + finally { + Monitor.Exit(_semaphore); + Logger.Debug("Ending sweep."); + } } Logger.Debug(String.Format("Failed to acquire a lock named {0} on machine {1}.", name, machineName)); From f2e0fb22b33129ed5e15064d5788bab8973a9c35 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 13:48:54 +0100 Subject: [PATCH 04/28] Removed copy/paste artifact. --- src/Orchard/Tasks/Locking/DistributedLockService.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Orchard/Tasks/Locking/DistributedLockService.cs b/src/Orchard/Tasks/Locking/DistributedLockService.cs index 042f47734..7bddc9f4e 100644 --- a/src/Orchard/Tasks/Locking/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/DistributedLockService.cs @@ -34,7 +34,6 @@ namespace Orchard.Tasks.Locking { }).Wait(); } - if (acquired) { Logger.Debug(String.Format("Successfully acquired a lock named {0} on machine {1}.", name, machineName)); return true; @@ -46,7 +45,6 @@ namespace Orchard.Tasks.Locking { } finally { Monitor.Exit(_semaphore); - Logger.Debug("Ending sweep."); } } From ab12e9e6309f8dc36638bd4205a8dff277c686a1 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 14:06:13 +0100 Subject: [PATCH 05/28] Turned IDistributedLockService into an ISingleton. --- src/Orchard/Tasks/Locking/IDistributedLockService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Orchard/Tasks/Locking/IDistributedLockService.cs b/src/Orchard/Tasks/Locking/IDistributedLockService.cs index df6169129..65aa7bf9a 100644 --- a/src/Orchard/Tasks/Locking/IDistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/IDistributedLockService.cs @@ -4,7 +4,7 @@ namespace Orchard.Tasks.Locking { /// /// Provides distributed locking functionality. /// - public interface IDistributedLockService : IDependency { + public interface IDistributedLockService : ISingletonDependency { /// /// Tries to acquire a lock on the specified name. /// From b62939f850ed97c59a4fe96a1c9cf70855cb19e0 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 14:06:58 +0100 Subject: [PATCH 06/28] Implemented locking using ReaderWriterLockSlim and simplified logging statements. --- src/Orchard/Tasks/Locking/DistributedLockService.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Orchard/Tasks/Locking/DistributedLockService.cs b/src/Orchard/Tasks/Locking/DistributedLockService.cs index 7bddc9f4e..ee5c2e84e 100644 --- a/src/Orchard/Tasks/Locking/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/DistributedLockService.cs @@ -6,7 +6,7 @@ using Orchard.Logging; namespace Orchard.Tasks.Locking { public class DistributedLockService : IDistributedLockService { - private static readonly object _semaphore = new object(); + private readonly ReaderWriterLockSlim _rwl = new ReaderWriterLockSlim(); private readonly Work _lock; private readonly IMachineNameProvider _machineNameProvider; @@ -22,7 +22,7 @@ namespace Orchard.Tasks.Locking { var machineName = _machineNameProvider.GetMachineName(); @lock = _lock.Value; - if (Monitor.TryEnter(_semaphore)) { + if (_rwl.TryEnterWriteLock(0)) { try { var waitedTime = TimeSpan.Zero; var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); @@ -35,20 +35,20 @@ namespace Orchard.Tasks.Locking { } if (acquired) { - Logger.Debug(String.Format("Successfully acquired a lock named {0} on machine {1}.", name, machineName)); + Logger.Debug("Successfully acquired a lock named {0} on machine {1}.", name, machineName); return true; } } catch (Exception ex) { - Logger.Error(ex, "Error during acquire lock."); + Logger.Error(ex, "Error while trying to acquire a lock named {0} on machine {1}.", name, machineName); throw; } finally { - Monitor.Exit(_semaphore); + _rwl.ExitWriteLock(); } } - Logger.Debug(String.Format("Failed to acquire a lock named {0} on machine {1}.", name, machineName)); + Logger.Debug("Could not acquire a lock named {0} on machine {1}.", name, machineName); return false; } From 17082eec9c4b0e92ed409702d7d3e2c2fdab5959 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 15:13:03 +0100 Subject: [PATCH 07/28] Refactored disposal of DatabaseLock. This ensures the cleanup will be able to resolve a database transaction even when it's Autofac that's disposing the object. --- .../Services/DatabaseLock.cs | 77 ++++++++++++------- 1 file changed, 49 insertions(+), 28 deletions(-) diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs index bc731e92e..1b31f6ec4 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs @@ -5,6 +5,7 @@ using Autofac; using Orchard.Data; using Orchard.Environment.Extensions; using Orchard.Exceptions; +using Orchard.Logging; using Orchard.Services; using Orchard.TaskLease.Models; using Orchard.Tasks.Locking; @@ -23,21 +24,28 @@ namespace Orchard.TaskLease.Services { private bool _isAcquired; private int _id; private bool _isDisposed; + private ILifetimeScope _scope; public DatabaseLock(ILifetimeScope lifetimeScope, IClock clock) { _lifetimeScope = lifetimeScope; _clock = clock; + Logger = NullLogger.Instance; } + public ILogger Logger { get; set; } + public bool TryAcquire(string name, TimeSpan maxLifetime) { Argument.ThrowIfNullOrEmpty(name, "name"); if (name.Length > 256) throw new ArgumentException("The lock's name can't be longer than 256 characters."); - // This way we can create a nested transaction scope instead of having the unwanted effect - // of manipulating the transaction of the caller. - using (var scope = BeginLifeTimeScope(name)) { + try { + var scope = EnsureLifetimeScope(name); + scope.Resolve().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 = scope.Resolve>(); var record = repository.Table.FirstOrDefault(x => x.Name == name); @@ -67,34 +75,47 @@ namespace Orchard.TaskLease.Services { return canAcquire; } - } - - // This will be called at least by the IoC container when the request ends. - public void Dispose() { - if (_isDisposed || !_isAcquired) return; - - _isDisposed = true; - - using (var scope = BeginLifeTimeScope(_name)) { - try { - var repository = scope.Resolve>(); - var record = repository.Get(_id); - - if (record != null) { - repository.Delete(record); - repository.Flush(); - } - } - catch (Exception ex) { - if (ex.IsFatal()) throw; - } + catch (Exception ex) { + Logger.Error(ex, "An error occurred while trying to acquire a lock."); + DisposeLifetimeScope(); + throw; } } - private ILifetimeScope BeginLifeTimeScope(string name) { - var scope = _lifetimeScope.BeginLifetimeScope("Orchard.Tasks.Locking.Database." + name); - scope.Resolve().RequireNew(IsolationLevel.ReadCommitted); - return scope; + // This will be called at least and at the latest by the IoC container when the request ends. + public void Dispose() { + if (_scope == null) + return; + + if (!_isDisposed) { + _isDisposed = true; + + if (_isAcquired) { + try { + var repository = _scope.Resolve>(); + var record = repository.Get(_id); + + if (record != null) { + repository.Delete(record); + repository.Flush(); + } + } + catch (Exception ex) { + if (ex.IsFatal()) throw; + } + } + + _scope.Dispose(); + } + } + + private ILifetimeScope EnsureLifetimeScope(string name) { + return _scope = _lifetimeScope.BeginLifetimeScope("Orchard.Tasks.Locking.Database." + name); + } + + private void DisposeLifetimeScope() { + if (_scope != null) + _scope.Dispose(); } } } \ No newline at end of file From 16cd4e89c4e1ed2ed78da598f12b52a4ea8c0522 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 15:24:18 +0100 Subject: [PATCH 08/28] Added tests for DatabaseLock. --- .../Orchard.Tests.Modules.csproj | 5 ++ .../TaskLease/DatabaseLockTests.cs | 66 +++++++++++++++++++ .../Orchard.Framework.Tests.csproj | 1 + .../Stubs/StubMachineNameProvider.cs | 9 +++ 4 files changed, 81 insertions(+) create mode 100644 src/Orchard.Tests.Modules/TaskLease/DatabaseLockTests.cs create mode 100644 src/Orchard.Tests/Stubs/StubMachineNameProvider.cs diff --git a/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj b/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj index 49d0f16e5..3fe8e2bff 100644 --- a/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj +++ b/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj @@ -182,6 +182,7 @@ + @@ -281,6 +282,10 @@ {5D0F00F0-26C9-4785-AD61-B85710C60EB0} Orchard.Tags + + {3f72a4e9-7b72-4260-b010-c16ec54f9baf} + Orchard.TaskLease + {CDE24A24-01D3-403C-84B9-37722E18DFB7} Orchard.Themes diff --git a/src/Orchard.Tests.Modules/TaskLease/DatabaseLockTests.cs b/src/Orchard.Tests.Modules/TaskLease/DatabaseLockTests.cs new file mode 100644 index 000000000..5cf9611b8 --- /dev/null +++ b/src/Orchard.Tests.Modules/TaskLease/DatabaseLockTests.cs @@ -0,0 +1,66 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using Autofac; +using NUnit.Framework; +using Orchard.Data; +using Orchard.TaskLease.Models; +using Orchard.TaskLease.Services; + +namespace Orchard.Tests.Modules.TaskLease { + [TestFixture] + public class DatabaseLockTests : DatabaseEnabledTestsBase { + private IRepository _databaseLockRepository; + private DatabaseLock _lock; + + protected override IEnumerable DatabaseTypes { + get { + yield return typeof(DatabaseLockRecord); + } + } + + public override void Register(ContainerBuilder builder) { + builder.RegisterType().AsSelf(); + } + + public override void Init() { + base.Init(); + + _databaseLockRepository = _container.Resolve>(); + _lock = _container.Resolve(); + } + + [Test] + public void AcquiringLockSucceeds() { + var lockAcquired = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + + Assert.That(lockAcquired, Is.True); + Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(1)); + } + + [Test] + public void DisposingRemovesLockRecord() { + _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + _lock.Dispose(); + Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(0)); + } + + [Test] + public void AcquiringLockTwiceFails() { + var attempt1 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + var attempt2 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + + Assert.That(attempt1, Is.True); + Assert.That(attempt2, Is.False); + } + + [Test] + public void AcquiringExpiredLockSucceeds() { + var attempt1 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + var attempt2 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(-1)); // Treat the previosuly stored lock as immediately expired. + + Assert.That(attempt1, Is.True); + Assert.That(attempt2, Is.True); + } + } +} \ No newline at end of file diff --git a/src/Orchard.Tests/Orchard.Framework.Tests.csproj b/src/Orchard.Tests/Orchard.Framework.Tests.csproj index c26fc9d28..2ab1f8f73 100644 --- a/src/Orchard.Tests/Orchard.Framework.Tests.csproj +++ b/src/Orchard.Tests/Orchard.Framework.Tests.csproj @@ -269,6 +269,7 @@ + diff --git a/src/Orchard.Tests/Stubs/StubMachineNameProvider.cs b/src/Orchard.Tests/Stubs/StubMachineNameProvider.cs new file mode 100644 index 000000000..5d4b49638 --- /dev/null +++ b/src/Orchard.Tests/Stubs/StubMachineNameProvider.cs @@ -0,0 +1,9 @@ +using Orchard.Environment; + +namespace Orchard.Tests.Stubs { + public class StubMachineNameProvider : IMachineNameProvider { + public string GetMachineName() { + return "Orchard Machine"; + } + } +} From 68c401c6ebdff4e4d405db7f929242140dfaff6d Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 15:27:15 +0100 Subject: [PATCH 09/28] Preventing usage disposed object and fixed EnsureLifetimeScope. --- .../Modules/Orchard.TaskLease/Services/DatabaseLock.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs index 1b31f6ec4..7e11d1c70 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs @@ -35,6 +35,9 @@ namespace Orchard.TaskLease.Services { public ILogger Logger { get; set; } public bool TryAcquire(string name, TimeSpan maxLifetime) { + if(_isDisposed) + throw new ObjectDisposedException("DatabaseLock"); + Argument.ThrowIfNullOrEmpty(name, "name"); if (name.Length > 256) @@ -110,7 +113,7 @@ namespace Orchard.TaskLease.Services { } private ILifetimeScope EnsureLifetimeScope(string name) { - return _scope = _lifetimeScope.BeginLifetimeScope("Orchard.Tasks.Locking.Database." + name); + return _scope ?? (_scope = _lifetimeScope.BeginLifetimeScope("Orchard.Tasks.Locking.Database." + name)); } private void DisposeLifetimeScope() { From 97fcfd4e241e197b66c5947a045f250727cd4f58 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 16:27:04 +0100 Subject: [PATCH 10/28] Added tests for DistributedService. --- .../Orchard.Framework.Tests.csproj | 2 + .../Stubs/StubDistributedLock.cs | 22 +++++++ .../Tasks/DistributedLockServiceTests.cs | 64 +++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 src/Orchard.Tests/Stubs/StubDistributedLock.cs create mode 100644 src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs diff --git a/src/Orchard.Tests/Orchard.Framework.Tests.csproj b/src/Orchard.Tests/Orchard.Framework.Tests.csproj index 2ab1f8f73..ca06a5084 100644 --- a/src/Orchard.Tests/Orchard.Framework.Tests.csproj +++ b/src/Orchard.Tests/Orchard.Framework.Tests.csproj @@ -269,6 +269,7 @@ + @@ -291,6 +292,7 @@ + diff --git a/src/Orchard.Tests/Stubs/StubDistributedLock.cs b/src/Orchard.Tests/Stubs/StubDistributedLock.cs new file mode 100644 index 000000000..120fb0ca5 --- /dev/null +++ b/src/Orchard.Tests/Stubs/StubDistributedLock.cs @@ -0,0 +1,22 @@ +using System; +using Orchard.Tasks.Locking; + +namespace Orchard.Tests.Stubs { + public class StubDistributedLock : IDistributedLock { + public static bool IsAcquired { get; private set; } + + public bool IsDisposed { get; private set; } + + public bool TryAcquire(string name, TimeSpan maxLifetime) { + if (IsAcquired) + return false; + + return IsAcquired = true; + } + + + public void Dispose() { + IsDisposed = true; + } + } +} diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs new file mode 100644 index 000000000..85d0772ff --- /dev/null +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -0,0 +1,64 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Autofac; +using NUnit.Framework; +using Orchard.Environment; +using Orchard.Tasks.Locking; +using Orchard.Tests.Stubs; + +namespace Orchard.Tests.Tasks { + [TestFixture] + public class DistributedLockServiceTests : ContainerTestBase { + private DistributedLockService _distributedLockService; + + protected override void Register(ContainerBuilder builder) { + builder.RegisterType().As(); + builder.RegisterType().As(); + builder.RegisterType().AsSelf(); + builder.RegisterInstance(new Work(resolve => _container.Resolve())).AsSelf(); + } + + protected override void Resolve(ILifetimeScope container) { + _distributedLockService = container.Resolve(); + } + + [Test] + public void AcquiringLockSucceeds() { + IDistributedLock @lock; + var lockAcquired = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + + Assert.That(lockAcquired, Is.True); + } + + [Test] + public void AcquiringLockTwiceFails() { + IDistributedLock @lock; + var attempt1 = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + var attempt2 = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + + Assert.That(attempt1, Is.True); + Assert.That(attempt2, Is.False); + } + + [Test] + public void MultipleSimultaneousAcquisitionsShouldAllowOneLock() { + var attempts = new List(); + var tasks = new List(); + + foreach (var index in Enumerable.Range(0, 10).AsParallel()) { + var task = Task.Factory.StartNew(() => { + IDistributedLock @lock; + var attempt = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + attempts.Add(attempt); + }); + + tasks.Add(task); + } + + Task.WaitAll(tasks.ToArray()); + Assert.That(attempts.Count(x => x == true), Is.EqualTo(1)); + } + } +} From 92c6d5fd80defe4d9282a88284954544b1649b3b Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 17:43:21 +0100 Subject: [PATCH 11/28] Added logging statement in case of non-fatal exception. --- .../Modules/Orchard.TaskLease/Services/DatabaseLock.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs index 7e11d1c70..bb251d9a2 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs @@ -105,6 +105,7 @@ namespace Orchard.TaskLease.Services { } catch (Exception ex) { if (ex.IsFatal()) throw; + Logger.Error(ex, "An non-fatal error occurred while trying to dispose the database lock."); } } From 01de9ef64686470a80786c59cb1037211056f194 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 18:05:15 +0100 Subject: [PATCH 12/28] Addec leanup code to the ContainerTestBase. --- src/Orchard.Tests/ContainerTestBase.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Orchard.Tests/ContainerTestBase.cs b/src/Orchard.Tests/ContainerTestBase.cs index 18625dbad..0bf447de8 100644 --- a/src/Orchard.Tests/ContainerTestBase.cs +++ b/src/Orchard.Tests/ContainerTestBase.cs @@ -14,6 +14,12 @@ namespace Orchard.Tests { Resolve(_container); } + [TearDown] + public void Cleanup() { + if (_container != null) + _container.Dispose(); + } + #if false // technically more accurate, and doesn't work [SetUp] From 0a9b05314ec4482adf5db81c8bda34f79bb919d9 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 18:06:57 +0100 Subject: [PATCH 13/28] Changed dependency resolution. This ensures dependency resolution works even when there's no work context (which happens when the code executes at the EndRequest event for example). --- src/Orchard/Tasks/Locking/DistributedLockService.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Orchard/Tasks/Locking/DistributedLockService.cs b/src/Orchard/Tasks/Locking/DistributedLockService.cs index ee5c2e84e..7322fc854 100644 --- a/src/Orchard/Tasks/Locking/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/DistributedLockService.cs @@ -7,11 +7,11 @@ using Orchard.Logging; namespace Orchard.Tasks.Locking { public class DistributedLockService : IDistributedLockService { private readonly ReaderWriterLockSlim _rwl = new ReaderWriterLockSlim(); - private readonly Work _lock; + private readonly IWorkContextAccessor _wca; private readonly IMachineNameProvider _machineNameProvider; - public DistributedLockService(Work @lock, IMachineNameProvider machineNameProvider) { - _lock = @lock; + public DistributedLockService(IWorkContextAccessor wca, IMachineNameProvider machineNameProvider) { + _wca = wca; _machineNameProvider = machineNameProvider; Logger = NullLogger.Instance; } @@ -20,7 +20,7 @@ namespace Orchard.Tasks.Locking { public bool TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout, out IDistributedLock @lock) { var machineName = _machineNameProvider.GetMachineName(); - @lock = _lock.Value; + @lock = Resolve(); if (_rwl.TryEnterWriteLock(0)) { try { @@ -52,6 +52,11 @@ namespace Orchard.Tasks.Locking { return false; } + private T Resolve() { + var workContext = _wca.GetContext() ?? _wca.CreateWorkContextScope().WorkContext; // In case this is invoked at the end of the request. + return workContext.Resolve(); + } + public IDistributedLock AcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout) { IDistributedLock lockResult; return TryAcquireLock(name, maxLifetime, timeout, out lockResult) ? lockResult : null; From cb6835aba3ab4694c0fcb64e3de564d35f77c268 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Aug 2015 18:08:00 +0100 Subject: [PATCH 14/28] Updated AutomaticDataMigrations to take advantage of the DistributedLockService. --- .../Stubs/StubDistributedLock.cs | 1 + .../Tasks/DistributedLockServiceTests.cs | 2 + .../Modules/Orchard.Setup/SetupMode.cs | 1 + .../Data/Migration/AutomaticDataMigrations.cs | 44 +++++++++++-------- src/Orchard/Tasks/Locking/DefaultLock.cs | 21 ++++++++- 5 files changed, 49 insertions(+), 20 deletions(-) diff --git a/src/Orchard.Tests/Stubs/StubDistributedLock.cs b/src/Orchard.Tests/Stubs/StubDistributedLock.cs index 120fb0ca5..341bd3732 100644 --- a/src/Orchard.Tests/Stubs/StubDistributedLock.cs +++ b/src/Orchard.Tests/Stubs/StubDistributedLock.cs @@ -17,6 +17,7 @@ namespace Orchard.Tests.Stubs { public void Dispose() { IsDisposed = true; + IsAcquired = false; } } } diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index 85d0772ff..a559540c1 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -14,6 +14,7 @@ namespace Orchard.Tests.Tasks { private DistributedLockService _distributedLockService; protected override void Register(ContainerBuilder builder) { + builder.RegisterType().As(); builder.RegisterType().As(); builder.RegisterType().As(); builder.RegisterType().AsSelf(); @@ -24,6 +25,7 @@ namespace Orchard.Tests.Tasks { _distributedLockService = container.Resolve(); } + [Test] public void AcquiringLockSucceeds() { IDistributedLock @lock; diff --git a/src/Orchard.Web/Modules/Orchard.Setup/SetupMode.cs b/src/Orchard.Web/Modules/Orchard.Setup/SetupMode.cs index bf0688b20..ec675dac8 100644 --- a/src/Orchard.Web/Modules/Orchard.Setup/SetupMode.cs +++ b/src/Orchard.Web/Modules/Orchard.Setup/SetupMode.cs @@ -29,6 +29,7 @@ using Orchard.Mvc.ViewEngines.ThemeAwareness; using Orchard.Recipes.Services; using Orchard.Settings; using Orchard.Tasks; +using Orchard.Tasks.Locking; using Orchard.Themes; using Orchard.UI.Notify; using Orchard.UI.PageClass; diff --git a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs index f592f30fc..70cb78408 100644 --- a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs +++ b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs @@ -3,6 +3,7 @@ using System.Linq; using Orchard.Environment; using Orchard.Environment.Features; using Orchard.Logging; +using Orchard.Tasks.Locking; namespace Orchard.Data.Migration { /// @@ -11,13 +12,16 @@ namespace Orchard.Data.Migration { public class AutomaticDataMigrations : IOrchardShellEvents { private readonly IDataMigrationManager _dataMigrationManager; private readonly IFeatureManager _featureManager; + private readonly IDistributedLockService _distributedLockService; public AutomaticDataMigrations( IDataMigrationManager dataMigrationManager, - IFeatureManager featureManager - ) { + IFeatureManager featureManager, + IDistributedLockService distributedLockService) { + _dataMigrationManager = dataMigrationManager; _featureManager = featureManager; + _distributedLockService = distributedLockService; Logger = NullLogger.Instance; } @@ -25,24 +29,28 @@ namespace Orchard.Data.Migration { public ILogger Logger { get; set; } public void Activated() { + IDistributedLock @lock; + if (_distributedLockService.TryAcquireLock(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); + } - 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); - } - 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 e) { + Logger.Error("Could not run migrations automatically on " + feature, e); + } + } } } } diff --git a/src/Orchard/Tasks/Locking/DefaultLock.cs b/src/Orchard/Tasks/Locking/DefaultLock.cs index adbf1dd36..df4ec1e7a 100644 --- a/src/Orchard/Tasks/Locking/DefaultLock.cs +++ b/src/Orchard/Tasks/Locking/DefaultLock.cs @@ -1,14 +1,31 @@ using System; namespace Orchard.Tasks.Locking { + public class DefaultLock : IDistributedLock { + private readonly IStaticLockSemaphore _semaphore; + + public DefaultLock(IStaticLockSemaphore semaphore) { + _semaphore = semaphore; + } public bool TryAcquire(string name, TimeSpan maxLifetime) { - return true; + if (_semaphore.IsAcquired) + return false; + + return _semaphore.IsAcquired = true; } public void Dispose() { - // Noop. + _semaphore.IsAcquired = false; + } + + public interface IStaticLockSemaphore : ISingletonDependency { + bool IsAcquired { get; set; } + } + + public class StaticLockSemaphore : IStaticLockSemaphore { + public bool IsAcquired { get; set; } } } } \ No newline at end of file From 77b8df516ae2cdd16b2ca6987b519d299c3898b0 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Tue, 11 Aug 2015 16:43:48 +0100 Subject: [PATCH 15/28] Improved locking code. - Moved DatabaseLock into Framework and making it the default lock provider. - Removed unnecessary thread synchronization code. - Allowing multiple lock acquisitions from the same machine (simulating a "lease"). - Updated unit tests. --- .../Orchard.Tests.Modules.csproj | 1 - .../Orchard.Framework.Tests.csproj | 1 + .../Stubs/StubDistributedLock.cs | 12 ++++-- .../Stubs/StubMachineNameProvider.cs | 6 ++- .../Tasks}/DatabaseLockTests.cs | 21 +++++----- .../Tasks/DistributedLockServiceTests.cs | 28 ++++++++++--- .../Modules/Orchard.TaskLease/Migrations.cs | 9 ----- .../Modules/Orchard.TaskLease/Module.txt | 3 +- .../Orchard.TaskLease.csproj | 2 - src/Orchard.sln | 2 +- .../Data/Migration/AutomaticDataMigrations.cs | 2 +- src/Orchard/Orchard.Framework.csproj | 10 +++-- src/Orchard/Tasks/Locking/DefaultLock.cs | 31 -------------- .../Locking/Migrations/FrameworkMigrations.cs | 17 ++++++++ .../Tasks/Locking/Providers}/DatabaseLock.cs | 16 ++++---- .../Locking/Records}/DatabaseLockRecord.cs | 3 +- .../{ => Services}/DistributedLockService.cs | 40 ++++++++----------- .../{ => Services}/IDistributedLock.cs | 5 ++- .../{ => Services}/IDistributedLockService.cs | 2 +- 19 files changed, 106 insertions(+), 105 deletions(-) rename src/{Orchard.Tests.Modules/TaskLease => Orchard.Tests/Tasks}/DatabaseLockTests.cs (62%) delete mode 100644 src/Orchard/Tasks/Locking/DefaultLock.cs create mode 100644 src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs rename src/{Orchard.Web/Modules/Orchard.TaskLease/Services => Orchard/Tasks/Locking/Providers}/DatabaseLock.cs (88%) rename src/{Orchard.Web/Modules/Orchard.TaskLease/Models => Orchard/Tasks/Locking/Records}/DatabaseLockRecord.cs (68%) rename src/Orchard/Tasks/Locking/{ => Services}/DistributedLockService.cs (57%) rename src/Orchard/Tasks/Locking/{ => Services}/IDistributedLock.cs (73%) rename src/Orchard/Tasks/Locking/{ => Services}/IDistributedLockService.cs (97%) diff --git a/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj b/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj index 3fe8e2bff..103e102d1 100644 --- a/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj +++ b/src/Orchard.Tests.Modules/Orchard.Tests.Modules.csproj @@ -182,7 +182,6 @@ - diff --git a/src/Orchard.Tests/Orchard.Framework.Tests.csproj b/src/Orchard.Tests/Orchard.Framework.Tests.csproj index ca06a5084..ad5b95db0 100644 --- a/src/Orchard.Tests/Orchard.Framework.Tests.csproj +++ b/src/Orchard.Tests/Orchard.Framework.Tests.csproj @@ -292,6 +292,7 @@ + diff --git a/src/Orchard.Tests/Stubs/StubDistributedLock.cs b/src/Orchard.Tests/Stubs/StubDistributedLock.cs index 341bd3732..6944b2bc6 100644 --- a/src/Orchard.Tests/Stubs/StubDistributedLock.cs +++ b/src/Orchard.Tests/Stubs/StubDistributedLock.cs @@ -1,23 +1,27 @@ using System; -using Orchard.Tasks.Locking; +using Orchard.Tasks.Locking.Services; namespace Orchard.Tests.Stubs { public class StubDistributedLock : IDistributedLock { public static bool IsAcquired { get; private set; } + public static string AcquiredByMachineName { get; private set; } public bool IsDisposed { get; private set; } - public bool TryAcquire(string name, TimeSpan maxLifetime) { - if (IsAcquired) + public bool TryAcquire(string name, string machineName, TimeSpan maxLifetime) { + if (IsAcquired && machineName != AcquiredByMachineName) return false; - return IsAcquired = true; + IsAcquired = true; + AcquiredByMachineName = machineName; + return true; } public void Dispose() { IsDisposed = true; IsAcquired = false; + AcquiredByMachineName = null; } } } diff --git a/src/Orchard.Tests/Stubs/StubMachineNameProvider.cs b/src/Orchard.Tests/Stubs/StubMachineNameProvider.cs index 5d4b49638..c453ac6f7 100644 --- a/src/Orchard.Tests/Stubs/StubMachineNameProvider.cs +++ b/src/Orchard.Tests/Stubs/StubMachineNameProvider.cs @@ -2,8 +2,12 @@ namespace Orchard.Tests.Stubs { public class StubMachineNameProvider : IMachineNameProvider { + public StubMachineNameProvider() { + MachineName = "Orchard Machine"; + } + public string MachineName { get; set; } public string GetMachineName() { - return "Orchard Machine"; + return MachineName; } } } diff --git a/src/Orchard.Tests.Modules/TaskLease/DatabaseLockTests.cs b/src/Orchard.Tests/Tasks/DatabaseLockTests.cs similarity index 62% rename from src/Orchard.Tests.Modules/TaskLease/DatabaseLockTests.cs rename to src/Orchard.Tests/Tasks/DatabaseLockTests.cs index 5cf9611b8..ae7114626 100644 --- a/src/Orchard.Tests.Modules/TaskLease/DatabaseLockTests.cs +++ b/src/Orchard.Tests/Tasks/DatabaseLockTests.cs @@ -4,12 +4,15 @@ using System.Linq; using Autofac; using NUnit.Framework; using Orchard.Data; -using Orchard.TaskLease.Models; -using Orchard.TaskLease.Services; +using Orchard.Tasks.Locking.Providers; +using Orchard.Tasks.Locking.Records; -namespace Orchard.Tests.Modules.TaskLease { +namespace Orchard.Tests.Tasks { [TestFixture] public class DatabaseLockTests : DatabaseEnabledTestsBase { + private const string MachineName1 = "Orchard Testing Machine 1"; + private const string MachineName2 = "Orchard Testing Machine 2"; + private const string LockName = "Orchard Test Lock"; private IRepository _databaseLockRepository; private DatabaseLock _lock; @@ -32,7 +35,7 @@ namespace Orchard.Tests.Modules.TaskLease { [Test] public void AcquiringLockSucceeds() { - var lockAcquired = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + var lockAcquired = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); Assert.That(lockAcquired, Is.True); Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(1)); @@ -40,15 +43,15 @@ namespace Orchard.Tests.Modules.TaskLease { [Test] public void DisposingRemovesLockRecord() { - _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); _lock.Dispose(); Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(0)); } [Test] public void AcquiringLockTwiceFails() { - var attempt1 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); - var attempt2 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); + var attempt1 = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); + var attempt2 = _lock.TryAcquire(LockName, MachineName2, TimeSpan.FromSeconds(60)); Assert.That(attempt1, Is.True); Assert.That(attempt2, Is.False); @@ -56,8 +59,8 @@ namespace Orchard.Tests.Modules.TaskLease { [Test] public void AcquiringExpiredLockSucceeds() { - var attempt1 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(60)); - var attempt2 = _lock.TryAcquire("Test", TimeSpan.FromSeconds(-1)); // Treat the previosuly stored lock as immediately expired. + var attempt1 = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); + var attempt2 = _lock.TryAcquire(LockName, MachineName2, TimeSpan.FromSeconds(-1)); // Treat the previosuly stored lock as immediately expired. Assert.That(attempt1, Is.True); Assert.That(attempt2, Is.True); diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index a559540c1..7e9f547fe 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -5,17 +5,19 @@ using System.Threading.Tasks; using Autofac; using NUnit.Framework; using Orchard.Environment; -using Orchard.Tasks.Locking; +using Orchard.Tasks.Locking.Services; using Orchard.Tests.Stubs; namespace Orchard.Tests.Tasks { [TestFixture] public class DistributedLockServiceTests : ContainerTestBase { + private const string LockName = "Orchard Test Lock"; private DistributedLockService _distributedLockService; + private StubMachineNameProvider _stubMachineNameProvider; protected override void Register(ContainerBuilder builder) { builder.RegisterType().As(); - builder.RegisterType().As(); + builder.RegisterType().As().SingleInstance(); builder.RegisterType().As(); builder.RegisterType().AsSelf(); builder.RegisterInstance(new Work(resolve => _container.Resolve())).AsSelf(); @@ -23,22 +25,35 @@ namespace Orchard.Tests.Tasks { protected override void Resolve(ILifetimeScope container) { _distributedLockService = container.Resolve(); + _stubMachineNameProvider = (StubMachineNameProvider)container.Resolve(); } [Test] public void AcquiringLockSucceeds() { IDistributedLock @lock; - var lockAcquired = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + var lockAcquired = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); Assert.That(lockAcquired, Is.True); } + [Test] + public void AcquiringLockTwiceOnSameMachineSucceeds() { + IDistributedLock @lock; + var attempt1 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + var attempt2 = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + + Assert.That(attempt1, Is.True); + Assert.That(attempt2, Is.True); + } + [Test] public void AcquiringLockTwiceFails() { IDistributedLock @lock; - var attempt1 = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); - var attempt2 = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @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); Assert.That(attempt1, Is.True); Assert.That(attempt2, Is.False); @@ -52,7 +67,8 @@ namespace Orchard.Tests.Tasks { foreach (var index in Enumerable.Range(0, 10).AsParallel()) { var task = Task.Factory.StartNew(() => { IDistributedLock @lock; - var attempt = _distributedLockService.TryAcquireLock("Test", TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); + _stubMachineNameProvider.MachineName = "Orchard Test Machine " + (index + 1); + var attempt = _distributedLockService.TryAcquireLock(LockName, TimeSpan.FromSeconds(60), TimeSpan.Zero, out @lock); attempts.Add(attempt); }); diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs index 464894eb5..c6797e8fb 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Migrations.cs @@ -17,14 +17,5 @@ namespace Orchard.TaskLease { return 1; } - - public int UpdateFrom1() { - SchemaBuilder.CreateTable("DatabaseLockRecord", table => table - .Column("Id", column => column.PrimaryKey().Identity()) - .Column("Name", column => column.NotNull().WithLength(256)) - .Column("AcquiredUtc")); - - return 2; - } } } \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Module.txt b/src/Orchard.Web/Modules/Orchard.TaskLease/Module.txt index ad76f427e..7748ef35a 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Module.txt +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Module.txt @@ -4,5 +4,6 @@ Author: The Orchard Team Website: http://orchardtasklease.codeplex.com Version: 1.9.1 OrchardVersion: 1.9 -Description: Provides services to synchronize tasks in a farm environment +LifecycleStatus: Deprecated +Description: Provides services to synchronize tasks in a farm environment. Category: Hosting \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj b/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj index 7d5361a83..900973bf8 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Orchard.TaskLease.csproj @@ -76,7 +76,6 @@ - @@ -86,7 +85,6 @@ - diff --git a/src/Orchard.sln b/src/Orchard.sln index e01ab113d..40a51d6f3 100644 --- a/src/Orchard.sln +++ b/src/Orchard.sln @@ -1133,7 +1133,7 @@ Global {642A49D7-8752-4177-80D6-BFBBCFAD3DE0} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5} {966EC390-3C7F-4D98-92A6-F0F30D02E9B1} = {902528F6-1444-42A3-8B75-A54B775B539C} {3158C928-888C-4A84-8BC1-4A8257489538} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5} - {3F72A4E9-7B72-4260-B010-C16EC54F9BAF} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5} + {3F72A4E9-7B72-4260-B010-C16EC54F9BAF} = {902528F6-1444-42A3-8B75-A54B775B539C} {E07AFA7E-7B36-44C3-A537-AFCCAA93EA7A} = {74E681ED-FECC-4034-B9BD-01B0BB1BDECA} {5531E894-D259-45A3-AA61-26DBE720C1CE} = {E9C9F120-07BA-4DFB-B9C3-3AFB9D44C9D5} {2969635F-D9C3-4D01-890D-437B46659690} = {74E681ED-FECC-4034-B9BD-01B0BB1BDECA} diff --git a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs index 70cb78408..aa55ff106 100644 --- a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs +++ b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs @@ -3,7 +3,7 @@ using System.Linq; using Orchard.Environment; using Orchard.Environment.Features; using Orchard.Logging; -using Orchard.Tasks.Locking; +using Orchard.Tasks.Locking.Services; namespace Orchard.Data.Migration { /// diff --git a/src/Orchard/Orchard.Framework.csproj b/src/Orchard/Orchard.Framework.csproj index d919e2a68..34f3d91ae 100644 --- a/src/Orchard/Orchard.Framework.csproj +++ b/src/Orchard/Orchard.Framework.csproj @@ -397,10 +397,12 @@ - - - - + + + + + + diff --git a/src/Orchard/Tasks/Locking/DefaultLock.cs b/src/Orchard/Tasks/Locking/DefaultLock.cs deleted file mode 100644 index df4ec1e7a..000000000 --- a/src/Orchard/Tasks/Locking/DefaultLock.cs +++ /dev/null @@ -1,31 +0,0 @@ -using System; - -namespace Orchard.Tasks.Locking { - - public class DefaultLock : IDistributedLock { - private readonly IStaticLockSemaphore _semaphore; - - public DefaultLock(IStaticLockSemaphore semaphore) { - _semaphore = semaphore; - } - - public bool TryAcquire(string name, TimeSpan maxLifetime) { - if (_semaphore.IsAcquired) - return false; - - return _semaphore.IsAcquired = true; - } - - public void Dispose() { - _semaphore.IsAcquired = false; - } - - public interface IStaticLockSemaphore : ISingletonDependency { - bool IsAcquired { get; set; } - } - - public class StaticLockSemaphore : IStaticLockSemaphore { - public bool IsAcquired { get; set; } - } - } -} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs new file mode 100644 index 000000000..31d831b63 --- /dev/null +++ b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs @@ -0,0 +1,17 @@ +using System; +using Orchard.Data.Migration; + +namespace Orchard.Tasks.Locking.Migrations { + public class FrameworkMigrations : DataMigrationImpl { + + public int Create() { + SchemaBuilder.CreateTable("DatabaseLockRecord", table => table + .Column("Id", column => column.PrimaryKey().Identity()) + .Column("Name", column => column.NotNull().WithLength(256)) + .Column("MachineName", column => column.WithLength(256)) + .Column("AcquiredUtc")); + + return 1; + } + } +} \ No newline at end of file diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs b/src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs similarity index 88% rename from src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs rename to src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs index bb251d9a2..28553bd52 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/DatabaseLock.cs +++ b/src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs @@ -3,20 +3,18 @@ using System.Data; using System.Linq; using Autofac; using Orchard.Data; -using Orchard.Environment.Extensions; using Orchard.Exceptions; using Orchard.Logging; using Orchard.Services; -using Orchard.TaskLease.Models; -using Orchard.Tasks.Locking; +using Orchard.Tasks.Locking.Records; +using Orchard.Tasks.Locking.Services; using Orchard.Validation; -namespace Orchard.TaskLease.Services { +namespace Orchard.Tasks.Locking.Providers { /// /// Provides a database driven implementation of /// - [OrchardSuppressDependency("Orchard.Tasks.Locking.DefaultLock")] public class DatabaseLock : IDistributedLock { private readonly ILifetimeScope _lifetimeScope; private readonly IClock _clock; @@ -34,7 +32,7 @@ namespace Orchard.TaskLease.Services { public ILogger Logger { get; set; } - public bool TryAcquire(string name, TimeSpan maxLifetime) { + public bool TryAcquire(string name, string machineName, TimeSpan maxLifetime) { if(_isDisposed) throw new ObjectDisposedException("DatabaseLock"); @@ -53,9 +51,10 @@ namespace Orchard.TaskLease.Services { var record = repository.Table.FirstOrDefault(x => x.Name == name); if (record != null) { - // There is a nexisting lock, but check if it has expired. + // There is an existing lock, but check if it has expired or if the current machine is requesting the lock. var isExpired = record.AcquiredUtc + maxLifetime < _clock.UtcNow; - if (isExpired) { + var isCurrentMachine = record.MachineName == machineName; + if (isExpired || isCurrentMachine) { repository.Delete(record); record = null; } @@ -66,6 +65,7 @@ namespace Orchard.TaskLease.Services { if (canAcquire) { record = new DatabaseLockRecord { Name = name, + MachineName = machineName, AcquiredUtc = _clock.UtcNow }; repository.Create(record); diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Models/DatabaseLockRecord.cs b/src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs similarity index 68% rename from src/Orchard.Web/Modules/Orchard.TaskLease/Models/DatabaseLockRecord.cs rename to src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs index 039e5d0d8..c8bf06828 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Models/DatabaseLockRecord.cs +++ b/src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs @@ -1,9 +1,10 @@ using System; -namespace Orchard.TaskLease.Models { +namespace Orchard.Tasks.Locking.Records { public class DatabaseLockRecord { public virtual int Id { get; set; } public virtual string Name { get; set; } + public virtual string MachineName { get; set; } public virtual DateTime? AcquiredUtc { get; set; } } } \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/DistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs similarity index 57% rename from src/Orchard/Tasks/Locking/DistributedLockService.cs rename to src/Orchard/Tasks/Locking/Services/DistributedLockService.cs index 7322fc854..c1566a782 100644 --- a/src/Orchard/Tasks/Locking/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -4,9 +4,8 @@ using System.Threading.Tasks; using Orchard.Environment; using Orchard.Logging; -namespace Orchard.Tasks.Locking { +namespace Orchard.Tasks.Locking.Services { public class DistributedLockService : IDistributedLockService { - private readonly ReaderWriterLockSlim _rwl = new ReaderWriterLockSlim(); private readonly IWorkContextAccessor _wca; private readonly IMachineNameProvider _machineNameProvider; @@ -22,31 +21,26 @@ namespace Orchard.Tasks.Locking { var machineName = _machineNameProvider.GetMachineName(); @lock = Resolve(); - if (_rwl.TryEnterWriteLock(0)) { - try { - var waitedTime = TimeSpan.Zero; - var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); - bool acquired; + try { + var waitedTime = TimeSpan.Zero; + var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); + bool acquired; - while (!(acquired = @lock.TryAcquire(name, maxLifetime)) && waitedTime < timeout) { - Task.Delay(timeout).ContinueWith(t => { - waitedTime += waitTime; - }).Wait(); - } + while (!(acquired = @lock.TryAcquire(name, machineName, maxLifetime)) && waitedTime < timeout) { + Task.Delay(timeout).ContinueWith(t => { + waitedTime += waitTime; + }).Wait(); + } - if (acquired) { - Logger.Debug("Successfully acquired a lock named {0} on machine {1}.", name, machineName); - return true; - } - } - catch (Exception ex) { - Logger.Error(ex, "Error while trying to acquire a lock named {0} on machine {1}.", name, machineName); - throw; - } - finally { - _rwl.ExitWriteLock(); + if (acquired) { + Logger.Debug("Successfully acquired a lock named {0} on machine {1}.", name, machineName); + return true; } } + catch (Exception ex) { + Logger.Error(ex, "Error while trying to acquire a lock named {0} on machine {1}.", name, machineName); + throw; + } Logger.Debug("Could not acquire a lock named {0} on machine {1}.", name, machineName); return false; diff --git a/src/Orchard/Tasks/Locking/IDistributedLock.cs b/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs similarity index 73% rename from src/Orchard/Tasks/Locking/IDistributedLock.cs rename to src/Orchard/Tasks/Locking/Services/IDistributedLock.cs index dd4655daf..dcbc2e7b9 100644 --- a/src/Orchard/Tasks/Locking/IDistributedLock.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs @@ -1,6 +1,6 @@ using System; -namespace Orchard.Tasks.Locking { +namespace Orchard.Tasks.Locking.Services { /// /// Provides a lock on a provided name. /// @@ -9,8 +9,9 @@ namespace Orchard.Tasks.Locking { /// Tries to acquire a lock on the specified name. /// /// The name to use for the lock. + /// The machine name trying to acquire a 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 true if a lock was acquired, false otherwise. - bool TryAcquire(string name, TimeSpan maxLifetime); + bool TryAcquire(string name, string machineName, TimeSpan maxLifetime); } } \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/IDistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs similarity index 97% rename from src/Orchard/Tasks/Locking/IDistributedLockService.cs rename to src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs index 65aa7bf9a..98e1992e0 100644 --- a/src/Orchard/Tasks/Locking/IDistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs @@ -1,6 +1,6 @@ using System; -namespace Orchard.Tasks.Locking { +namespace Orchard.Tasks.Locking.Services { /// /// Provides distributed locking functionality. /// From 61f1c3ae00aa3f8a25c7e6a5990ac4b565fca665 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Wed, 19 Aug 2015 23:05:39 +0100 Subject: [PATCH 16/28] Simplified distributed locking implementation and did the following: - Added Support for re-entry - Made TryAcquireLock method thread-safe. - Changed the way locks are released by decreasing their ref count. A lock is expired if: a) ref count == 0 or b) ExpiredUtc lies in the past. It is no longer necessary to delete a lock record (but would be fine, maybe as a background task). --- .../Orchard.Framework.Tests.csproj | 2 +- .../Stubs/StubDistributedLock.cs | 20 +-- src/Orchard.Tests/Tasks/DatabaseLockTests.cs | 69 -------- .../Tasks/DistributedLockServiceTests.cs | 50 +++++- src/Orchard.Tests/Tasks/LockTests.cs | 40 +++++ .../Data/Migration/AutomaticDataMigrations.cs | 40 ++--- src/Orchard/Orchard.Framework.csproj | 4 +- .../Locking/Migrations/FrameworkMigrations.cs | 8 +- .../Tasks/Locking/Providers/DatabaseLock.cs | 125 -------------- .../Locking/Records/DatabaseLockRecord.cs | 10 -- .../Tasks/Locking/Records/LockRecord.cs | 12 ++ .../Services/DistributedLockService.cs | 156 ++++++++++++++---- .../Locking/Services/IDistributedLock.cs | 14 +- .../Services/IDistributedLockService.cs | 5 + src/Orchard/Tasks/Locking/Services/Lock.cs | 31 ++++ 15 files changed, 292 insertions(+), 294 deletions(-) delete mode 100644 src/Orchard.Tests/Tasks/DatabaseLockTests.cs create mode 100644 src/Orchard.Tests/Tasks/LockTests.cs delete mode 100644 src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs delete mode 100644 src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs create mode 100644 src/Orchard/Tasks/Locking/Records/LockRecord.cs create 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 ad5b95db0..7f39003cf 100644 --- a/src/Orchard.Tests/Orchard.Framework.Tests.csproj +++ b/src/Orchard.Tests/Orchard.Framework.Tests.csproj @@ -292,7 +292,7 @@ - + diff --git a/src/Orchard.Tests/Stubs/StubDistributedLock.cs b/src/Orchard.Tests/Stubs/StubDistributedLock.cs index 6944b2bc6..e02d7883d 100644 --- a/src/Orchard.Tests/Stubs/StubDistributedLock.cs +++ b/src/Orchard.Tests/Stubs/StubDistributedLock.cs @@ -3,25 +3,13 @@ using Orchard.Tasks.Locking.Services; namespace Orchard.Tests.Stubs { public class StubDistributedLock : IDistributedLock { - public static bool IsAcquired { get; private set; } - public static string AcquiredByMachineName { get; private set; } - public bool IsDisposed { get; private set; } - - public bool TryAcquire(string name, string machineName, TimeSpan maxLifetime) { - if (IsAcquired && machineName != AcquiredByMachineName) - return false; - - IsAcquired = true; - AcquiredByMachineName = machineName; - return true; - } - - + public void Dispose() { IsDisposed = true; - IsAcquired = false; - AcquiredByMachineName = null; } + + public int Id { get; set; } + public string Name { get; set; } } } diff --git a/src/Orchard.Tests/Tasks/DatabaseLockTests.cs b/src/Orchard.Tests/Tasks/DatabaseLockTests.cs deleted file mode 100644 index ae7114626..000000000 --- a/src/Orchard.Tests/Tasks/DatabaseLockTests.cs +++ /dev/null @@ -1,69 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using Autofac; -using NUnit.Framework; -using Orchard.Data; -using Orchard.Tasks.Locking.Providers; -using Orchard.Tasks.Locking.Records; - -namespace Orchard.Tests.Tasks { - [TestFixture] - public class DatabaseLockTests : DatabaseEnabledTestsBase { - private const string MachineName1 = "Orchard Testing Machine 1"; - private const string MachineName2 = "Orchard Testing Machine 2"; - private const string LockName = "Orchard Test Lock"; - private IRepository _databaseLockRepository; - private DatabaseLock _lock; - - protected override IEnumerable DatabaseTypes { - get { - yield return typeof(DatabaseLockRecord); - } - } - - public override void Register(ContainerBuilder builder) { - builder.RegisterType().AsSelf(); - } - - public override void Init() { - base.Init(); - - _databaseLockRepository = _container.Resolve>(); - _lock = _container.Resolve(); - } - - [Test] - public void AcquiringLockSucceeds() { - var lockAcquired = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); - - Assert.That(lockAcquired, Is.True); - Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(1)); - } - - [Test] - public void DisposingRemovesLockRecord() { - _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); - _lock.Dispose(); - Assert.That(_databaseLockRepository.Table.Count(), Is.EqualTo(0)); - } - - [Test] - public void AcquiringLockTwiceFails() { - var attempt1 = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); - var attempt2 = _lock.TryAcquire(LockName, MachineName2, TimeSpan.FromSeconds(60)); - - Assert.That(attempt1, Is.True); - Assert.That(attempt2, Is.False); - } - - [Test] - public void AcquiringExpiredLockSucceeds() { - var attempt1 = _lock.TryAcquire(LockName, MachineName1, TimeSpan.FromSeconds(60)); - var attempt2 = _lock.TryAcquire(LockName, MachineName2, TimeSpan.FromSeconds(-1)); // Treat the previosuly stored lock as immediately expired. - - Assert.That(attempt1, Is.True); - Assert.That(attempt2, Is.True); - } - } -} \ No newline at end of file diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index 7e9f547fe..c7d2bed92 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -4,28 +4,40 @@ using System.Linq; using System.Threading.Tasks; using Autofac; using NUnit.Framework; +using Orchard.Data; using Orchard.Environment; +using Orchard.Services; +using Orchard.Tasks.Locking.Records; using Orchard.Tasks.Locking.Services; using Orchard.Tests.Stubs; namespace Orchard.Tests.Tasks { [TestFixture] - public class DistributedLockServiceTests : ContainerTestBase { + public class DistributedLockServiceTests : DatabaseEnabledTestsBase { private const string LockName = "Orchard Test Lock"; private DistributedLockService _distributedLockService; private StubMachineNameProvider _stubMachineNameProvider; + private IRepository _lockRepository; - protected override void Register(ContainerBuilder builder) { - builder.RegisterType().As(); + protected override IEnumerable DatabaseTypes + { + get { yield return typeof (LockRecord); } + } + + public override void Register(ContainerBuilder builder) { + builder.RegisterType().As(); + //builder.RegisterType().As(); builder.RegisterType().As().SingleInstance(); builder.RegisterType().As(); builder.RegisterType().AsSelf(); builder.RegisterInstance(new Work(resolve => _container.Resolve())).AsSelf(); } - protected override void Resolve(ILifetimeScope container) { - _distributedLockService = container.Resolve(); - _stubMachineNameProvider = (StubMachineNameProvider)container.Resolve(); + public override void Init() { + base.Init(); + _distributedLockService = _container.Resolve(); + _stubMachineNameProvider = (StubMachineNameProvider)_container.Resolve(); + _lockRepository = _container.Resolve>(); } @@ -47,6 +59,30 @@ namespace Orchard.Tests.Tasks { 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); + + var lockRecord = _lockRepository.Get(@lock.Id); + Assert.That(lockRecord.ReferenceCount, 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); + + _distributedLockService.DisposeLock(@lock); + Assert.That(lockRecord.ReferenceCount, Is.EqualTo(1)); + + _distributedLockService.DisposeLock(@lock); + Assert.That(lockRecord.ReferenceCount, Is.EqualTo(0)); + } + [Test] public void AcquiringLockTwiceFails() { IDistributedLock @lock; @@ -64,7 +100,7 @@ namespace Orchard.Tests.Tasks { var attempts = new List(); var tasks = new List(); - foreach (var index in Enumerable.Range(0, 10).AsParallel()) { + foreach (var index in Enumerable.Range(0, 20)) { var task = Task.Factory.StartNew(() => { IDistributedLock @lock; _stubMachineNameProvider.MachineName = "Orchard Test Machine " + (index + 1); diff --git a/src/Orchard.Tests/Tasks/LockTests.cs b/src/Orchard.Tests/Tasks/LockTests.cs new file mode 100644 index 000000000..eaa09e9e4 --- /dev/null +++ b/src/Orchard.Tests/Tasks/LockTests.cs @@ -0,0 +1,40 @@ +using System; +using Autofac; +using Moq; +using NUnit.Framework; +using Orchard.Tasks.Locking.Services; + +namespace Orchard.Tests.Tasks { + [TestFixture] + public class LockTests : ContainerTestBase { + private const string LockName = "Orchard Test Lock"; + private const int LockId = 1; + private Mock _distributedLockServiceMock; + private Lock _lock; + + protected override void Register(ContainerBuilder builder) { + _distributedLockServiceMock = new Mock(); + builder.RegisterInstance(_distributedLockServiceMock.Object); + } + + protected override void Resolve(ILifetimeScope container) { + _lock = new Lock(_distributedLockServiceMock.Object, LockName, LockId); + } + + [Test] + public void DisposeInvokesDistributedLockServiceDisposeLock() { + _lock.Dispose(); + + _distributedLockServiceMock.Verify(service => service.DisposeLock(_lock), Times.Exactly(1)); + } + + [Test] + public void DisposingMultipleTimesInvokesDistributedLockServiceDisposeLockOnce() { + _lock.Dispose(); + _lock.Dispose(); + _lock.Dispose(); + + _distributedLockServiceMock.Verify(service => service.DisposeLock(_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 aa55ff106..26cd8bc21 100644 --- a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs +++ b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs @@ -29,34 +29,34 @@ namespace Orchard.Data.Migration { public ILogger Logger { get; set; } public void Activated() { - IDistributedLock @lock; - if (_distributedLockService.TryAcquireLock(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" - }; + using(var @lock = _distributedLockService.AcquireLock(GetType().FullName, TimeSpan.FromMinutes(30), TimeSpan.FromMilliseconds(250))) { + if (@lock == null) + return; - 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); + // 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); } - - foreach (var feature in _dataMigrationManager.GetFeaturesThatNeedUpdate()) { - try { - _dataMigrationManager.Update(feature); - } - catch (Exception e) { - Logger.Error("Could not run migrations automatically on " + feature, e); - } + catch (Exception e) { + Logger.Error("Could not run migrations automatically on " + feature, e); } } } } public void Terminating() { - + // No-op. } } } diff --git a/src/Orchard/Orchard.Framework.csproj b/src/Orchard/Orchard.Framework.csproj index 34f3d91ae..d75ad7574 100644 --- a/src/Orchard/Orchard.Framework.csproj +++ b/src/Orchard/Orchard.Framework.csproj @@ -398,11 +398,11 @@ - + - + diff --git a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs index 31d831b63..925b98c86 100644 --- a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs +++ b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs @@ -5,11 +5,13 @@ namespace Orchard.Tasks.Locking.Migrations { public class FrameworkMigrations : DataMigrationImpl { public int Create() { - SchemaBuilder.CreateTable("DatabaseLockRecord", table => table + SchemaBuilder.CreateTable("LockRecord", table => table .Column("Id", column => column.PrimaryKey().Identity()) .Column("Name", column => column.NotNull().WithLength(256)) - .Column("MachineName", column => column.WithLength(256)) - .Column("AcquiredUtc")); + .Column("Owner", column => column.WithLength(256)) + .Column("ReferenceCount") + .Column("CreatedUtc") + .Column("ValidUntilUtc")); return 1; } diff --git a/src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs b/src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs deleted file mode 100644 index 28553bd52..000000000 --- a/src/Orchard/Tasks/Locking/Providers/DatabaseLock.cs +++ /dev/null @@ -1,125 +0,0 @@ -using System; -using System.Data; -using System.Linq; -using Autofac; -using Orchard.Data; -using Orchard.Exceptions; -using Orchard.Logging; -using Orchard.Services; -using Orchard.Tasks.Locking.Records; -using Orchard.Tasks.Locking.Services; -using Orchard.Validation; - -namespace Orchard.Tasks.Locking.Providers { - - /// - /// Provides a database driven implementation of - /// - public class DatabaseLock : IDistributedLock { - private readonly ILifetimeScope _lifetimeScope; - private readonly IClock _clock; - private string _name; - private bool _isAcquired; - private int _id; - private bool _isDisposed; - private ILifetimeScope _scope; - - public DatabaseLock(ILifetimeScope lifetimeScope, IClock clock) { - _lifetimeScope = lifetimeScope; - _clock = clock; - Logger = NullLogger.Instance; - } - - public ILogger Logger { get; set; } - - public bool TryAcquire(string name, string machineName, TimeSpan maxLifetime) { - if(_isDisposed) - throw new ObjectDisposedException("DatabaseLock"); - - Argument.ThrowIfNullOrEmpty(name, "name"); - - if (name.Length > 256) - throw new ArgumentException("The lock's name can't be longer than 256 characters."); - - try { - var scope = EnsureLifetimeScope(name); - scope.Resolve().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 = scope.Resolve>(); - var record = repository.Table.FirstOrDefault(x => x.Name == name); - - if (record != null) { - // There is an existing lock, but check if it has expired or if the current machine is requesting the lock. - var isExpired = record.AcquiredUtc + maxLifetime < _clock.UtcNow; - var isCurrentMachine = record.MachineName == machineName; - if (isExpired || isCurrentMachine) { - repository.Delete(record); - record = null; - } - } - - var canAcquire = record == null; - - if (canAcquire) { - record = new DatabaseLockRecord { - Name = name, - MachineName = machineName, - AcquiredUtc = _clock.UtcNow - }; - repository.Create(record); - repository.Flush(); - - _name = name; - _isAcquired = true; - _id = record.Id; - } - - return canAcquire; - } - catch (Exception ex) { - Logger.Error(ex, "An error occurred while trying to acquire a lock."); - DisposeLifetimeScope(); - throw; - } - } - - // This will be called at least and at the latest by the IoC container when the request ends. - public void Dispose() { - if (_scope == null) - return; - - if (!_isDisposed) { - _isDisposed = true; - - if (_isAcquired) { - try { - var repository = _scope.Resolve>(); - var record = repository.Get(_id); - - if (record != null) { - repository.Delete(record); - repository.Flush(); - } - } - catch (Exception ex) { - if (ex.IsFatal()) throw; - Logger.Error(ex, "An non-fatal error occurred while trying to dispose the database lock."); - } - } - - _scope.Dispose(); - } - } - - private ILifetimeScope EnsureLifetimeScope(string name) { - return _scope ?? (_scope = _lifetimeScope.BeginLifetimeScope("Orchard.Tasks.Locking.Database." + name)); - } - - private void DisposeLifetimeScope() { - if (_scope != null) - _scope.Dispose(); - } - } -} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs b/src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs deleted file mode 100644 index c8bf06828..000000000 --- a/src/Orchard/Tasks/Locking/Records/DatabaseLockRecord.cs +++ /dev/null @@ -1,10 +0,0 @@ -using System; - -namespace Orchard.Tasks.Locking.Records { - public class DatabaseLockRecord { - public virtual int Id { get; set; } - public virtual string Name { get; set; } - public virtual string MachineName { get; set; } - public virtual DateTime? AcquiredUtc { get; set; } - } -} \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Records/LockRecord.cs b/src/Orchard/Tasks/Locking/Records/LockRecord.cs new file mode 100644 index 000000000..9cb44d3de --- /dev/null +++ b/src/Orchard/Tasks/Locking/Records/LockRecord.cs @@ -0,0 +1,12 @@ +using System; + +namespace Orchard.Tasks.Locking.Records { + public class LockRecord { + 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 DateTime CreatedUtc { 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 c1566a782..e2211980a 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -1,59 +1,153 @@ using System; +using System.Data; +using System.Linq; using System.Threading; using System.Threading.Tasks; +using Autofac; +using Orchard.Data; using Orchard.Environment; +using Orchard.Exceptions; using Orchard.Logging; +using Orchard.Services; +using Orchard.Tasks.Locking.Records; namespace Orchard.Tasks.Locking.Services { - public class DistributedLockService : IDistributedLockService { - private readonly IWorkContextAccessor _wca; - private readonly IMachineNameProvider _machineNameProvider; - public DistributedLockService(IWorkContextAccessor wca, IMachineNameProvider machineNameProvider) { - _wca = wca; + public class DistributedLockService : IDistributedLockService { + private readonly IMachineNameProvider _machineNameProvider; + private readonly ILifetimeScope _lifetimeScope; + private readonly IClock _clock; + private readonly object _semaphore = new object(); + + public DistributedLockService(IMachineNameProvider machineNameProvider, ILifetimeScope lifetimeScope, IClock clock) { _machineNameProvider = machineNameProvider; + _lifetimeScope = lifetimeScope; + _clock = clock; Logger = NullLogger.Instance; } public ILogger Logger { get; set; } public bool TryAcquireLock(string name, TimeSpan maxLifetime, TimeSpan timeout, out IDistributedLock @lock) { - var machineName = _machineNameProvider.GetMachineName(); - @lock = Resolve(); + lock (_semaphore) { + @lock = default(IDistributedLock); - try { - var waitedTime = TimeSpan.Zero; - var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); - bool acquired; + try { + var waitedTime = TimeSpan.Zero; + var waitTime = TimeSpan.FromMilliseconds(timeout.TotalMilliseconds / 10); + bool acquired; - while (!(acquired = @lock.TryAcquire(name, machineName, maxLifetime)) && waitedTime < timeout) { - Task.Delay(timeout).ContinueWith(t => { - waitedTime += waitTime; - }).Wait(); + while (!(acquired = TryAcquireLockRecord(name, maxLifetime, out @lock)) && waitedTime < timeout) { + Task.Delay(timeout).ContinueWith(t => { + waitedTime += waitTime; + }).Wait(); + } + + if (acquired) { + Logger.Debug("Successfully acquired a lock named {0}.", name); + return true; + } + } + catch (Exception ex) { + Logger.Error(ex, "Error while trying to acquire a lock named {0}.", name); + throw; } - if (acquired) { - Logger.Debug("Successfully acquired a lock named {0} on machine {1}.", name, machineName); - return true; - } + Logger.Debug("Could not acquire a lock named {0}.", name); + return false; } - catch (Exception ex) { - Logger.Error(ex, "Error while trying to acquire a lock named {0} on machine {1}.", name, machineName); - throw; - } - - Logger.Debug("Could not acquire a lock named {0} on machine {1}.", name, machineName); - return false; - } - - private T Resolve() { - var workContext = _wca.GetContext() ?? _wca.CreateWorkContextScope().WorkContext; // In case this is invoked at the end of the request. - return workContext.Resolve(); } 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); + return true; + } + 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); + } + + private ILifetimeScope CreateChildLifetimeScope(string name) { + return _lifetimeScope.BeginLifetimeScope("Orchard.Tasks.Locking." + name); + } } } \ 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 dcbc2e7b9..e38fe5da2 100644 --- a/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLock.cs @@ -2,16 +2,10 @@ namespace Orchard.Tasks.Locking.Services { /// - /// Provides a lock on a provided name. + /// Represents a distributed lock. /// - public interface IDistributedLock : ITransientDependency, IDisposable { - /// - /// Tries to acquire a lock on the specified name. - /// - /// The name to use for the lock. - /// The machine name trying to acquire a 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 true if a lock was acquired, false otherwise. - bool TryAcquire(string name, string machineName, TimeSpan maxLifetime); + 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 98e1992e0..1ba6d9060 100644 --- a/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs @@ -23,5 +23,10 @@ namespace Orchard.Tasks.Locking.Services { /// 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); + + /// + /// Disposes the specified lock. + /// + void DisposeLock(IDistributedLock @lock); } } \ No newline at end of file diff --git a/src/Orchard/Tasks/Locking/Services/Lock.cs b/src/Orchard/Tasks/Locking/Services/Lock.cs new file mode 100644 index 000000000..21a2eca42 --- /dev/null +++ b/src/Orchard/Tasks/Locking/Services/Lock.cs @@ -0,0 +1,31 @@ +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 From 6c712a984027f8f3f143a499705e3133d11ca1c9 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Fri, 21 Aug 2015 15:22:32 +0100 Subject: [PATCH 17/28] 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 From 81bf5d94fd253912204da4c645eaf1f8f1f5fe2f Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Fri, 21 Aug 2015 19:35:42 +0100 Subject: [PATCH 18/28] Updated usages of ITaskLeaseService with IDistributedLockService. --- .../AuditTrailTrimmingBackgroundTask.cs | 31 +-- .../Services/Jobs/JobProcessor.cs | 188 +++++++++--------- .../Services/JobsQueueProcessor.cs | 30 +-- .../Services/ITaskLeaseService.cs | 2 +- .../Services/TaskLeaseService.cs | 2 +- src/Orchard/Environment/OrchardStarter.cs | 2 +- 6 files changed, 128 insertions(+), 127 deletions(-) diff --git a/src/Orchard.Web/Modules/Orchard.AuditTrail/Services/AuditTrailTrimmingBackgroundTask.cs b/src/Orchard.Web/Modules/Orchard.AuditTrail/Services/AuditTrailTrimmingBackgroundTask.cs index 83da1ae69..1a648ed78 100644 --- a/src/Orchard.Web/Modules/Orchard.AuditTrail/Services/AuditTrailTrimmingBackgroundTask.cs +++ b/src/Orchard.Web/Modules/Orchard.AuditTrail/Services/AuditTrailTrimmingBackgroundTask.cs @@ -7,8 +7,8 @@ using Orchard.Environment.Extensions; using Orchard.Logging; using Orchard.Services; using Orchard.Settings; -using Orchard.TaskLease.Services; using Orchard.Tasks; +using Orchard.Tasks.Locking.Services; namespace Orchard.AuditTrail.Services { [OrchardFeature("Orchard.AuditTrail.Trimming")] @@ -16,19 +16,19 @@ namespace Orchard.AuditTrail.Services { private static readonly object _sweepLock = new object(); private readonly ISiteService _siteService; private readonly IClock _clock; - private readonly ITaskLeaseService _taskLeaseService; private readonly IAuditTrailManager _auditTrailManager; + private readonly IDistributedLockService _distributedLockService; public AuditTrailTrimmingBackgroundTask( ISiteService siteService, IClock clock, - ITaskLeaseService taskLeaseService, - IAuditTrailManager auditTrailManager) { + IAuditTrailManager auditTrailManager, + IDistributedLockService distributedLockService) { _siteService = siteService; _clock = clock; - _taskLeaseService = taskLeaseService; _auditTrailManager = auditTrailManager; + _distributedLockService = distributedLockService; } public AuditTrailTrimmingSettingsPart Settings { @@ -41,17 +41,20 @@ namespace Orchard.AuditTrail.Services { Logger.Debug("Beginning sweep."); // Only allow this task to run on one farm node at a time. - if (_taskLeaseService.Acquire(GetType().FullName, _clock.UtcNow.AddHours(1)) != null) { + DistributedLock @lock; + if (_distributedLockService.TryAcquireLockForMachine(GetType().FullName, TimeSpan.FromHours(1), out @lock)) { + using (@lock) { - // We don't need to check the audit trail for events to remove every minute. Let's stick with twice a day. - if (!GetIsTimeToTrim()) - return; + // We don't need to check the audit trail for events to remove every minute. Let's stick with twice a day. + if (!GetIsTimeToTrim()) + return; - Logger.Debug("Starting audit trail trimming."); - var deletedRecords = _auditTrailManager.Trim(TimeSpan.FromDays(Settings.RetentionPeriod)); - Logger.Debug("Audit trail trimming completed. {0} records were deleted.", deletedRecords.Count()); - Settings.LastRunUtc = _clock.UtcNow; - } + Logger.Debug("Starting audit trail trimming."); + var deletedRecords = _auditTrailManager.Trim(TimeSpan.FromDays(Settings.RetentionPeriod)); + Logger.Debug("Audit trail trimming completed. {0} records were deleted.", deletedRecords.Count()); + Settings.LastRunUtc = _clock.UtcNow; + } + } } catch (Exception ex) { Logger.Error(ex, "Error during sweep."); diff --git a/src/Orchard.Web/Modules/Orchard.Azure.MediaServices/Services/Jobs/JobProcessor.cs b/src/Orchard.Web/Modules/Orchard.Azure.MediaServices/Services/Jobs/JobProcessor.cs index 4a73fd266..4a066e68a 100644 --- a/src/Orchard.Web/Modules/Orchard.Azure.MediaServices/Services/Jobs/JobProcessor.cs +++ b/src/Orchard.Web/Modules/Orchard.Azure.MediaServices/Services/Jobs/JobProcessor.cs @@ -3,19 +3,17 @@ using System.Collections.Generic; using System.Linq; using System.Text; using System.Threading; +using Microsoft.WindowsAzure.MediaServices.Client; using Orchard.Azure.MediaServices.Helpers; using Orchard.Azure.MediaServices.Models; using Orchard.Azure.MediaServices.Models.Assets; using Orchard.Azure.MediaServices.Models.Jobs; using Orchard.Azure.MediaServices.Services.Assets; using Orchard.Azure.MediaServices.Services.Wams; -using Microsoft.WindowsAzure.MediaServices.Client; -using Orchard; using Orchard.ContentManagement; using Orchard.Logging; -using Orchard.Services; -using Orchard.TaskLease.Services; using Orchard.Tasks; +using Orchard.Tasks.Locking.Services; namespace Orchard.Azure.MediaServices.Services.Jobs { public class JobProcessor : Component, IBackgroundTask { @@ -23,26 +21,23 @@ namespace Orchard.Azure.MediaServices.Services.Jobs { private static readonly object _sweepLock = new object(); private readonly IWamsClient _wamsClient; - private readonly IClock _clock; - private readonly ITaskLeaseService _taskLeaseService; private readonly IAssetManager _assetManager; private readonly IJobManager _jobManager; private readonly IOrchardServices _orchardServices; + private readonly IDistributedLockService _distributedLockService; public JobProcessor( IWamsClient wamsClient, - IClock clock, - ITaskLeaseService taskLeaseService, IAssetManager assetManager, IJobManager jobManager, - IOrchardServices orchardServices) { + IOrchardServices orchardServices, + IDistributedLockService distributedLockService) { _wamsClient = wamsClient; - _clock = clock; - _taskLeaseService = taskLeaseService; _assetManager = assetManager; _jobManager = jobManager; _orchardServices = orchardServices; + _distributedLockService = distributedLockService; } public void Sweep() { @@ -56,103 +51,106 @@ namespace Orchard.Azure.MediaServices.Services.Jobs { } // Only allow this task to run on one farm node at a time. - if (_taskLeaseService.Acquire(GetType().FullName, _clock.UtcNow.AddHours(1)) != null) { - var jobs = _jobManager.GetActiveJobs().ToDictionary(job => job.WamsJobId); + DistributedLock @lock; + if (_distributedLockService.TryAcquireLockForMachine(GetType().FullName, TimeSpan.FromHours(1), out @lock)) { + using (@lock) { + var jobs = _jobManager.GetActiveJobs().ToDictionary(job => job.WamsJobId); - if (!jobs.Any()) { - Logger.Debug("No open jobs were found; going back to sleep."); - return; - } - - Logger.Information("Beginning processing of {0} open jobs.", jobs.Count()); - - var wamsJobs = _wamsClient.GetJobsById(jobs.Keys); - - foreach (var wamsJob in wamsJobs) { - Logger.Information("Processing job '{0}'...", wamsJob.Name); - - var job = jobs[wamsJob.Id]; - var tasks = job.Tasks.ToDictionary(task => task.WamsTaskId); - var wamsTasks = wamsJob.Tasks.ToArray(); - - foreach (var wamsTask in wamsTasks) { - var task = tasks[wamsTask.Id]; - task.Status = MapWamsJobState(wamsTask.State); - task.PercentComplete = (int)wamsTask.Progress; + if (!jobs.Any()) { + Logger.Debug("No open jobs were found; going back to sleep."); + return; } - var previousStatus = job.Status; - var wamsJobErrors = HarvestWamsJobErrors(wamsJob).ToArray(); + Logger.Information("Beginning processing of {0} open jobs.", jobs.Count()); - job.CreatedUtc = wamsJob.Created; - job.StartedUtc = wamsJob.StartTime; - job.FinishedUtc = wamsJob.EndTime; - job.Status = MapWamsJobState(wamsJob.State); - job.ErrorMessage = GetAggregateErrorMessage(wamsJobErrors); + var wamsJobs = _wamsClient.GetJobsById(jobs.Keys); - LogWamsJobErrors(wamsJobErrors); + foreach (var wamsJob in wamsJobs) { + Logger.Information("Processing job '{0}'...", wamsJob.Name); - if (job.Status != previousStatus) { - if (job.Status == JobStatus.Finished) { - Logger.Information("Job '{0}' was finished in WAMS; creating locators.", wamsJob.Name); + var job = jobs[wamsJob.Id]; + var tasks = job.Tasks.ToDictionary(task => task.WamsTaskId); + var wamsTasks = wamsJob.Tasks.ToArray(); - var lastTask = job.Tasks.Last(); - var lastWamsTask = wamsTasks.Where(task => task.Id == lastTask.WamsTaskId).Single(); - var outputAsset = lastWamsTask.OutputAssets.First(); - var outputAssetName = !String.IsNullOrWhiteSpace(job.OutputAssetName) ? job.OutputAssetName : lastWamsTask.Name; - var outputAssetDescription = job.OutputAssetDescription.TrimSafe(); - var encoderMetadataXml = _wamsClient.GetEncoderMetadataXml(outputAsset).Result; - var cloudVideoPart = job.CloudVideoPart; - var wamsLocators = _wamsClient.CreateLocatorsAsync(outputAsset, WamsLocatorCategory.Private).Result; + foreach (var wamsTask in wamsTasks) { + var task = tasks[wamsTask.Id]; + task.Status = MapWamsJobState(wamsTask.State); + task.PercentComplete = (int)wamsTask.Progress; + } - // HACK: Temporary workaround to disable dynamic packaging for VC1-based assets. In future versions - // this will be implemented more robustly by testing all the dynamic URLs to see which ones work - // and only store and use the working ones. - var forceNonDynamicAsset = lastWamsTask.Configuration.StartsWith("VC1"); + var previousStatus = job.Status; + var wamsJobErrors = HarvestWamsJobErrors(wamsJob).ToArray(); - if (wamsLocators.OnDemandLocator != null && !forceNonDynamicAsset) { - _assetManager.CreateAssetFor(cloudVideoPart, asset => { - asset.IncludeInPlayer = true; - asset.Name = outputAssetName; - asset.Description = outputAssetDescription; - asset.EncodingPreset = lastTask.HarvestAssetName; - asset.WamsPrivateLocatorId = wamsLocators.SasLocator.Id; - asset.WamsPrivateLocatorUrl = wamsLocators.SasLocator.Url; - asset.WamsPrivateOnDemandLocatorId = wamsLocators.OnDemandLocator.Id; - asset.WamsPrivateOnDemandLocatorUrl = wamsLocators.OnDemandLocator.Url; - asset.WamsManifestFilename = wamsLocators.OnDemandManifestFilename; - asset.WamsAssetId = outputAsset.Id; - asset.WamsEncoderMetadataXml = encoderMetadataXml; - asset.UploadState.Status = AssetUploadStatus.Uploaded; - asset.PublishState.Status = AssetPublishStatus.None; - }); - } - else { - _assetManager.CreateAssetFor(cloudVideoPart, asset => { - asset.IncludeInPlayer = true; - asset.Name = outputAssetName; - asset.Description = outputAssetDescription; - asset.EncodingPreset = lastTask.HarvestAssetName; - asset.WamsPrivateLocatorId = wamsLocators.SasLocator.Id; - asset.WamsPrivateLocatorUrl = wamsLocators.SasLocator.Url; - asset.WamsAssetId = outputAsset.Id; - asset.WamsEncoderMetadataXml = encoderMetadataXml; - asset.UploadState.Status = AssetUploadStatus.Uploaded; - asset.PublishState.Status = AssetPublishStatus.None; - }); - } + job.CreatedUtc = wamsJob.Created; + job.StartedUtc = wamsJob.StartTime; + job.FinishedUtc = wamsJob.EndTime; + job.Status = MapWamsJobState(wamsJob.State); + job.ErrorMessage = GetAggregateErrorMessage(wamsJobErrors); - try { - if (cloudVideoPart.IsPublished()) - _assetManager.PublishAssetsFor(cloudVideoPart); - } - catch (Exception ex) { - Logger.Warning(ex, "Processing of job '{0}' was completed but an error occurred while publishing the cloud video item with ID {1} after processing.", wamsJob.Name, cloudVideoPart.Id); + LogWamsJobErrors(wamsJobErrors); + + if (job.Status != previousStatus) { + if (job.Status == JobStatus.Finished) { + Logger.Information("Job '{0}' was finished in WAMS; creating locators.", wamsJob.Name); + + var lastTask = job.Tasks.Last(); + var lastWamsTask = wamsTasks.Single(task => task.Id == lastTask.WamsTaskId); + var outputAsset = lastWamsTask.OutputAssets.First(); + var outputAssetName = !String.IsNullOrWhiteSpace(job.OutputAssetName) ? job.OutputAssetName : lastWamsTask.Name; + var outputAssetDescription = job.OutputAssetDescription.TrimSafe(); + var encoderMetadataXml = _wamsClient.GetEncoderMetadataXml(outputAsset).Result; + var cloudVideoPart = job.CloudVideoPart; + var wamsLocators = _wamsClient.CreateLocatorsAsync(outputAsset, WamsLocatorCategory.Private).Result; + + // HACK: Temporary workaround to disable dynamic packaging for VC1-based assets. In future versions + // this will be implemented more robustly by testing all the dynamic URLs to see which ones work + // and only store and use the working ones. + var forceNonDynamicAsset = lastWamsTask.Configuration.StartsWith("VC1"); + + if (wamsLocators.OnDemandLocator != null && !forceNonDynamicAsset) { + _assetManager.CreateAssetFor(cloudVideoPart, asset => { + asset.IncludeInPlayer = true; + asset.Name = outputAssetName; + asset.Description = outputAssetDescription; + asset.EncodingPreset = lastTask.HarvestAssetName; + asset.WamsPrivateLocatorId = wamsLocators.SasLocator.Id; + asset.WamsPrivateLocatorUrl = wamsLocators.SasLocator.Url; + asset.WamsPrivateOnDemandLocatorId = wamsLocators.OnDemandLocator.Id; + asset.WamsPrivateOnDemandLocatorUrl = wamsLocators.OnDemandLocator.Url; + asset.WamsManifestFilename = wamsLocators.OnDemandManifestFilename; + asset.WamsAssetId = outputAsset.Id; + asset.WamsEncoderMetadataXml = encoderMetadataXml; + asset.UploadState.Status = AssetUploadStatus.Uploaded; + asset.PublishState.Status = AssetPublishStatus.None; + }); + } + else { + _assetManager.CreateAssetFor(cloudVideoPart, asset => { + asset.IncludeInPlayer = true; + asset.Name = outputAssetName; + asset.Description = outputAssetDescription; + asset.EncodingPreset = lastTask.HarvestAssetName; + asset.WamsPrivateLocatorId = wamsLocators.SasLocator.Id; + asset.WamsPrivateLocatorUrl = wamsLocators.SasLocator.Url; + asset.WamsAssetId = outputAsset.Id; + asset.WamsEncoderMetadataXml = encoderMetadataXml; + asset.UploadState.Status = AssetUploadStatus.Uploaded; + asset.PublishState.Status = AssetPublishStatus.None; + }); + } + + try { + if (cloudVideoPart.IsPublished()) + _assetManager.PublishAssetsFor(cloudVideoPart); + } + catch (Exception ex) { + Logger.Warning(ex, "Processing of job '{0}' was completed but an error occurred while publishing the cloud video item with ID {1} after processing.", wamsJob.Name, cloudVideoPart.Id); + } } } - } - Logger.Information("Processing of job '{0}' was successfully completed.", wamsJob.Name); + Logger.Information("Processing of job '{0}' was successfully completed.", wamsJob.Name); + } } } } diff --git a/src/Orchard.Web/Modules/Orchard.JobsQueue/Services/JobsQueueProcessor.cs b/src/Orchard.Web/Modules/Orchard.JobsQueue/Services/JobsQueueProcessor.cs index dd5ddc633..039bfbc75 100644 --- a/src/Orchard.Web/Modules/Orchard.JobsQueue/Services/JobsQueueProcessor.cs +++ b/src/Orchard.Web/Modules/Orchard.JobsQueue/Services/JobsQueueProcessor.cs @@ -5,28 +5,25 @@ using System.Threading; using Newtonsoft.Json.Linq; using Orchard.Environment; using Orchard.Events; -using Orchard.Logging; using Orchard.JobsQueue.Models; -using Orchard.Services; -using Orchard.TaskLease.Services; +using Orchard.Logging; +using Orchard.Tasks.Locking.Services; namespace Orchard.JobsQueue.Services { public class JobsQueueProcessor : IJobsQueueProcessor { private readonly Work _jobsQueueManager; - private readonly Work _clock; - private readonly Work _taskLeaseService; private readonly Work _eventBus; private readonly ReaderWriterLockSlim _rwl = new ReaderWriterLockSlim(); + private readonly IDistributedLockService _distributedLockService; public JobsQueueProcessor( - Work clock, Work jobsQueueManager, - Work taskLeaseService, - Work eventBus) { - _clock = clock; + Work eventBus, + IDistributedLockService distributedLockService) { + _jobsQueueManager = jobsQueueManager; - _taskLeaseService = taskLeaseService; _eventBus = eventBus; + _distributedLockService = distributedLockService; Logger = NullLogger.Instance; } @@ -35,12 +32,15 @@ namespace Orchard.JobsQueue.Services { // prevent two threads on the same machine to process the message queue if (_rwl.TryEnterWriteLock(0)) { try { - if (_taskLeaseService.Value.Acquire("JobsQueueProcessor", _clock.Value.UtcNow.AddMinutes(5)) != null) { - IEnumerable messages; + DistributedLock @lock; + if(_distributedLockService.TryAcquireLockForMachine(GetType().FullName, TimeSpan.FromMinutes(5), out @lock)){ + using (@lock) { + IEnumerable messages; - while ((messages = _jobsQueueManager.Value.GetJobs(0, 10).ToArray()).Any()) { - foreach (var message in messages) { - ProcessMessage(message); + while ((messages = _jobsQueueManager.Value.GetJobs(0, 10).ToArray()).Any()) { + foreach (var message in messages) { + ProcessMessage(message); + } } } } diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs index c32e3d37e..f9eaa7e35 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs @@ -6,7 +6,7 @@ namespace Orchard.TaskLease.Services { /// Describes a service to save and acquire task leases. A task lease can't be acquired by two different machines, /// for a specific amount of time. Optionnally a State can be saved along with the lease. /// - [Obsolete("Use Orchard.Tasks.Locking.IDistributedLockService instead.")] + [Obsolete("Use Orchard.Tasks.Locking.IDistributedLockService.AcquireForMachine instead.")] public interface ITaskLeaseService : IDependency { /// diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs index 398c1092f..5d5fd6179 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs @@ -9,7 +9,7 @@ namespace Orchard.TaskLease.Services { /// /// Provides a database driven implementation of /// - [Obsolete("Use Orchard.Tasks.Locking.DistributedLockService instead.")] + [Obsolete("Use Orchard.Tasks.Locking.DistributedLockService.AcquireForMachine instead.")] public class TaskLeaseService : ITaskLeaseService { private readonly IRepository _repository; diff --git a/src/Orchard/Environment/OrchardStarter.cs b/src/Orchard/Environment/OrchardStarter.cs index 3d3dd6958..1e0db5736 100644 --- a/src/Orchard/Environment/OrchardStarter.cs +++ b/src/Orchard/Environment/OrchardStarter.cs @@ -67,7 +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(); //builder.RegisterType().As().SingleInstance(); RegisterVolatileProvider(builder); From 1cf2b6cf2707c93d374a3263f2443fe0636398f9 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 17:05:42 +0100 Subject: [PATCH 19/28] Corrected message on ObsoleteAttribute applied to ITaskLeaseService/TaskLeaseService. --- .../Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs | 2 +- .../Modules/Orchard.TaskLease/Services/TaskLeaseService.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs index f9eaa7e35..625472446 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/ITaskLeaseService.cs @@ -6,7 +6,7 @@ namespace Orchard.TaskLease.Services { /// Describes a service to save and acquire task leases. A task lease can't be acquired by two different machines, /// for a specific amount of time. Optionnally a State can be saved along with the lease. /// - [Obsolete("Use Orchard.Tasks.Locking.IDistributedLockService.AcquireForMachine instead.")] + [Obsolete("Use Orchard.Tasks.Locking.IDistributedLockService and the AcquireLockForMachine/TryAcquireLockForMachine methods instead.")] public interface ITaskLeaseService : IDependency { /// diff --git a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs index 5d5fd6179..8cf04b3fe 100644 --- a/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs +++ b/src/Orchard.Web/Modules/Orchard.TaskLease/Services/TaskLeaseService.cs @@ -9,7 +9,7 @@ namespace Orchard.TaskLease.Services { /// /// Provides a database driven implementation of /// - [Obsolete("Use Orchard.Tasks.Locking.DistributedLockService.AcquireForMachine instead.")] + [Obsolete("Use Orchard.Tasks.Locking.DistributedLockService and the AcquireLockForMachine/TryAcquireLockForMachine methods instead.")] public class TaskLeaseService : ITaskLeaseService { private readonly IRepository _repository; From 079fb1f84b1565c18e09bda459e6b2419664bffe Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 17:11:03 +0100 Subject: [PATCH 20/28] Narrowed the lock scope to per-thread. --- src/Orchard/Data/Migration/AutomaticDataMigrations.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs index 45b5b6bb6..50727d47f 100644 --- a/src/Orchard/Data/Migration/AutomaticDataMigrations.cs +++ b/src/Orchard/Data/Migration/AutomaticDataMigrations.cs @@ -30,7 +30,7 @@ namespace Orchard.Data.Migration { public void Activated() { DistributedLock @lock; - if(_distributedLockService.TryAcquireLockForMachine(GetType().FullName, TimeSpan.FromMinutes(30), TimeSpan.FromMilliseconds(250), out @lock)) { + if(_distributedLockService.TryAcquireLockForThread(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[] { From 5dcbad22355411b042eafaffaa1dcbd36c4f9af8 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 17:51:34 +0100 Subject: [PATCH 21/28] Formatted code. Moved static members to top of class. --- .../Tasks/Locking/Services/DistributedLock.cs | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLock.cs b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs index cd7143fc1..05977bea4 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLock.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLock.cs @@ -7,23 +7,6 @@ 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, @@ -42,5 +25,22 @@ namespace Orchard.Tasks.Locking.Services { Id = lockId }; } + + 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); + } } } \ No newline at end of file From 3615cbe6db4af0c496dff2c56978f1697b1fa5fd Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 18:04:12 +0100 Subject: [PATCH 22/28] Added table index to DistributedLockRecord. --- src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs index 47e8a42c9..952e4a554 100644 --- a/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs +++ b/src/Orchard/Tasks/Locking/Migrations/FrameworkMigrations.cs @@ -14,6 +14,10 @@ namespace Orchard.Tasks.Locking.Migrations { .Column("CreatedUtc") .Column("ValidUntilUtc")); + SchemaBuilder.AlterTable("DistributedLockRecord", table => { + table.CreateIndex("IDX_DistributedLockRecord_Name_ValidUntilUtc_Count", "Name", "ValidUntilUtc", "Count"); + }); + return 1; } } From 7673e2e12f30b893f812811d5868150c77ff326e Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 18:10:43 +0100 Subject: [PATCH 23/28] Non-functional change but more explicit. Perhaps a bit subjective, but passing in a null value or a zero value controls whether the AcquireLockInternal will block or not, so probably better to provide an explicit Zero value from TryAcquireLock instead. --- src/Orchard/Tasks/Locking/Services/DistributedLockService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs index 1aa581123..2d0392b76 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -72,7 +72,7 @@ namespace Orchard.Tasks.Locking.Services { } private bool TryAcquireLock(string name, TimeSpan maxValidFor, TimeSpan? timeout, string machineName, int? threadId, out DistributedLock @lock) { - @lock = AcquireLockInternal(name, maxValidFor, machineName, threadId, timeout.GetValueOrDefault()); + @lock = AcquireLockInternal(name, maxValidFor, machineName, threadId, timeout ?? TimeSpan.Zero); if (@lock != null) return true; From 5f4cd14937e5e08ff39073d1a7ec76e061e08dfb Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 20:13:46 +0100 Subject: [PATCH 24/28] Changed the lifetime of DB services to match their lifetime scope. This enables tests that involve multi threading and child lifetime scopes. --- src/Orchard.Tests/DatabaseEnabledTestsBase.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Orchard.Tests/DatabaseEnabledTestsBase.cs b/src/Orchard.Tests/DatabaseEnabledTestsBase.cs index b1d782cc0..e2f1fda3c 100644 --- a/src/Orchard.Tests/DatabaseEnabledTestsBase.cs +++ b/src/Orchard.Tests/DatabaseEnabledTestsBase.cs @@ -47,9 +47,10 @@ namespace Orchard.Tests { builder.RegisterType().As(); builder.RegisterInstance(new StubLocator(_session)).As(); builder.RegisterInstance(_clock).As(); - builder.RegisterGeneric(typeof(Repository<>)).As(typeof(IRepository<>)); + builder.RegisterGeneric(typeof(Repository<>)).As(typeof(IRepository<>)).InstancePerLifetimeScope(); builder.RegisterInstance(new ShellSettings { Name = ShellSettings.DefaultName, DataProvider = "SqlCe" }); - builder.RegisterInstance(new TestTransactionManager(_session)).As(); + builder.RegisterType().As().InstancePerLifetimeScope(); + builder.Register(context => _sessionFactory.OpenSession()).As().InstancePerLifetimeScope(); Register(builder); _container = builder.Build(); From c5b0cac24a164e73bf552c72ca441d8eea29cbf1 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 20:15:20 +0100 Subject: [PATCH 25/28] Removed unnecessary lock statements and updated tests. The updated test needs to refresh the lock record entity since the DB record is updated using different connections (from child lifetime scopes). --- .../Tasks/DistributedLockServiceTests.cs | 10 +++- .../Services/DistributedLockService.cs | 57 ++++++++++--------- 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index 6f789d8e4..7828c23fe 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -1,7 +1,9 @@ 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; @@ -18,6 +20,8 @@ namespace Orchard.Tests.Tasks { private StubMachineNameProvider _machineNameProvider; private StubThreadProvider _threadProvider; private IRepository _distributedLockRepository; + private ITransactionManager _transactionManager; + protected override IEnumerable DatabaseTypes { get { yield return typeof(DistributedLockRecord); } @@ -36,6 +40,7 @@ namespace Orchard.Tests.Tasks { _machineNameProvider = (StubMachineNameProvider)_container.Resolve(); _threadProvider = (StubThreadProvider)_container.Resolve(); _distributedLockRepository = _container.Resolve>(); + _transactionManager = _container.Resolve(); } [Test] @@ -75,11 +80,13 @@ namespace Orchard.Tests.Tasks { var lockId = Int32.Parse(@lock.Id); var lockRecord = _distributedLockRepository.Get(lockId); - + _distributedLockService.ReleaseLock(@lock); + _session.Refresh(lockRecord); Assert.That(lockRecord.Count, Is.EqualTo(1)); _distributedLockService.ReleaseLock(@lock); + _session.Refresh(lockRecord); Assert.That(lockRecord.Count, Is.EqualTo(0)); } @@ -193,6 +200,7 @@ namespace Orchard.Tests.Tasks { }; _distributedLockRepository.Create(record); + _transactionManager.RequireNew(); return record; } diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs index 2d0392b76..abf4f462f 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -16,7 +16,6 @@ namespace Orchard.Tasks.Locking.Services { private readonly IMachineNameProvider _machineNameProvider; private readonly ILifetimeScope _lifetimeScope; private readonly IClock _clock; - private readonly object _transactionManagerLock = new object(); private readonly IThreadProvider _threadProvider; public DistributedLockService(IMachineNameProvider machineNameProvider, IThreadProvider threadProvider, ILifetimeScope lifetimeScope, IClock clock) { @@ -43,31 +42,29 @@ namespace Orchard.Tasks.Locking.Services { } public void ReleaseLock(DistributedLock @lock) { - lock (_transactionManagerLock) { - var childLifetimeScope = CreateChildLifetimeScope(@lock.Name); + var childLifetimeScope = CreateChildLifetimeScope(@lock.Name); - try { - var repository = childLifetimeScope.Resolve>(); - var transactionManager = childLifetimeScope.Resolve(); - transactionManager.RequireNew(IsolationLevel.ReadCommitted); - var lockId = Int32.Parse(@lock.Id); - var record = repository.Get(lockId); + try { + var repository = childLifetimeScope.Resolve>(); + var transactionManager = childLifetimeScope.Resolve(); + transactionManager.RequireNew(IsolationLevel.ReadCommitted); + var lockId = Int32.Parse(@lock.Id); + var record = repository.Get(lockId); - if (record == null) - throw new ObjectDisposedException("@lock", "No lock record could be found for the specified lock to be released."); + if (record == null) + throw new ObjectDisposedException("@lock", "No lock record could be found for the specified lock to be released."); - if (record.Count <= 0) - throw new ObjectDisposedException("@lock", "The specified lock has already been released."); + if (record.Count <= 0) + throw new ObjectDisposedException("@lock", "The specified lock has already been released."); - record.Count--; - } - 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(); - } + record.Count--; + } + 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(); } } @@ -76,7 +73,6 @@ namespace Orchard.Tasks.Locking.Services { if (@lock != null) return true; - Logger.Debug("Could not acquire a lock named '{0}'.", name); return false; } @@ -85,7 +81,7 @@ namespace Orchard.Tasks.Locking.Services { if (@lock != null) return @lock; - throw new TimeoutException("Could not acquire a lock within the specified amount of time."); + 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) { @@ -105,11 +101,16 @@ namespace Orchard.Tasks.Locking.Services { throw; } + Logger.Debug(timeout == null + ? "Failed to acquire a lock named '{0}'." + : "Failed to acquire a lock named '{0}' within the specified timeout ('{1}')." + , name, timeout); + return null; } private DistributedLockRecord AcquireLockRecord(string name, TimeSpan maxValidFor, string machineName, int? threadId) { - lock (_transactionManagerLock) { + //lock (_transactionManagerLock) { var childLifetimeScope = CreateChildLifetimeScope(name); try { @@ -130,9 +131,9 @@ namespace Orchard.Tasks.Locking.Services { 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) + if (record.ThreadId != threadId) throw new InvalidOperationException( - threadId == null + 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."); @@ -166,7 +167,7 @@ namespace Orchard.Tasks.Locking.Services { finally { childLifetimeScope.Dispose(); } - } + //} } /// From 86bae087fa92fefd18a43f2990b9143b39b1625c Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 22:55:00 +0100 Subject: [PATCH 26/28] Added support for eternal distributed locks. Eternal locks never expire until they are explicitly released. --- .../Tasks/DistributedLockServiceTests.cs | 39 ++++++- .../Locking/Migrations/FrameworkMigrations.cs | 2 +- .../Locking/Records/DistributedLockRecord.cs | 2 +- .../Services/DistributedLockService.cs | 110 +++++++++--------- .../Services/IDistributedLockService.cs | 63 +++++++--- 5 files changed, 138 insertions(+), 78 deletions(-) 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 From ec0c071ab6a225e876c18825aa8fac329d01fce5 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 22 Aug 2015 23:07:55 +0100 Subject: [PATCH 27/28] Refactored minor internal affairs. --- .../Services/DistributedLockService.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs index f6ef2f94c..0639f1521 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -69,7 +69,7 @@ namespace Orchard.Tasks.Locking.Services { } 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); + @lock = AcquireLock(name, maxValidFor, machineName, threadId, timeout ?? TimeSpan.Zero); if (@lock != null) return true; @@ -77,23 +77,21 @@ namespace Orchard.Tasks.Locking.Services { } private DistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, string machineName, int? threadId) { - var @lock = AcquireLockInternal(name, maxValidFor, machineName, threadId, timeout); + var @lock = AcquireLock(name, maxValidFor, machineName, threadId, timeout); if (@lock != null) return @lock; 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 AcquireLock(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); + DistributedLock @lock = null; + var acquired = Poll(() => (@lock = AcquireLockInternal(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()); + return @lock; } } catch (Exception ex) { @@ -109,7 +107,7 @@ namespace Orchard.Tasks.Locking.Services { return null; } - private DistributedLockRecord AcquireLockRecord(string name, TimeSpan? maxValidFor, string machineName, int? threadId) { + private DistributedLock AcquireLockInternal(string name, TimeSpan? maxValidFor, string machineName, int? threadId) { var childLifetimeScope = CreateChildLifetimeScope(name); try { @@ -157,7 +155,9 @@ namespace Orchard.Tasks.Locking.Services { if (!canAcquireLock) return null; - return record; + 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."); From db03498cfa1ff7984cef3ca0ebbdc817fce5cdd7 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Wed, 2 Sep 2015 11:43:12 +0100 Subject: [PATCH 28/28] Deletes lock records when Count reaches 0. Also some minor polishing and changed ObjectDisposedException to OrchardException in case a lock was already released. --- .../Tasks/DistributedLockServiceTests.cs | 13 +++++++++++-- .../Locking/Services/DistributedLockService.cs | 18 +++++++++--------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs index 092585fe6..5b0074c27 100644 --- a/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs +++ b/src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs @@ -82,10 +82,19 @@ namespace Orchard.Tests.Tasks { _distributedLockService.ReleaseLock(@lock); _session.Refresh(lockRecord); Assert.That(lockRecord.Count, Is.EqualTo(1)); + } + [Test] + public void ReleasingLockAndCountReachesZeroDeletesLock() + { + DistributedLock @lock; + _distributedLockService.TryAcquireLockForMachine(LockName, TimeSpan.FromSeconds(60), null, out @lock); + + var lockId = Int32.Parse(@lock.Id); _distributedLockService.ReleaseLock(@lock); - _session.Refresh(lockRecord); - Assert.That(lockRecord.Count, Is.EqualTo(0)); + var lockRecord = _distributedLockRepository.Get(lockId); + + Assert.That(lockRecord, Is.Null); } [Test] diff --git a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs index 0639f1521..ed0fb21e0 100644 --- a/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs +++ b/src/Orchard/Tasks/Locking/Services/DistributedLockService.cs @@ -52,12 +52,15 @@ namespace Orchard.Tasks.Locking.Services { var record = repository.Get(lockId); if (record == null) - throw new ObjectDisposedException("@lock", "No lock record could be found for the specified lock to be released."); + throw new OrchardException(T("No lock record could be found for the specified lock to be released.")); if (record.Count <= 0) - throw new ObjectDisposedException("@lock", "The specified lock has already been released."); + throw new OrchardException(T("The specified lock has already been released.")); record.Count--; + + if(record.Count == 0) + repository.Delete(record); } catch (Exception ex) { if (ex.IsFatal()) throw; @@ -70,10 +73,7 @@ namespace Orchard.Tasks.Locking.Services { private bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, string machineName, int? threadId, out DistributedLock @lock) { @lock = AcquireLock(name, maxValidFor, machineName, threadId, timeout ?? TimeSpan.Zero); - if (@lock != null) - return true; - - return false; + return @lock != null; } private DistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, string machineName, int? threadId) { @@ -171,15 +171,15 @@ namespace Orchard.Tasks.Locking.Services { /// /// 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 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 pollFunc, TimeSpan? timeout) { + private bool Poll(Func operation, TimeSpan? timeout) { var waitedTime = TimeSpan.Zero; var waitTime = TimeSpan.FromMilliseconds(timeout.GetValueOrDefault().TotalMilliseconds / 10); bool acquired; - while (!(acquired = pollFunc()) && (timeout == null || waitedTime < timeout.Value)) { + while (!(acquired = operation()) && (timeout == null || waitedTime < timeout.Value)) { Task.Delay(waitTime).ContinueWith(t => { waitedTime += waitTime; }).Wait();