search

Grokzen / redis-py-cluster

Python cluster client for the official redis cluster. Redis 3.0+.
https://redis-py-cluster.readthedocs.io/
MIT License
1.1k stars 316 forks source link

fix: possible connection leak in pipeline when max_connections is reached #420

Open Neon4o4 opened 3 years ago

Neon4o4 commented 3 years ago

When max_connections is reached connection_pool.get_connection_by_node will throw an error (Too many connections). However other connections allocated before the error are clean and should be returned to connection pool.

Grokzen commented 3 years ago

@Neon4o4 Is this the same behavior that redis-py is using? Also are you considering that it might not be that good to release connections to other server/nodes in your cluster as it seems like you are doing that as well and not only connections to a specific node?

Neon4o4 commented 3 years ago

@Grokzen Thanks for the comment

Is this the same behavior that redis-py is using?

I think they are not the same. redis-py's pipeline keeps used connection with self.connection and releases it in reset() (called in finally block) https://github.com/andymccurdy/redis-py/blob/master/redis/client.py#L4150

For redis-py-cluster, connections are released after read/writes. If error occurred when making new connections (e.g. Too many connections) these connections would be considered as used and will never be released. https://github.com/Grokzen/redis-py-cluster/blob/master/rediscluster/pipeline.py#L233 I noticed the comments after read/writes about why not releasing connections in finally block and I do agree with it. However if error occurred even before any operation is performed on these connections, it should be safe to release them.

it might not be that good to release connections to other server/nodes in your cluster

Actually I'm trying to release the connections. I think they should be released because we run into an error when making new connections and these connections are not used and will not be used in this pipeline execution. Connections to other nodes will be considered as "used" if they are not released. If that happened we would never be able to use these connections in the future. And if we have max_connections set and got enough un-released connections, we would not be able to make any new connections with this cluster object. For some micro-service setups, there is only one cluster object per process.

Neon4o4 commented 3 years ago

@alivx Hi, thanks for the comment.

use timeout option and retry in X time for any available connection

Sure. I will update the code.