AliBazzi / IdentityServer4.Contrib.RedisStore

A persistence layer using Redis DB for operational data and for caching capability for Identity Server 4
https://www.nuget.org/packages/IdentityServer4.Contrib.RedisStore
MIT License
137 stars 48 forks source link

Change from lazy to double check locking to avoid caching exceptions when initialize #49

Closed mknet3 closed 3 years ago

mknet3 commented 3 years ago

Fix #48

Avoiding change LazyThreadSafetyMode to avoid concurrent problems. Instead of will be use double check locking.

AliBazzi commented 3 years ago

Hi @m-knet can you elaborate why you didn't use what is suggested in the Issue you've opened ?

your current implementation in this PR will delay the connection creation until the first time the multiplexer is needed, while the current implementation will attempt to connect on the initialisation, why not just apply what you've suggested ?

mknet3 commented 3 years ago

Hi @AliBazzi, I didn't used LazyThreadSafetyMode because I was wrong about the default value of this property. The default value is ExecutionAndPublication. This means that lazy initialization is thread safe but is caching the exception if fails when is initialized.

None is not thread safe and is better avoid it.

PublicationOnly is not caching the exception and is thread safe. It was the first option to choose (seems that fit with solution). But this solution has a problem: The first thread to complete initialization sets the value of the Lazy instance.. This means that can have multiple threads trying to initialize the instance and trying to open connections with Redis.

AliBazzi commented 3 years ago

Hi @m-knet ,

Thanks for the clarification, but still, your changes is behavioural changes to the library, for example, in my case, I do want the instance to fail on initialisation if it was not successful in connecting to Redis, you are differing this to the first connection attempt, and that will not fail the instance.

So in my use case, I do want the instance to fail and report unhealthy if it wasn't able to connect to Redis on initialisation.