Add busy lock support for RedisLockExtension

[mgoodfellow]

Hi @Turnerj

Going on from #181 this was the busy lock solution I was talking about. I had some time to sketch it out this morning so I thought I would PR it.

If you don't want this to be part of this project, that's fine, I'm happy to make it a separate extension project as an add on to CacheTower - I just built it here as it was much easier to test!

What's the difference?

We are dropping pub/sub in favour of busy locks. Effectively this is almost the same as pub/sub, but instead of relying on this channel, the context that failed to get the lock instead sets up a tight loop to spin on the lock and check for when it is released.

The trade off here is that your responsiveness in the lock being released is controlled by the SpinTime, however, in the failure conditions of these pub/sub messages being lost, we don't just stuck waiting on a long LockTime before we fail safe on the cancellation token.

In our use case, I don't want to reduce the LockTime to make sure we don't crash out some processes for a long time, but would take the responsiveness cost of the SpinTime slowing down the release on other contexts waiting on the lock. Our lock contention is generally pretty low, but a crashed lock is more expensive to us.

Anyway, as always food for through, and any/all feedback is always appreciated!

I'm wondering if a combination of both might be even better? Starts to get a tad complex then however.

[Turnerj]

Hey - thanks for submitting this!

If you don't want this to be part of this project, that's fine, I'm happy to make it a separate extension project as an add on to CacheTower - I just built it here as it was much easier to test!

Honestly I'd be happy both ways (whether it was in the project or as a separate thing) but I think for it to be in the project, there is kinda one major suggestion I had - what if instead of having it as a separate extension we instead had it as a "strategy" to a singular extension.

I'm wondering if a combination of both might be even better? Starts to get a tad complex then however.

Sounds like we might have the same idea!

For all intents and purposes - it is just a Redis lock. How it actually does it is the strategy it takes. There is a lot of overlap in the code otherwise and I think we could take advantage of that.

So it could look something a little like this:

public class RedisLockExtension : ICacheRefreshCallSiteWrapperExtension
{
	private ISubscriber Subscriber { get; }
	private IDatabaseAsync Database { get; }
	private RedisLockOptions Options { get; }

	private ICacheStack? RegisteredStack { get; set; }

	internal ConcurrentDictionary<string, TaskCompletionSource> LockedOnKeyRefresh { get; }

	public RedisLockExtension(IConnectionMultiplexer connection) : this(connection, RedisLockOptions.Default) { }

	public RedisLockExtension(IConnectionMultiplexer connection, RedisLockOptions options)
	{
		if (connection == null)
		{
			throw new ArgumentNullException(nameof(connection));
		}

		Options = options;
		Database = connection.GetDatabase(options.DatabaseIndex);
		LockedOnKeyRefresh = new ConcurrentDictionary<string, TaskCompletionSource>(StringComparer.Ordinal);

		if (options.Strategy == RedisLockStrategy.SpinLock)
		{
			// do your spin lock setup
		}
		else if (options.Strategy == RedisLockStrategy.PubSub)
		{
			Subscriber = connection.GetSubscriber();
			Subscriber.Subscribe(options.RedisChannel, (channel, value) => UnlockWaitingTasks(value));
		}
		else
		{
			throw new ArgumentException("Invalid Redis lock strategy in options", nameof(options));
		}
	}
	
	private ValueTask<bool> GetLock(string lockKey, TimeSpan expiry)
	{
		return Database.StringSetAsync(lockKey, RedisValue.EmptyString, expiry: expiry, when: When.NotExists);
	}
	
	private ValueTask<bool> CheckLock(string lockKey)
	{
		return Database.KeyExistsAsync(lockKey);
	}
	
	private async ValueTask Unlock(string lockKey)
	{
		await Database.KeyDeleteAsync(lockKey, CommandFlags.FireAndForget);

		if (Options.Strategy == RedisLockStrategy.PubSub)
		{
			await Subscriber.PublishAsync(Options.RedisChannel, cacheKey, CommandFlags.FireAndForget);
		}
	}

	private TaskCompletionSource GetTaskCompletionSource(string lockKey, CacheSettings settings)
	{
		var tcs = new TaskCompletionSource();
		if (Options.Strategy == RedisLockStrategy.SpinLock)
		{
			_ = TestLock(tcs, Options.SpinAttempts);
			return tcs;

			async Task TestLock(TaskCompletionSource taskCompletionSource, int spinAttempts)
			{
				var spinAttempt = 0;
				while (spinAttempt <= spinAttempts)
				{
					spinAttempt++;
					
					var lockExists = await CheckLock(lockKey);
					if (lockExists)
					{
						await Task.Delay(Options.SpinTime);
						continue;
					}

					taskCompletionSource.TrySetResult();
					return;
				}

				taskCompletionSource.TrySetCanceled();
			}
		}
		else
		{
			var cts = new CancellationTokenSource(Options.LockTimeout);
			cts.Token.Register(tcs => ((TaskCompletionSource)tcs).TrySetCanceled(), tcs, useSynchronizationContext: false);
		}
		
		return tcs;
	}

	/// <remarks>
	/// The <see cref="RedisLockExtension"/> attempts to set a key in Redis representing whether it has achieved a lock.
	/// If it succeeds to set the key, it continues to refresh the value, broadcasting the success of the updated value to all subscribers.
	/// If it fails to set the key, it waits until notified that the cache is updated, retrieving it from the cache stack and returning the value.
	/// </remarks>
	/// <inheritdoc/>
	public async ValueTask<CacheEntry<T>> WithRefreshAsync<T>(string cacheKey, Func<ValueTask<CacheEntry<T>>> valueProvider, CacheSettings settings)
	{
		var lockKey = string.Format(Options.KeyFormat, cacheKey);
		var hasLock = await GetLock(lockKey, Options.LockTimeout);

		if (hasLock)
		{
			try
			{
				var cacheEntry = await valueProvider();
				return cacheEntry;
			}
			finally
			{
				await Unlock(lockKey);
			}
		}
		else
		{
			var completionSource = LockedOnKeyRefresh.GetOrAdd(cacheKey, key => GetTaskCompletionSource(lockKey, settings));

			//Last minute check to confirm whether waiting is required (in case the notification is missed)
			var currentEntry = await RegisteredStack!.GetAsync<T>(cacheKey);
			if (currentEntry != null && currentEntry.GetStaleDate(settings) > Internal.DateTimeProvider.Now)
			{
				UnlockWaitingTasks(cacheKey);
				return currentEntry;
			}

			//Lock until we are notified to be unlocked
			await completionSource.Task;

			//Get the updated value from the cache stack
			return (await RegisteredStack.GetAsync<T>(cacheKey))!;
		}
	}

	private void UnlockWaitingTasks(string cacheKey)
	{
		if (LockedOnKeyRefresh.TryRemove(cacheKey, out var waitingTasks))
		{
			waitingTasks.TrySetResult();
		}
	}
}

A few things with that block of code:

• I haven't run it
• I removed a bunch of code (like the registering of the cache stack) to make a smaller example - those would need to be re-added
• It combines the existing RedisLockExtension with your code in this repo + the change to remove the generic TaskCompletionSource
• I'm not using the LockTakeAsync etc methods - they mostly wrap the methods I'm using in the current RedisLockExtension and (for some reason) they actually perform slower...

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
LockTakeLockRelease	394.8 μs	1.57 μs	1.39 μs	0.9766	-	-	3896 B
StringSetKeyDelete	268.3 μs	1.00 μs	0.89 μs	-	-	-	777 B

What are your thoughts on this?

For testing, we'd just need to double up on the lock tests to test each strategy.

[mgoodfellow]

Interesting to see that performance difference between Locks and StringSets - I wouldn't have expected that level of difference!

Ok, one step further - do we even need to "turn off" subscriber mode? Instead really you are just enabling a busy lock to deal with the situation that a published message may fail to be received via the subscription.

Let me quickly sketch up a new form of this

[mgoodfellow]

Ok, updated to have this busy lock checker as an additional feature that can be turned on for the existing lock.

The more I think about it, the more I cannot think why you wouldn't want the feature of pub/sub - this is just an additional safety net which reacts faster than the task timeout on the lock.

Open to more feedback - also really happy to accept any suggestions on naming, or configuring options. I started to think that maybe a RedisLockOptionsBuilder() might be nice with a fluent interface to "turn things on". I think that might be overkill though.

Anyway, let me know what you think!

[mgoodfellow]

By the way, non-generic TCS only exists in .NET 5 it seems

[Turnerj]

Yeah, that's a good point - could always use pub/sub but have spin lock optional.

The perf difference of the key setting code surprised me too. Only looked into it as I was wondering what it was. Actually thought the results could have been the other way around.

[Turnerj]

Looks good to me!