Redis: Renew the underlying connection if there is an exception #1004
Conversation
|
@sungam3r PTAL |
| @@ -63,6 +63,7 @@ public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context | |||
| } | |||
| catch (Exception ex) | |||
| { | |||
| _connections.TryRemove(_redisConnectionString, out _); | |||
There was a problem hiding this comment.
I don't get the first part, sorry.
Would you like that, instead of using TryGetValue/TryAdd/TryRemove I update it to match the approach from the PR that you have posted?
Related with IDisposable you are right, I'll update my PR with that 😄
There was a problem hiding this comment.
Would you like that, instead of using TryGetValue/TryAdd/TryRemove I update it to match the approach from the PR that you have posted?
Yes. In fact that should be done in many places to get rid of race conditions.
There was a problem hiding this comment.
Thanks for the clarification ❤️
I'll update my PR with both changes
There was a problem hiding this comment.
I'm doing the change and I have faced with a problem using GetOrAdd, it hasn't got an async method. That pushes me to use something like this:
var connection = _connections.GetOrAdd(_redisConnectionString, async key =>
{
return ConnectionMultiplexer.Connect(key);
}); My concern here is about doing sync calls inside the async context of the health check execution. Maybe I create a bigger problem with that than the original race condition with TryAdd/TryGet/TryRemove.
Any thoughts/suggestion @sungam3r ?
There was a problem hiding this comment.
OK. Let's keep as is for now.
|
Thanks for merging :) |
|
Hi Jorge, I think I will release this package this weekend.
El jue., 10 mar. 2022 20:09, Jorge Turrado Ferrero ***@***.***>
escribió:
… Thanks for merging :)
One question, when will be next release? (no rush, just for knowing)
—
Reply to this email directly, view it on GitHub
<#1004 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALZUJ2H7IR5A6AZ5H7IPXLU7JCHHANCNFSM5QMRJ7KA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: <Xabaril/AspNetCore.Diagnostics.HealthChecks/pull/1004/c1064399120@
github.com>
|
|
wow, so fast! Thanks a ton ❤️ |
|
Published on package 6.0.3 |
|
❤️ ❤️ ❤️ ❤️ |
What this PR does / why we need it: This PR solves the reconnection problem in Redis with Sentinel arch after master node is missing
Which issue(s) this PR fixes: Fixes #1003
Please make sure you've completed the relevant tasks for this PR, out of the following list: