Implement ForceReconnect

[simonpinn]

Hi @AliBazzi

We've seen some instances while running in Azure Redis when a Redis failover has caused the ConnectionMultiplexer to not retry the connection (instead requiring a restart of the service), this isn't a great situation!
To avoid this in other projects we use the ForceReconnect pattern from the Redis

https://gist.github.com/JonCole/925630df72be1351b21440625ff2671f#file-redis-bestpractices-stackexchange-redis-md from
https://docs.microsoft.com/en-gb/azure/azure-cache-for-redis/cache-best-practices

It does require changing the Get / Set to check and reconnect to something like:

public async Task<bool> AddAsync<T>(string key, T value, TimeSpan expiresIn)
        {
            try
            {
                try
                {
                    return await _cacheClient.AddAsync(key, value, expiresIn);
                }
                catch (RedisConnectionException)
                {
                    RedisConnection.ForceReconnect();
                }
                catch (SocketException)
                {
                    RedisConnection.ForceReconnect();
                }
                catch (WebSocketException)
                {
                    RedisConnection.ForceReconnect();
                }
            }
            catch (ObjectDisposedException)
            {
                //ForceReconnect can potentially throw ObjectDisposedException, nothing we can do, hopefully cache back on next call
                return false;
            }
            
            return false;
    }

Let me know if you'd consider adding this, or if a PR would be accepted.

thanks

[AliBazzi]

Hi @simonpinn , I'm not sure if this is something specific to Azure, but this is aggressive to force reconnection when an exception happens, and I'm not considering adding it as default behaviour to the library, but the good news is that you can customize the library, I would suggest to you to do the following:

keep using the library, but do define your own custom class the derives from PersistedGrantStore, override each function in the IPersistedGrantStore interface like:

public class CustomPersistedGrant : IdentityServer4.Contrib.RedisStore.Stores.PersistedGrantStore
    {
        public CustomPersistedGrant(...)
            : base(...)
        {
        }

        public override Task<IEnumerable<PersistedGrant>> GetAllAsync(PersistedGrantFilter filter) { 
                //your custom logic goes here for error handling after calling the base class respective function
               //you can access the connection via options.RedisConnectionMultiplexer and do whatever suits you
        } 
        ...
   } 

After you register the library as advised in the readme file, you should register the new CustomPersistedGrant class after you register the library as:

identityServerBuilder.Services.AddScoped<IPersistedGrantStore, CustomPersistedGrant>();

hope that will allow you to cover your requirement.

[simonpinn]

Hi @AliBazzi

I agree that this behaviour is something I've only encountered in Azure, in more than one application, and the guidance has come from MS to perform the force reconnect. Also agree that it is quite aggressive, though only when an error occurs.

Understand the position of your library, and yes, that extension will work for us, greatly appreciate the response.

cheers

[AliBazzi]

@simonpinn you are welcome !

[simonpinn]

Hey @AliBazzi

Actually, upon implementation I'm thwarted by how access to the underlying multiplexer is protected.

The main issue is the private GetDatabase method in RedisMultiplexer even providing my own inherited RedisOptions (where I would override the RedisConnectionMultiplexer prop), but I'm caught by the use of internal IConnectionMultiplexer Multiplexer => this.multiplexer.Value; basically making it impossible to manage my own lifecycle for the connection...
Previously we used https://github.com/imperugo/IdentityServer3.Contrib.Cache.Redis for ID3, which allowed us to provide our own CacheManager

It'd be great if we could provide more control over the underlying multiplexer. Open to ideas!

thanks

[AliBazzi]

But the options.RedisConnectionMultiplexer is public, and my understanding is that you want to call a function on it is ForceReconnect right ?

I'm not following, if you can provide a sample repo that would be great.

[simonpinn]

Not quite as simple, since the Options class in your project is a singleton, and calls GetDatabase once, the implementation to ForceReconnect recreates the LazyConnectionMultiplexer (https://gist.github.com/JonCole/925630df72be1351b21440625ff2671f#file-redis-bestpractices-stackexchange-redis-md) however in IdentityServer4.Contrib.RedisStore this is performed internally in the abstract RedisOptions class method GetConnectionMultiplexer. We then have no chance to control the lifetime of the LazyConnectionMultiplexer.

Also options.RedisConnectionMultiplexer is only a property, be great if this was a Func, and even better if it was to provide the actual LazyConnectionMultiplexer not just the connection.

[AliBazzi]

if you can @simonpinn a gist or a GitHub project I can follow it will be great

[simonpinn]

Hi @AliBazzi

Sorry I can't share my clients repo. However the issue is more about the class structure and protection levels chosen within this library.

For example https://github.com/AliBazzi/IdentityServer4.Contrib.RedisStore/blob/master/IdentityServer4.Contrib.RedisStore/Cache/RedisCache.cs#L23 has a dependency on a concrete type RedisMultiplexer<RedisCacheOptions> so even by providing my own I cannot get around the base implementation which instantiates the IDatabase https://github.com/AliBazzi/IdentityServer4.Contrib.RedisStore/blob/master/IdentityServer4.Contrib.RedisStore/Extensions/RedisMultiplexer.cs#L14

You'll notice from https://gist.github.com/JonCole/925630df72be1351b21440625ff2671f#file-redis-bestpractices-stackexchange-redis-md that control over the lifetime of the Lazy<ConnectionMultiplexer> is what's required.

If this library took it's dependencies using interfaces rather than concrete types (like the ID3 Redis library https://github.com/imperugo/IdentityServer3.Contrib.Cache.Redis), or was open to extension by passing the control of the multiplexer to the consumer, then this could be avoided. However due to how https://github.com/AliBazzi/IdentityServer4.Contrib.RedisStore/blob/master/IdentityServer4.Contrib.RedisStore/Extensions/RedisOptions.cs#L71 even providing my own RedisOptions will still call your concrete implementation for GetConnectionMultiplexer

Happy if I'm wrong! So any ideas are greatly appreciated.

cheers