Open simonpinn opened 3 years ago
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.
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
@simonpinn you are welcome !
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
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.
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.
if you can @simonpinn a gist or a GitHub project I can follow it will be great
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
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:
Let me know if you'd consider adding this, or if a PR would be accepted.
thanks