Ensure that the connections obtained from the ConnectionPool are ready to send a comand.

[sateeshkumarb]

Looking at the ConnectionPool implementation provided by redis-py I notice that upon getting a connection object from the pool redis-py has additional checks to ensure that all the data has been completely read from the socket before the connection object was freed, and the socket has been closed. (https://github.com/andymccurdy/redis-py/blob/master/redis/connection.py#L1147).

However in ClusterConnectionPool I don't see such checks in get_connection. Won't the checks that are done in get_connection of redis-py need to be done when we get a connection from the ClusterConnectionPool.

It seems to me, not having such checks could lead to a scenario where a new cluster connection reads the residual data from previous connection. Sorry, I couldn't get a test verifying that not having such a check in ClusterConnectionPool leads to problems as it is bit difficult to simulate such a scenario.

Thanks for all your work in creating this module, which is very useful.

[Grokzen]

Yeah that is very true and a good find. When i made the first implementation i wanted to use the get_connection() parent method from Redis itself but it was not possible to tie them together so a full custom implementation was needed. I do not like that and i really want to reuse more methods from redis-py itself and i hope that i can get that working for 3.0.0 as i have planned and made progress on with a brand new connectionpool solution.

But in the meantime it should be rather easy to add this feature to the make_connection method as that is the one that creates the connection in here https://github.com/Grokzen/redis-py-cluster/blob/master/rediscluster/connection.py#L239

[sateeshkumarb]

@Grokzen Thanks for your response.

Looking at the code it seems to me adding the checks at make_connection alone might not be enough. As I see it the checks are needed not only when the new connection is created, and also when any existing connection available from the connection pool is reused (i.e. https://github.com/Grokzen/redis-py-cluster/blob/master/rediscluster/connection.py#L228). It needs to be ensured that the connection that was put back to the pool had no residual data to be read and was properly closed. Or do I miss anything here ?

[Grokzen]

Oh yeah you are right it needs to be up there instead, yeah so possibly the get_connection should have it yes.

[aajay]

@Grokzen, I have created PR #430 to fix this issue. Could you review? I am not able to add you as a reviewer.

[Grokzen]

Fixed in 95fee797dac63501f2933c8a0b46d72dbc5250d9