2
\$\begingroup\$

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

```
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Can you please share with us which nuget packages are being used? StackExchange.Redis and Microsoft.Extensions.Caching.StackExchangeRedis? \$\endgroup\$ Commented Jan 27, 2023 at 13:52
  • \$\begingroup\$ @PeterCsala yup, only StackExchange.Redis and .NET 7.0 \$\endgroup\$
    – nop
    Commented Jan 27, 2023 at 18:07

1 Answer 1

3
+100
\$\begingroup\$

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;   
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you very much for the descriptive answer! :) \$\endgroup\$
    – nop
    Commented Jan 28, 2023 at 15:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.