Removed the dictionary of locked keys as it will not be needed with NH 5.2

Author: maca88

StackExRedis/NHibernate.Caches.StackExRedis.Tests/Async/Caches/DistributedRedisCache.cs

@@ -62,19 +62,23 @@ public async Task LockAsync(object key, CancellationToken cancellationToken)
 		{
 			// A simple locking that requires all instances to obtain the lock
 			// A real distributed cache should use something like the Redlock algorithm.
+			var lockValues = new string[_regionStrategies.Length];
 			try
 			{
-				foreach (var strategy in _regionStrategies)
+				for (var i = 0; i < _regionStrategies.Length; i++)
 				{
-					await (strategy.LockAsync(key, cancellationToken));
+					lockValues[i] = await (_regionStrategies[i].LockAsync(key, cancellationToken));
 				}
-
 			}
 			catch (CacheException)
 			{
-				foreach (var strategy in _regionStrategies)
+				for (var i = 0; i < _regionStrategies.Length; i++)
 				{
-					await (strategy.UnlockAsync(key, cancellationToken));
+					if (lockValues[i] == null)
+					{
+						continue;
+					}
+					await (_regionStrategies[i].UnlockAsync(key, lockValues[i], cancellationToken));
 				}
 				throw;
 			}
@@ -85,7 +89,8 @@ public async Task UnlockAsync(object key, CancellationToken cancellationToken)
 		{
 			foreach (var strategy in _regionStrategies)
 			{
-				await (strategy.UnlockAsync(key, cancellationToken));
+				// TODO: use the lockValue when upgrading to NH 5.2
+				await (strategy.UnlockAsync(key, null, cancellationToken));
 			}
 		}
 

StackExRedis/NHibernate.Caches.StackExRedis.Tests/Caches/DistributedRedisCache.cs

@@ -83,19 +83,23 @@ public void Lock(object key)
 		{
 			// A simple locking that requires all instances to obtain the lock
 			// A real distributed cache should use something like the Redlock algorithm.
+			var lockValues = new string[_regionStrategies.Length];
 			try
 			{
-				foreach (var strategy in _regionStrategies)
+				for (var i = 0; i < _regionStrategies.Length; i++)
 				{
-					strategy.Lock(key);
+					lockValues[i] = _regionStrategies[i].Lock(key);
 				}
-
 			}
 			catch (CacheException)
 			{
-				foreach (var strategy in _regionStrategies)
+				for (var i = 0; i < _regionStrategies.Length; i++)
 				{
-					strategy.Unlock(key);
+					if (lockValues[i] == null)
+					{
+						continue;
+					}
+					_regionStrategies[i].Unlock(key, lockValues[i]);
 				}
 				throw;
 			}
@@ -106,7 +110,8 @@ public void Unlock(object key)
 		{
 			foreach (var strategy in _regionStrategies)
 			{
-				strategy.Unlock(key);
+				// TODO: use the lockValue when upgrading to NH 5.2
+				strategy.Unlock(key, null);
 			}
 		}
 

StackExRedis/NHibernate.Caches.StackExRedis/AbstractRegionStrategy.cs

@@ -34,22 +34,6 @@ public abstract partial class AbstractRegionStrategy
 
 		private readonly RedisKeyLocker _keyLocker;
 
-		/// <summary>
-		/// With the current NHibernate version (5.1) the <see cref="ICache.Lock"/> method does not have a
-		/// return value that would indicate which lock value was used to lock the key, that would then be
-		/// passed to the <see cref="ICache.Unlock"/> method in order to prevent unlocking keys that were locked
-		/// by others. Luckily, the <see cref="ICache.Lock"/> and <see cref="ICache.Unlock"/> methods are currently
-		/// always called inside a lock statement which we abuse by storing the locked key when the
-		/// <see cref="ICache.Lock"/> is called and use it when <see cref="ICache.Unlock"/> is called. This
-		/// means that our <see cref="Lock"/> and <see cref="Unlock"/> are not thread safe and have to be called
-		/// sequentially in order prevent overriding the lock key for a given key.
-		/// Currently, the <see cref="ICache.Unlock"/> is always called after the <see cref="ICache.Lock"/> method
-		/// even if an exception occurs which we are also abusing in order to avoid having a special mechanism to 
-		/// clear the dictionary in order to prevent memory leaks.
-		/// </summary>
-		private readonly ConcurrentDictionary<string, string> _acquiredKeyLocks = new ConcurrentDictionary<string, string>();
-
-
 		/// <summary>
 		/// The constructor for creating the region strategy.
 		/// </summary>
@@ -405,17 +389,7 @@ public virtual string Lock(object key)
 
 			var cacheKey = GetCacheKey(key);
 			Log.Debug("Locking object with key: '{0}'.", cacheKey);
-			var lockValue = _keyLocker.Lock(cacheKey, LockScript, GetAdditionalKeys(), GetAdditionalValues());
-
-			_acquiredKeyLocks.AddOrUpdate(cacheKey, _ => lockValue, (_, currValue) =>
-			{
-				Log.Warn(
-					$"Calling {nameof(Lock)} method for key:'{cacheKey}' that was already locked. " +
-					$"{nameof(Unlock)} method must be called after each {nameof(Lock)} call for " +
-					$"the same key prior calling {nameof(Lock)} again with the same key.");
-				return lockValue;
-			});
-			return lockValue;
+			return _keyLocker.Lock(cacheKey, LockScript, GetAdditionalKeys(), GetAdditionalValues());
 		}
 
 		/// <summary>
@@ -450,8 +424,9 @@ public virtual string LockMany(object[] keys)
 		/// Unlocks the key.
 		/// </summary>
 		/// <param name="key">The key to unlock.</param>
+		/// <param name="lockValue">The value used to lock the key.</param>
 		/// <returns>Whether the key was unlocked</returns>
-		public virtual bool Unlock(object key)
+		public virtual bool Unlock(object key, string lockValue)
 		{
 			if (key == null)
 			{
@@ -460,14 +435,6 @@ public virtual bool Unlock(object key)
 
 			var cacheKey = GetCacheKey(key);
 			Log.Debug("Unlocking object with key: '{0}'.", cacheKey);
-
-			if (!_acquiredKeyLocks.TryRemove(cacheKey, out var lockValue))
-			{
-				Log.Warn(
-					$"Calling {nameof(Unlock)} method for key:'{cacheKey}' that was not locked with {nameof(Lock)} method before.");
-				return false;
-			}
-
 			var unlocked = _keyLocker.Unlock(cacheKey, lockValue, UnlockScript, GetAdditionalKeys(), GetAdditionalValues());
 			Log.Debug("Unlock key '{0}' result: {1}", cacheKey, unlocked);
 			return unlocked;
@@ -477,7 +444,7 @@ public virtual bool Unlock(object key)
 		/// Unlocks many keys at once.
 		/// </summary>
 		/// <param name="keys">The keys to unlock.</param>
-		/// <param name="lockValue">The value used to lock the keys</param>
+		/// <param name="lockValue">The value used to lock the keys.</param>
 		/// <returns>The number of unlocked keys.</returns>
 		public virtual int UnlockMany(object[] keys, string lockValue)
 		{

StackExRedis/NHibernate.Caches.StackExRedis/Async/AbstractRegionStrategy.cs

@@ -321,23 +321,16 @@ public virtual Task<string> LockAsync(object key, CancellationToken cancellation
 			{
 				return Task.FromCanceled<string>(cancellationToken);
 			}
-			return InternalLockAsync();
-			async Task<string> InternalLockAsync()
+			try
 			{
 
 				var cacheKey = GetCacheKey(key);
 				Log.Debug("Locking object with key: '{0}'.", cacheKey);
-				var lockValue = await (_keyLocker.LockAsync(cacheKey, LockScript, GetAdditionalKeys(), GetAdditionalValues(), cancellationToken)).ConfigureAwait(false);
-
-				_acquiredKeyLocks.AddOrUpdate(cacheKey, _ => lockValue, (_, currValue) =>
-				{
-					Log.Warn(
-					$"Calling {nameof(LockAsync)} method for key:'{cacheKey}' that was already locked. " +
-					$"{nameof(UnlockAsync)} method must be called after each {nameof(LockAsync)} call for " +
-					$"the same key prior calling {nameof(LockAsync)} again with the same key.");
-					return lockValue;
-				});
-				return lockValue;
+				return _keyLocker.LockAsync(cacheKey, LockScript, GetAdditionalKeys(), GetAdditionalValues(), cancellationToken);
+			}
+			catch (Exception ex)
+			{
+				return Task.FromException<string>(ex);
 			}
 		}
 
@@ -385,9 +378,10 @@ public virtual Task<string> LockManyAsync(object[] keys, CancellationToken cance
 		/// Unlocks the key.
 		/// </summary>
 		/// <param name="key">The key to unlock.</param>
+		/// <param name="lockValue">The value used to lock the key.</param>
 		/// <param name="cancellationToken">A cancellation token that can be used to cancel the work</param>
 		/// <returns>Whether the key was unlocked</returns>
-		public virtual Task<bool> UnlockAsync(object key, CancellationToken cancellationToken)
+		public virtual Task<bool> UnlockAsync(object key, string lockValue, CancellationToken cancellationToken)
 		{
 			if (key == null)
 			{
@@ -403,14 +397,6 @@ async Task<bool> InternalUnlockAsync()
 
 				var cacheKey = GetCacheKey(key);
 				Log.Debug("Unlocking object with key: '{0}'.", cacheKey);
-
-				if (!_acquiredKeyLocks.TryRemove(cacheKey, out var lockValue))
-				{
-					Log.Warn(
-					$"Calling {nameof(UnlockAsync)} method for key:'{cacheKey}' that was not locked with {nameof(LockAsync)} method before.");
-					return false;
-				}
-
 				var unlocked = await (_keyLocker.UnlockAsync(cacheKey, lockValue, UnlockScript, GetAdditionalKeys(), GetAdditionalValues(), cancellationToken)).ConfigureAwait(false);
 				Log.Debug("Unlock key '{0}' result: {1}", cacheKey, unlocked);
 				return unlocked;
@@ -421,7 +407,7 @@ async Task<bool> InternalUnlockAsync()
 		/// Unlocks many keys at once.
 		/// </summary>
 		/// <param name="keys">The keys to unlock.</param>
-		/// <param name="lockValue">The value used to lock the keys</param>
+		/// <param name="lockValue">The value used to lock the keys.</param>
 		/// <param name="cancellationToken">A cancellation token that can be used to cancel the work</param>
 		/// <returns>The number of unlocked keys.</returns>
 		public virtual Task<int> UnlockManyAsync(object[] keys, string lockValue, CancellationToken cancellationToken)

StackExRedis/NHibernate.Caches.StackExRedis/Async/DefaultRegionStrategy.cs

@@ -182,12 +182,12 @@ public override async Task<long> RemoveManyAsync(object[] keys, CancellationToke
 		}
 
 		/// <inheritdoc />
-		public override async Task<bool> UnlockAsync(object key, CancellationToken cancellationToken)
+		public override async Task<bool> UnlockAsync(object key, string lockValue, CancellationToken cancellationToken)
 		{
 			cancellationToken.ThrowIfCancellationRequested();
 			try
 			{
-				return await (base.UnlockAsync(key, cancellationToken)).ConfigureAwait(false);
+				return await (base.UnlockAsync(key, lockValue, cancellationToken)).ConfigureAwait(false);
 			}
 			catch (RedisServerException e) when (e.Message == InvalidVersionMessage)
 			{

StackExRedis/NHibernate.Caches.StackExRedis/Async/RedisCache.cs

@@ -115,7 +115,8 @@ public Task UnlockAsync(object key, CancellationToken cancellationToken)
 			{
 				return Task.FromCanceled<object>(cancellationToken);
 			}
-			return RegionStrategy.UnlockAsync(key, cancellationToken);
+			// TODO: use the lockValue when upgrading to NH 5.2
+			return RegionStrategy.UnlockAsync(key, null, cancellationToken);
 		}
 
 		/// <inheritdoc />

StackExRedis/NHibernate.Caches.StackExRedis/DefaultRegionStrategy.cs

@@ -259,11 +259,11 @@ public override long RemoveMany(object[] keys)
 		}
 
 		/// <inheritdoc />
-		public override bool Unlock(object key)
+		public override bool Unlock(object key, string lockValue)
 		{
 			try
 			{
-				return base.Unlock(key);
+				return base.Unlock(key, lockValue);
 			}
 			catch (RedisServerException e) when (e.Message == InvalidVersionMessage)
 			{

StackExRedis/NHibernate.Caches.StackExRedis/RedisCache.cs

@@ -99,7 +99,8 @@ public object LockMany(object[] keys)
 		/// <inheritdoc />
 		public void Unlock(object key)
 		{
-			RegionStrategy.Unlock(key);
+			// TODO: use the lockValue when upgrading to NH 5.2
+			RegionStrategy.Unlock(key, null);
 		}
 
 		/// <inheritdoc />