Add Redis.from_pool() class method, for explicitly owning and closing a ConnectionPool

[kristjanvalur]

Pull Request check-list

• Does $ tox pass with this change (including linting)?
• Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Is there an example added to the examples folder (if applicable)?
• Was the change added to CHANGES file?

Description of change

Traditionally, synchronous redis-py has relied on __del__ to disconnect any ConnectionPool owned by a Redis object.
Explicitly closing those has been possible, via client.connection_pool.disconnect(), but this does not work well with
using context managers and other approaches. Relying on __del__() to clean up sockets is not considered good practice since it relies on the peculiarities of the particular implementation, garbage collection and such things.

In async redis, a mechanism was added to make an asyncio.Redis object own a connection and close it, to provide
better connection over connection lifetime. This was done here:
aio-libs-abandoned/aioredis-py#1256
This mechanism carried over when that library was added to redis-py, and we increasinly rely on it to properly clean up sockets used in asynchronous mode. However, the mechanism added was cumbersome to use and overly verbose. In particular, it made it awkward to create a custom ConnectionPool instance, hand it to a new Redis object, and expect it to close it automatically.

This PR, aims to:

1. fix auto_close_connection_pool ignored #2901, adding a new from_pool() class method to use instead of the auto_close_connection_pool constructor argument.
2. apply the same semantics to the synchronous Redis.
3. Discourage the use of the auto_close_connection_pool argument.

The following now applies for both synchronous and asynchronous redis:

• A Redis object created without providing a ConnectionPool instance, e.g. via Redis() or Redis.from_url(), will now automatically call connection_pool.disconnect() when call() is closed.
• A Redis object created Redis.from_pool() similarly takes ownership of the pool, and will disconnect the pool.
• As before, if the connection_pool argument is used by Redis.__init__() this is not done, instead it is assumed the caller
  will close the pool.
• A Sentinel object, will use a from_pool() class method constructing a Redis object to return from the master_for() and slave_for() methods.
• An auto_close_connection_pool Redis.__init__() argument is not provided for redis.Redis, but kept in place for backwards compatibility for redis.asyncio.Redis

The above behaviour is controlled by the auto_close_connection_pool property of Redis.

[codecov-commenter]

Codecov Report

Patch coverage: 98.10% and project coverage change: +0.02% 🎉

Comparison is base (509c77c) 91.35% compared to head (b34c890) 91.38%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2913      +/-   ##
==========================================
+ Coverage   91.35%   91.38%   +0.02%     
==========================================
  Files         126      126              
  Lines       32335    32469     +134     
==========================================
+ Hits        29540    29671     +131     
- Misses       2795     2798       +3     

Files Changed	Coverage Δ
redis/asyncio/connection.py	83.70% <ø> (ø)
tests/test_asyncio/test_connection.py	93.72% <96.72%> (+0.75%)	⬆️
tests/test_connection.py	98.27% <97.95%> (-0.14%)	⬇️
redis/asyncio/client.py	92.30% <100.00%> (+0.15%)	⬆️
redis/asyncio/sentinel.py	83.92% <100.00%> (-0.56%)	⬇️
redis/client.py	91.94% <100.00%> (+0.13%)	⬆️
redis/sentinel.py	86.95% <100.00%> (ø)
tests/test_sentinel.py	99.40% <100.00%> (+0.05%)	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[chayim]

This breaks the interface...

[chayim]

Here, my concern is primarly around breaking the connection interface. I realize that async is in flux, and frankly many things will be - but this interface is public and exposed, and not internal. Maybe there's another way - without breaking the user contract?

[kristjanvalur]

I understand. I think we can leave this attribute in place. I'm sure you agree that it feels cludgy, and long.
Do we have a standard schedule and mechanism to deprecate functionality / arguments in redis-py?

I'm re-adding it in code, along with a comment on why I think we should try to phase it out.
basically, if one does not want it auto closed, one should hold a reference to it already, and so it is better to create the pool before passing it to the Redis constructor, rather than fishing it out of Redis later from an internal attribute.

i.e. this is a healthier pattern

pool = ConnectionPool.from_url(url)
client = Redis(connection_pool=pool)
...
pool.close()

rather than this:

client = Redis.from_url(url, auto_close_connection_pool=False)
pool = client.connection_pool   # need to fish out the pool so that we can close it later.
...
pool.close()

[chayim]

I definitely agree is kludgey...

I generally dislike these interface (sync and asycn.. heck even cluster). But we should break accordingly in the future.