dotnet
Caching.StackExchangeRedis: add "force reconnect" pattern
Caching.StackExchangeRedis: add "force reconnect" pattern
Adds the "force reconnect" pattern as described in azure cache best practices, with an implementation modified to retain the pre-existing custom connect logic
Tests forced locally (they're [Skip] in the source), and observed with redis-cli monitor to validate that key-prefix still valid.
Description
- add "force reconnect" pattern via OnRedisError
- avoid thread-race problems by having Connect[Async] return the cache, rather than relying on field
- avoid thread-race problems by only using a single ref field (cache), rather than cache+muxer; access muxer indirectly (it is only ever needed to shut down)
- use more efficient key-prefix path via RedisKey.Append
- use more efficient hash-fetch by pre-storing the hash-key arrays (removes need for RedisExtensions.cs)
- general code tidy; null-handling, defaults, etc (no semantic change)
Fixes #39082
/cc @philon-msft re eyeball of core reconnect loop (absorbed from https://github.com/Azure-Samples/azure-cache-redis-samples/blob/main/quickstart/dotnet-core/RedisConnection.cs#L89)
@BrennanConroy should we add this to the SignalR backplane as well?
Also @ReubenBond should this be part of the redis clustering provider for orleans?
For the other projects: if using the StackExchange.Redis client, we'd recommend upgrading to latest (2.6.x) and see if you actually hit any issues. If so, please ping ASAP and let's fix those in the core library. This entire feature shouldn't be needed any more - if there are some scenarios still tossing the connection (and anything buffered, which the base library would send upon connecting up to the timeout), let's figure that out and improve for all. The workaround is inherently a bit destructive compared to the base library, and should only be a last resort.
For context: most of the need for something like this was before working with the Azure Redis cache folks to understand scenarios previously unknown and handling everything we're aware of today. TL;DR: it's mostly outdated advice, and the defaults should be doing good things today on latest library bits.
This entire feature shouldn't be needed any more
So why are we doing this PR then?
@BrennanConroy because people keep finding new and exotic ways of making things go pear-shaped, and this allows us to mitigate while we investigating and resolve at the library level; which is also why it is off by default
We're going to add public api for a feature that we shouldn't need to write and might be able to remove in the future? Or, if we think it's important enough to add support for it because "people keep finding new and exotic ways of making things go pear-shaped", then why not add it to our other redis libraries too?
@BrennanConroy should we add this to the SignalR backplane as well?
Also @ReubenBond should this be part of the redis clustering provider for orleans?
what I would recommend is making sure you rev the lib occasionally; since me and Nick came over to MSFT and have been able to work more closely with some internal teams, we've made a bunch of stability improvements that I highly recommend
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}