Redis Output Cache Store refactoring - IOutputCacheStore Redis Implementation

Question

This code incorporates the new .NET 7 Output Caching. I would like to get some refactoring tips on the RedisOutputCacheStore class.

How to use

// Redis Output Cache
builder.Services.AddStackExchangeRedisCache(options => options.ConfigurationOptions = new ConfigurationOptions
{
 EndPoints = { "localhost:6379" },
 Password = "my_master_password",
 Ssl = false // false for development testing purposes
});
builder.Services.AddRedisOutputCache();
app.UseOutputCache();
app.MapGet("/cache", () =>
 {
 Log.Information("Cache 1 called");
 return Task.FromResult(Results.Ok("This is the endpoint that gets cached for 30 seconds. It also gets categorised under the 'cache' tag"));
 })
 .CacheOutput(x => x
 .Expire(TimeSpan.FromSeconds(30))
 .Tag("cache"));
app.MapGet("/cache2", (ILoggerFactory loggerFactory) =>
 {
 Log.Information("Cache 2 called");
 return Task.FromResult(Results.Ok("This is the secondary endpoint that gets cached for 20 seconds. It also gets categorised under the 'cache' tag"));
 })
 .CacheOutput(x => x
 .Expire(TimeSpan.FromSeconds(20))
 .Tag("cache"));
app.MapGet("/uncache",
 async (IOutputCacheStore outputCacheStore, CancellationToken token) =>
 {
 await outputCacheStore.EvictByTagAsync("cache", token);
 return Results.Ok("This endpoint removes all the values for the cached endpoints that are categorised under the 'cache' tag");
 });

Code

public static class OutputCacheExtensions
{
 /// <summary>
 /// Add output caching services using Redis.
 /// </summary>
 /// <param name="services">The <see cref="IServiceCollection" /> for adding services.</param>
 /// <returns></returns>
 public static IServiceCollection AddRedisOutputCache(this IServiceCollection services)
 {
 ArgumentNullException.ThrowIfNull(services);
 services.AddSingleton<ObjectPoolProvider, DefaultObjectPoolProvider>();
 services.AddSingleton<IOutputCacheStore>(sp =>
 {
 var distributedCache = sp.GetRequiredService<IDistributedCache>();
 var redisCacheOptions = sp.GetRequiredService<IOptions<RedisCacheOptions>>();
 return new RedisOutputCacheStore(distributedCache, redisCacheOptions);
 });
 return services;
 }
}
public sealed class RedisOutputCacheStore : IOutputCacheStore
{
 private const string SetScript = @"
 redis.call('HSET', KEYS[1], 'absexp', ARGV[1], 'sldexp', ARGV[2], 'data', ARGV[4])
 if ARGV[3] ~= '-1' then
 redis.call('EXPIRE', KEYS[1], ARGV[3])
 end
 return 1";
 private const string SetScriptPreExtendedSetCommand = @"
 redis.call('HMSET', KEYS[1], 'absexp', ARGV[1], 'sldexp', ARGV[2], 'data', ARGV[4])
 if ARGV[3] ~= '-1' then
 redis.call('EXPIRE', KEYS[1], ARGV[3])
 end
 return 1";
 private static readonly Version ServerVersionWithExtendedSetCommand = new(4, 0, 0);
 private readonly IDistributedCache _distributedCache;
 private readonly RedisCacheOptions _options;
 private readonly SemaphoreSlim _connectionLock = new(1, 1);
 private volatile IConnectionMultiplexer? _connection;
 private IDatabase? _cache;
 private string _setScript = SetScript;
 public RedisOutputCacheStore(IDistributedCache distributedCache, IOptions<RedisCacheOptions> redisCacheOptions)
 {
 _distributedCache = distributedCache;
 _options = redisCacheOptions.Value;
 }
 public async ValueTask EvictByTagAsync(string tag, CancellationToken cancellationToken)
 {
 await ConnectAsync(cancellationToken).ConfigureAwait(false);
 var memberKeys = _cache!.SetMembers($"tag_{tag}").Select(x => x.ToString());
 var redisKeys = memberKeys.Select(x => new RedisKey(x)).ToArray();
 await _cache.KeyDeleteAsync(redisKeys).ConfigureAwait(false);
 }
 public async ValueTask<byte[]?> GetAsync(string key, CancellationToken cancellationToken)
 {
 return await _distributedCache.GetAsync(key, cancellationToken).ConfigureAwait(false);
 }
 public async ValueTask SetAsync(string key, byte[] value, string[]? tags, TimeSpan validFor, CancellationToken cancellationToken)
 {
 var distributedCacheEntryOptions = new DistributedCacheEntryOptions { AbsoluteExpirationRelativeToNow = validFor };
 await _distributedCache.SetAsync(key, value, distributedCacheEntryOptions, cancellationToken);
 await AddKeyToTagSet(key, tags, cancellationToken).ConfigureAwait(false);
 }
 private async Task AddKeyToTagSet(string key, string[]? tags, CancellationToken cancellationToken)
 {
 if (tags == null)
 {
 return;
 }
 await ConnectAsync(cancellationToken).ConfigureAwait(false);
 foreach (var tag in tags)
 {
 await _cache!.SetAddAsync($"tag_{tag}", key).ConfigureAwait(false);
 }
 }
 
 private async Task ConnectAsync(CancellationToken token = default)
 {
 token.ThrowIfCancellationRequested();
 if (_cache != null)
 {
 Debug.Assert(_connection != null);
 return;
 }
 await _connectionLock.WaitAsync(token).ConfigureAwait(false);
 try
 {
 if (_cache == null)
 {
 if (_options.ConnectionMultiplexerFactory is null)
 {
 if (_options.ConfigurationOptions is not null)
 {
 _connection = await ConnectionMultiplexer.ConnectAsync(_options.ConfigurationOptions).ConfigureAwait(false);
 }
 else
 {
 _connection = await ConnectionMultiplexer.ConnectAsync(_options.Configuration).ConfigureAwait(false);
 }
 }
 else
 {
 _connection = await _options.ConnectionMultiplexerFactory().ConfigureAwait(false);
 }
 PrepareConnection();
 _cache = _connection.GetDatabase();
 }
 }
 finally
 {
 _connectionLock.Release();
 }
 }
 private void PrepareConnection()
 {
 ValidateServerFeatures();
 TryRegisterProfiler();
 }
 private void ValidateServerFeatures()
 {
 _ = _connection ?? throw new InvalidOperationException($"{nameof(_connection)} cannot be null.");
 try
 {
 foreach (var endPoint in _connection.GetEndPoints())
 {
 if (_connection.GetServer(endPoint).Version < ServerVersionWithExtendedSetCommand)
 {
 _setScript = SetScriptPreExtendedSetCommand;
 return;
 }
 }
 }
 catch (NotSupportedException)
 {
 _setScript = SetScriptPreExtendedSetCommand;
 }
 }
 private void TryRegisterProfiler()
 {
 _ = _connection ?? throw new InvalidOperationException($"{nameof(_connection)} cannot be null.");
 if (_options.ProfilingSession != null)
 {
 _connection.RegisterProfiler(_options.ProfilingSession);
 }
 }
}
```

• Peter Csala: Can you please share with us which nuget packages are being used? StackExchange.Redis and Microsoft.Extensions.Caching.StackExchangeRedis?
• nop: @PeterCsala yup, only StackExchange.Redis and .NET 7.0

Answer 1

It seems a fairly decent implement and easy to read code for me.
Here are some refactor ideas:

SetScript class members

• As far as I can see the only difference is a single letter: HSET vs HMSET
• In order to reduce the possibility of typo(s) I would suggest to use a template string and two string.Format calls

private const string ScriptTemplate = @"
 redis.call('{0}', KEYS[1], 'absexp', ARGV[1], 'sldexp', ARGV[2], 'data', ARGV[4])
 if ARGV[3] ~= '-1' then
 redis.call('EXPIRE', KEYS[1], ARGV[3])
 end
 return 1";
private readonly string SetScript = string.Format(ScriptTemplate, "HSET");
private readonly string SetScriptPreExtendedSetCommand = string.Format(ScriptTemplate, "HMSET");

EvictByTagAsync

• I don't see any reason why do you need the memberKeys variable
• You could simply combine the two Linq queries into a single

var redisKeys = _cache!.SetMembers($"tag_{tag}")
 .Select(value => new RedisKey(value.ToString()))
 .ToArray();

SetAsync

• It may or not may be a problem but the _distributedCache.SetAsync and AddKeyToTagSet are not treated atomically
  • It can happen that the first command succeeds but the second fails
  • It could cause inconsistency which could cause further problems later

AddKeyToTagSet

• It might make sense to move the null check of the tags to the caller-side

await _distributedCache.SetAsync(key, value, distributedCacheEntryOptions, cancellationToken);
if(tags is not null)
 await AddKeyToTagSet(key, tags, cancellationToken).ConfigureAwait(false);

• It might also make sense to replace the sequential calls of SetAddAsync to parallel

var addTasks = tags.Select(tag => _cache!.SetAddAsync($"tag_{tag}", key));
await Task.WhenAll(addTasks).ConfigureAwait(false);

ConnectAsync

• Personally I don't like multi-level if-else branching
  • The entire try block could be refactored like this:

if (_cache != null) return;
Func<Task<ConnectionMultiplexer>> connectVariant = _options.ConfigurationOptions is not null
 ? () => ConnectionMultiplexer.ConnectAsync(_options.ConfigurationOptions)
 : () => ConnectionMultiplexer.ConnectAsync(_options.Configuration);
_connection = _options.ConnectionMultiplexerFactory is not null
 ? await _options.ConnectionMultiplexerFactory().ConfigureAwait(false)
 : await connectVariant().ConfigureAwait(false);
//I've just inlined `PrepareConnection`
ValidateServerFeatures();
TryRegisterProfiler();
_cache = _connection.GetDatabase();

• Unfortunately we can't combine all three options into single expression
  • Due to the incompatibility of Task<ConnectionMultiplexer> and Task<IConnectionMultiplexer>

ValidateServerFeatures

• I've found quite confusing that
  • sometimes you use the null forgiving operator (so called damn-it operator)
  • other times null coalescing operator with throw expression
  • and you also have Debug.Assert calls
• I think using a single option consistently helps the maintainability of your code
• I would suggest to move the try-catch block inside the loop

bool shouldUpdateSetScript = false;
foreach (var endPoint in _connection.GetEndPoints())
{
 try
 {
 var version = _connection.GetServer(endPoint).Version;
 if (version < ServerVersionWithExtendedSetCommand)
 shouldUpdateSetScript = true;
 }
 catch (NotSupportedException)
 {
 shouldUpdateSetScript = true;
 }
 
 if (shouldUpdateSetScript) break; 
}
if(shouldUpdateSetScript)
 _setScript = SetScriptPreExtendedSetCommand; 

• Or it might make sense to use a while loop instead of for

bool shouldUpdateSetScript = false;
var endpointIterator = _connection.GetEndPoints().Cast<EndPoint>().GetEnumerator();
while (!shouldUpdateSetScript || endpointIterator.MoveNext())
{
 try
 {
 var version = _connection.GetServer(endpointIterator.Current).Version;
 if (version < ServerVersionWithExtendedSetCommand)
 shouldUpdateSetScript = true;
 }
 catch (NotSupportedException)
 {
 shouldUpdateSetScript = true;
 }
}
if(shouldUpdateSetScript)
 _setScript = SetScriptPreExtendedSetCommand; 

• nop: Thank you very much for the descriptive answer! :)