Fix socket garbage collection

[kristjanvalur]

Pull Request check-list

Please make sure to review and check all of these items:

• 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?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Issue #2831 shows how connections are kept open when they are simply discarded.
The problem is two-fold:

1. Redis connections opened with from-url don't have their auto_close_connection boolean set to True, as is the default for a pool which is created automatically. Therefore, when Redis.close() was called, the pool would not close its collections.
2. Even so, garbage collection of the Connection objects ought to have closed the sockets associated with the underlying StreamWriter objects. This apparently does not reliably happen, so we re-instate a __del__ handler for the AbstractConnection which does the bare minimum required, calling StreamWriter.close().
3. Add an auto_close_connection_pool argument to asyncio.Redis.from_url() which defaults to true, same as for asyncio.Redis()

A fix was added to an unrelated test which caused unit tests to fail on non-linux platforms.

Update

I've changed the PR so as to remove the 2, above. In general for asyncio Python, socket cleanup is not done on garbage collection (since it involves async operations). Instead, it is assumed that a well written program performs proper disconnects as needed. __del__ handlers instead are used to emit warnings, that is, if a socket (or other resource) is found to be not closed when purging an object, a warning is emitted. This is to make sure resources are released properly by the application.

Fixing of (1) above ensures that the connection pool is indeed cleaned up correctly. Trying to put cleanup and IO logic into destructors is in general not thought to be a good practice and should be avoided whenever possible. Correct program operation should not rely on the prompt execution of __del__ handlers, and implementing them would deprive us of the built-in resource warnings which would otherwise be emitted.

[codecov-commenter]

Codecov Report

Patch coverage: 70.21% and project coverage change: +0.31% 🎉

Comparison is base (2732a85) 90.66% compared to head (34a69ff) 90.98%.
Report is 1 commits behind head on master.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2859      +/-   ##
==========================================
+ Coverage   90.66%   90.98%   +0.31%     
==========================================
  Files         126      126              
  Lines       32229    32295      +66     
==========================================
+ Hits        29221    29384     +163     
+ Misses       3008     2911      -97     

Files Changed	Coverage Δ
tests/test_connect.py	87.21% <22.22%> (-11.24%)	⬇️
redis/asyncio/client.py	91.99% <100.00%> (-0.16%)	⬇️
tests/test_asyncio/test_connection.py	92.93% <100.00%> (+1.05%)	⬆️

... and 13 files with indirect coverage changes

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

[kristjanvalur]

@dvora-h you may want to see the latest change. Arguments intended for the "Redis" class are now explicit in from_url, while the kwargs get passed to the ConnectionPool constructor. This makes the docstring more correct.