Cleaned up naming, formatting and comments in distributed locks.

src/Orchard.Tests/Tasks/DistributedLockServiceTests.cs

@@ -223,14 +223,14 @@ namespace Orchard.Tests.Tasks {
 
             // Create a never expiring lock.
             _machineNameProvider.MachineName = "Orchard Test Machine 1";
-            var attempt1 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, l: out @lock);
+            var attempt1 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, dLock: out @lock);
             
             // Release the lock.
             _distributedLockService.ReleaseDistributedLock((DistributedLock)@lock);
 
             // Acquire the lock from another machine.
             _machineNameProvider.MachineName = "Orchard Test Machine 2";
-            var attempt2 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, l: out @lock);
+            var attempt2 = _distributedLockService.TryAcquireLock(LockName, maxValidFor: null, timeout: null, dLock: out @lock);
 
             // Validate the results.
             Assert.That(attempt1, Is.True);

src/Orchard/Tasks/Locking/Services/DistributedLock.cs

@@ -9,19 +9,19 @@ namespace Orchard.Tasks.Locking.Services {
         private string _name;
         private int _count;
 
-        public string Name {
-            get {
-                return _name;
-            }
-        }
-
         public DistributedLock(DistributedLockService service, string name) {
             _service = service;
             _name = name;
             _count = 1;
         }
 
-        public void IncreaseReferenceCount() {
+        public string Name {
+            get {
+                return _name;
+            }
+        }
+
+        public void Increment() {
             _count++;
         }
 

src/Orchard/Tasks/Locking/Services/DistributedLockService.cs

@@ -16,13 +16,12 @@ using Orchard.Tasks.Locking.Records;
 namespace Orchard.Tasks.Locking.Services {
 
     public class DistributedLockService : Component, IDistributedLockService {
-        private readonly IClock _clock;
-        private readonly IMachineNameProvider _machineNameProvider;
-        private readonly ShellSettings _shellSettings;
-        private readonly ILifetimeScope _lifetimeScope;
-        private readonly ConcurrentDictionary<string, DistributedLock> _locks = new ConcurrentDictionary<string, DistributedLock>();
 
-        public bool DisableMonitorLock { get; set; }
+        private readonly IMachineNameProvider _machineNameProvider;
+        private readonly ILifetimeScope _lifetimeScope;
+		private readonly IClock _clock;
+        private readonly ShellSettings _shellSettings;
+        private readonly ConcurrentDictionary<string, DistributedLock> _locks;
 
         public DistributedLockService(
             IMachineNameProvider machineNameProvider,
@@ -31,62 +30,58 @@ namespace Orchard.Tasks.Locking.Services {
             ShellSettings shellSettings) {
             _clock = clock;
             _lifetimeScope = lifetimeScope;
-
             _shellSettings = shellSettings;
             _machineNameProvider = machineNameProvider;
-        }
+			_locks = new ConcurrentDictionary<string, DistributedLock>();
+		}
 
-        public bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock l) {
+        public bool DisableMonitorLock { get; set; }
+
+		public bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock dLock) {
             try {
-                l = AcquireLock(name, maxValidFor, timeout);
+                dLock = AcquireLock(name, maxValidFor, timeout);
-                return l != null;
+                return dLock != null;
             }
             catch {
-                l = null;
+                dLock = null;
                 return false;
             }
         }
 
         public IDistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout) {
-            DistributedLock l = null;
+            DistributedLock dLock = null;
             
             try {
+                var acquired = Poll(() => (dLock = AcquireLockInternal(name, maxValidFor)) != null, timeout);
 
-                var acquired = Poll(() => (l = AcquireLockInternal(name, maxValidFor)) != null, timeout);
+                if (acquired)
-
+                    Logger.Debug("Successfully acquired lock '{0}'.", name);
-                if (acquired) {
+                else
-                    Logger.Debug("Successfully acquired a lock named '{0}'.", name);
+                    Logger.Debug("Failed to acquire lock '{0}' within the specified timeout.", name);
-                }
-                else {
-                    Logger.Debug("Failed to acquire a lock named '{0}'.", name);
-                }
             }
             catch (Exception ex) {
-                Logger.Error(ex, "Error while trying to acquire a lock named '{0}'.", name);
+                Logger.Error(ex, "Error while trying to acquire lock '{0}'.", name);
                 throw;
             }
 
-            if (l == null && timeout != null) {
+            if (dLock == null && timeout != null)
-                throw new TimeoutException(String.Format("Failed to acquire a lock named '{0}' within the specified timeout ('{1}').", name, timeout));
+                throw new TimeoutException(String.Format("Failed to acquire lock '{0}' within the specified timeout ({1}).", name, timeout));
-            }
 
-            return l;
+			return dLock;
         }
 
-        public void ReleaseDistributedLock(DistributedLock l) {
+        public void ReleaseDistributedLock(DistributedLock dLock) {
-
             try {
-                var record = GetDistributedLockRecordByName(l.Name);
+                var record = GetDistributedLockRecordByName(dLock.Name);
-
+                if (record == null)
-                if (record == null) {
+                    throw new OrchardException(T("No lock record could be found in the database for lock '{0}'.", dLock.Name));
-                    throw new OrchardException(T("No lock record could be found for the specified lock to be released."));
-                }
 
                 TryCommitNewTransaction(repository => repository.Delete(record));
             }
             catch (Exception ex) {
-                if (ex.IsFatal()) throw;
+                if (ex.IsFatal())
-                Logger.Error(ex, "An non-fatal error occurred while trying to dispose a distributed lock with name '{0}'.", l.Name);
+					throw;
+				Logger.Error(ex, "An non-fatal error occurred while releasing lock '{0}'.", dLock.Name);
             }
         }
 
@@ -105,8 +100,7 @@ namespace Orchard.Tasks.Locking.Services {
             DistributedLockRecord result = null;
             TryCommitNewTransaction(repository => {
                 result = repository.Table.FirstOrDefault(x =>
-                    x.Name == name &&
+                    x.Name == name && (x.ValidUntilUtc == null || x.ValidUntilUtc >= _clock.UtcNow)
-                    (x.ValidUntilUtc == null || x.ValidUntilUtc >= _clock.UtcNow)
                 );
             });
 
@@ -117,19 +111,18 @@ namespace Orchard.Tasks.Locking.Services {
             try {
                 name = GetTenantLockName(name);
 
-                if (!DisableMonitorLock && !Monitor.TryEnter(String.Intern(name))) {
+                if (!DisableMonitorLock && !Monitor.TryEnter(String.Intern(name))) 
                     return null;
-                }
 
                 DistributedLock dLock = null;
                 
-                // Returns the existing lock in case of reentrancy.
+                // Return the existing lock in case of reentrancy.
                 if(!DisableMonitorLock && _locks.TryGetValue(name, out dLock)) {
-                    dLock.IncreaseReferenceCount();
+                    dLock.Increment();
                     return dLock;
                 }
 
-                // Find an existing, active lock, if any.
+                // Find an existing active lock, if any.
                 var record = GetValidDistributedLockRecordByName(name);
 
                 // The current owner name (based on machine name).
@@ -151,8 +144,7 @@ namespace Orchard.Tasks.Locking.Services {
                         ValidUntilUtc = maxValidFor != null ? _clock.UtcNow + maxValidFor : null
                     };
 
-                    
+                    canAcquireLock = TryCommitNewTransaction(repository => {
-                    canAcquireLock = TryCommitNewTransaction( repository => {
                         repository.Create(record);
                     });
                 }
@@ -201,9 +193,8 @@ namespace Orchard.Tasks.Locking.Services {
         }
 
         private bool TryCommitNewTransaction(Action<IRepository<DistributedLockRecord>> action) {
-            if (action == null) {
+            if (action == null)
                 throw new ArgumentNullException();
-            }
 
             try {
                 using (var childLifetimeScope = _lifetimeScope.BeginLifetimeScope()) {
@@ -218,7 +209,6 @@ namespace Orchard.Tasks.Locking.Services {
             catch {
                 return false;
             }
-            
         }
     }
 }

src/Orchard/Tasks/Locking/Services/IDistributedLock.cs

@@ -3,7 +3,9 @@
 namespace Orchard.Tasks.Locking.Services {
 
     /// <summary>
-    /// Represents a distributed lock. />
+    /// Represents a distributed lock returned by <c>IDistributedLockService</c>. The owner of the 
+	/// lock should call <c>Dispose()</c> on an instance of this interface to release the distributed
+	/// lock.
     /// </summary>
     public interface IDistributedLock : IDisposable {
         string Name { get; }

src/Orchard/Tasks/Locking/Services/IDistributedLockService.cs

@@ -1,71 +1,82 @@
 using System;
 
 namespace Orchard.Tasks.Locking.Services {
-    /// <summary>
+
-    /// Provides distributed locking functionality.
+	/// <summary>
-    /// </summary>
+	/// Provides functionality to acquire and release tenant-wide locks which are
-    public interface IDistributedLockService : IDependency {
+	/// distributed across all instances in a web farm.
-        /// <summary>
+	/// </summary>
-        /// Tries to acquire a distributed lock on a named resource for the current tenant.
+	/// <remarks>
-        /// </summary>
+	/// Distributed locks can be used to protect critical sections that should only ever
-        /// <param name="name">The name to use for the lock.</param>
+	/// be executed by a single thread of execution across a whole web farm. The distributed
-        /// <param name="maxValidFor">The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If <c>null</c> is specified, the lock never expires until it's released by the owner.</param>
+	/// locks returned by this service are reentrant, i.e. the owner of a lock can reacquire it
-        /// <param name="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.</param>
+	/// multiple times, and must also release it (dispose of it) as many times as it was
-        /// <param name="lock">The acquired lock.</param>
+	/// acquired for the lock to be released.
-        /// <returns>Returns true if a lock was successfully acquired, false otherwise.</returns>
+	/// </remarks>
-        bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock @lock);
+	public interface IDistributedLockService : IDependency {
 
         /// <summary>
-        /// Acquires a distributed lock on a named resource for the current tenant.
+        /// Tries to acquire a named distributed lock within the current tenant.
         /// </summary>
-        /// <param name="name">The name to use for the lock.</param>
+        /// <param name="name">The name of the lock to acquire.</param>
-        /// <param name="maxValidFor">The maximum amount of time the lock is allowed. This is a safety net in case the caller fails to release the lock. If <c>null</c> is specified, the lock never expires until it's released by the owner.</param>
+        /// <param name="maxValidFor">The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If <c>null</c> is specified, the lock never automatically expires.</param>
-        /// <param name="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.</param>
+        /// <param name="timeout">The amount of time to wait for the lock to be acquired. Passing <c>TimeSpan.Zero</c> will cause the method to return immediately. Passing <c>null</c> will cause the method to block indefinitely until a lock can be acquired.</param>
-        /// <returns>Returns a lock.</returns>
+        /// <param name="lock">This out parameter will be assigned the acquired lock if successful.</param>
-        /// <exception cref="TimeoutException">Throws a TimeoutException if no lock could be acquired in time.</exception>
+        /// <returns><c>true</c> if the lock was successfully acquired, otherwise <c>false</c>.</returns>
-        IDistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout);
+        bool TryAcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout, out IDistributedLock @lock);
+
+		/// <summary>
+		/// Acquires a named distributed lock within the current tenant or throws if the lock cannot be acquired.
+		/// </summary>
+		/// <param name="name">The name of the lock to acquire.</param>
+		/// <param name="maxValidFor">The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If <c>null</c> is specified, the lock never automatically expires.</param>
+		/// <param name="timeout">The amount of time to wait for the lock to be acquired. Passing <c>TimeSpan.Zero</c> will cause the method to return immediately. Passing <c>null</c> will cause the method to block indefinitely until a lock can be acquired.</param>
+		/// <returns>The acquired lock.</returns>
+		/// <exception cref="TimeoutException">This method throws a TimeoutException if the lock could not be acquired within the specified timeout period.</exception>
+		IDistributedLock AcquireLock(string name, TimeSpan? maxValidFor, TimeSpan? timeout);
     }
 
     public static class DistributedLockServiceExtensions {
-        /// <summary>
+
-        /// Tries to acquire a lock on the specified name for the current machine.
+		/// <summary>
-        /// </summary>
+		/// Tries to immediately acquire a named distributed lock with a given expiration time within the current tenant.
-        /// <param name="name">The name to use for the lock.</param>
+		/// </summary>
-        /// <param name="maxValidFor">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.</param>
+		/// <param name="name">The name of the lock to acquire.</param>
-        /// <param name="lock">The acquired lock.</param>
+		/// <param name="maxValidFor">The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If <c>null</c> is specified, the lock never automatically expires.</param>
-        /// <returns>Returns true if a lock was successfully acquired, false otherwise.</returns>
+		/// <param name="lock">This out parameter will be assigned the acquired lock if successful.</param>
-        public static bool TryAcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor, out IDistributedLock @lock) {
+		/// <returns><c>true</c> if the lock could be immediately acquired, otherwise <c>false</c>.</returns>
+		public static bool TryAcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor, out IDistributedLock @lock) {
             return service.TryAcquireLock(name, maxValidFor, TimeSpan.Zero, out @lock);
         }
 
-        /// <summary>
+		/// <summary>
-        /// Tries to acquire a lock on the specified name for the current machine.
+		/// Tries to immediately acquire a named distributed lock with no expiration time within the current tenant.
-        /// </summary>
+		/// </summary>
-        /// <param name="name">The name to use for the lock.</param>
+		/// <param name="name">The name of the lock to acquire.</param>
-        /// <param name="lock">The acquired lock.</param>
+		/// <param name="lock">This out parameter will be assigned the acquired lock if successful.</param>
-        /// <returns>Returns true if a lock was successfully acquired, false otherwise.</returns>
+		/// <returns><c>true</c> if the lock could be immediately acquired, otherwise <c>false</c>.</returns>
-        public static bool TryAcquireLock(this IDistributedLockService service, string name, out IDistributedLock @lock) {
+		public static bool TryAcquireLock(this IDistributedLockService service, string name, out IDistributedLock @lock) {
             return service.TryAcquireLock(name, null, TimeSpan.Zero, out @lock);
         }
 
-        /// <summary>
+		/// <summary>
-        /// Acquires a lock with the specified parameters for the current machine.
+		/// Acquires a named distributed lock with a given expiration time within the current tenant.
-        /// </summary>
+		/// </summary>
-        /// <param name="name">The name to use for the lock.</param>
+		/// <param name="name">The name of the lock to acquire.</param>
-        /// <param name="maxValidFor">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.</param>
+		/// <param name="maxValidFor">The maximum amount of time the lock should be held before automatically expiring. This is a safeguard in case the owner fails to release the lock. If <c>null</c> is specified, the lock never automatically expires.</param>
-        /// <returns>Returns a lock.</returns>
+		/// <returns>The acquired lock.</returns>
-        /// <exception cref="TimeoutException">Throws a TimeoutException if no lock could be acquired in time.</exception>
+		/// <remarks>This method blocks indefinitely until the lock can be acquired.</remarks>
-        public static IDistributedLock AcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor) {
+		public static IDistributedLock AcquireLock(this IDistributedLockService service, string name, TimeSpan? maxValidFor) {
             return service.AcquireLock(name, maxValidFor, null);
         }
 
-        /// <summary>
+		/// <summary>
-        /// Acquires a lock with the specified parameters for the current machine.
+		/// Acquires a named distributed lock with no expiration time within the current tenant.
-        /// </summary>
+		/// </summary>
-        /// <param name="name">The name to use for the lock.</param>
+		/// <param name="name">The name to use for the lock.</param>
-        /// <returns>Returns a lock.</returns>
+		/// <returns>The acquired lock.</returns>
-        /// <exception cref="TimeoutException">Throws a TimeoutException if no lock could be acquired in time.</exception>
+		/// <remarks>This method blocks indefinitely until the lock can be acquired.</remarks>
-        public static IDistributedLock AcquireLock(this IDistributedLockService service, string name) {
+		public static IDistributedLock AcquireLock(this IDistributedLockService service, string name) {
             return service.AcquireLock(name, null, null);
         }
     }