When using the FailoverClusterClient, TCP connections to Sentinel are not closed by ClusterClient.Close().
Expected Behavior
Calling ClusterClient.Close() should leave no established connections.
Current Behavior
ClusterClient.Close() only closes connections in the node pool, not the sentinel connections themselves.
Possible Solution
I'm not super familiar with the code here, but FailoverClient does not exhibit this same behaviour. I notice that NewFailoverClient has this line: https://github.com/redis/go-redis/blob/master/sentinel.go#L616
NewFailoverClusterClient does not have the same onClose registration that I can see. To verify this as the cause, I built my binary with #4000 applied and confirmed that sentinel conns stopped leaking.
Steps to Reproduce
- Create a few
FailoverClusterClient objects via NewFailoverClusterClient and call Close on them
- Observe that there are still open connections to the configured Sentinel port using
netstat
Context (Environment)
This hit me when using Redis as a store for Caddy. The way Caddy config reloads work results in a lot of potentially-temporary clients being initialized, as they get initialized as part of the reload but then discarded if some part of the config reload fails (whether in the Storage module or otherwise). I initially thought the Caddy storage module we have was not calling Close on the redis client but confirmed it was. A pathological config reload failure loop in Caddy caused runaway Sentinel connection leakage, which uncovered this bug that otherwise was just a small amount of connection over time so went unnoticed.
Detailed Description
PR proposes registering the onClose handler, same as the FailoverClient currently does.
Possible Implementation
#4000
When using the
FailoverClusterClient, TCP connections to Sentinel are not closed byClusterClient.Close().Expected Behavior
Calling
ClusterClient.Close()should leave no established connections.Current Behavior
ClusterClient.Close()only closes connections in the node pool, not the sentinel connections themselves.Possible Solution
I'm not super familiar with the code here, but
FailoverClientdoes not exhibit this same behaviour. I notice thatNewFailoverClienthas this line: https://github.com/redis/go-redis/blob/master/sentinel.go#L616NewFailoverClusterClientdoes not have the sameonCloseregistration that I can see. To verify this as the cause, I built my binary with #4000 applied and confirmed that sentinel conns stopped leaking.Steps to Reproduce
FailoverClusterClientobjects viaNewFailoverClusterClientand callCloseon themnetstatContext (Environment)
This hit me when using Redis as a store for Caddy. The way Caddy config reloads work results in a lot of potentially-temporary clients being initialized, as they get initialized as part of the reload but then discarded if some part of the config reload fails (whether in the Storage module or otherwise). I initially thought the Caddy storage module we have was not calling
Closeon the redis client but confirmed it was. A pathological config reload failure loop in Caddy caused runaway Sentinel connection leakage, which uncovered this bug that otherwise was just a small amount of connection over time so went unnoticed.Detailed Description
PR proposes registering the
onClosehandler, same as theFailoverClientcurrently does.Possible Implementation
#4000