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);
        }
    }
}

```

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;