auto_close_connection_pool ignored

[kristjanvalur]

This defect pertains to issues discovered with #2831

In async mode, it is important that a Redis object which holds its own pool, can be told to close it when the Redis object
is closed, to free the user from such hassles. Examples in the code include

from redis.asyncio.client import Redis
client = Redis(connection_pool=BlockingConnectionPool())

when this client is closed, the internal connection pool remains open, i.e. its connections are not shut down.

The Redis class includes a auto_close_connection_pool argument. However, this is ignored when a connection pool
is provided to the constructor. Presumably the thinking is that the caller will manage that pool himself.
But that makes the above kind of pattern, also used within the code, unusable.

client = Redis(connection_pool=BlockingConnectionPool(), auto_close_connection_pool=True)
assert client.auto_close_connection_pool=False

Instead, it is necessary to have code like this, which is very poor practice:

client = Redis(connection_pool=BlockingConnectionPool(), auto_close_connection_pool=True)
client.auto_close_connection_pool=True

I propose, that we either:

• Always honour the auto_close_connection_pool argument.
• add a own_connection_pool:ConnectionPool argument, to be used instead of connection_pool.
• rename auto_close_connection_pool to own_pool, or something less cumbersome.
• add a construct_connection_pool argument which takes a callable which returns a connection pool, one which Redis considers its own.

At any rate, it should be easy to create a connection pool, and hand it to the Redis constructor for it to "own" without
having to fudge an internal property afterwards.

In addition, I propose that the same mechanics be added to the non-async parts of the library. Currently the redis.Redis objects do not close connection pools, instead the library relies on the prompt execution of __del__() methods, or explicit ConnectionPool.disconnect() calls.
Relying on garbage collection to free resources is considered bad programming practice in Python, where resource management should in general be more explicit. It is particularly frowned upon for async code, since executing asynchronous code there is not a good idea(*). the synchronous code should receive whatever solution we choose for the above as well, so that pools owned by a Redis client object are also disconnected when those Redis objects are closed.

(*) __del__() methods are not async. While it is possible to use something like asyncio.run within such a function, it will block the rest of the program. Also, __del__() methods are often invoked during program cleanup when it is simply not possible to do anything and trying to do so will produce errors and confusion.

Update

In pr #2913 I have updated a Redis.from_pool() method to use when handing over a ConnectionPool to be owned by the Redis instance. I think this is a much nicer way than to heap on more constructor arguments to Redis