search

FZambia / sentinel

Redis Sentinel support for Redigo library
Apache License 2.0
69 stars 24 forks source link

Sentinel pool closing mechanism is not goroutine safe #2

Closed dpnm closed 5 years ago

dpnm commented 5 years ago

doUntilSuccess error closes the Pool shared by all other goroutines.

Pool close: https://github.com/FZambia/sentinel/blob/master/sentinel.go#L228 Pool Get: https://github.com/FZambia/sentinel/blob/master/sentinel.go#L167

This will lead to error logs like: time="2019-06-05T13:58:42Z" level=error msg="redigo: no sentinels available; last error: redigo: get on closed pool"

FZambia commented 5 years ago

@dineshpanchananam thanks for report, I missed notification somehow. Actually my first question here is why we close pool at that place – I can't find a good reason to do this at moment.

dpnm commented 5 years ago

@FZambia From my testing, this happens when the request to fetch current MASTER_IP or SLAVE_IPS times out. Yes, there is no need to close the pool, close the connection maybe?

FZambia commented 5 years ago

Yes, there is no need to close the pool, close the connection maybe?

We already close connection here. Or what do you mean?

My concern is that it's not actually required to close pool after each error, at least I can't understand why I did it this way. Need to test it out.

dpnm commented 5 years ago

My oversight. Yes, we are closing the connection there. But you are also right that we should never close the pool, which is a shared resource

FZambia commented 5 years ago

@dineshpanchananam thanks for your help! Created tag v1.1.0 with fix from #3

dpnm commented 5 years ago

No problem!

On Sat, Aug 24, 2019 at 11:21 AM Alexander Emelin notifications@github.com wrote:

@dineshpanchananam https://github.com/dineshpanchananam thanks for your help! Created tag v1.1.0 with fix from #3 https://github.com/FZambia/sentinel/pull/3

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FZambia/sentinel/issues/2?email_source=notifications&email_token=AAR5NXLKBPAKOBPPTD7DBOLQGFGZHA5CNFSM4HVCLBB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5CCFJY#issuecomment-524559015, or mute the thread https://github.com/notifications/unsubscribe-auth/AAR5NXLZL6J6N3YRWKJEBX3QGFGZHANCNFSM4HVCLBBQ .