Dev/no can read

[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

The asyncio.Connection.can_read() API is unnecessary in async-io, since polling a connection is best done with timeout.
Keeping this api adds complexity in the lower layers of the asyncio.Connection since it requires reading data and then "unreading" them for subsequent read operations.

Further, this code is almost exclusively used by assertion macros, to make sure that there is no data present on a connection pulled from a connection pool. For this use case, it is fine that the read data is discarded.

The only remaining use case was in the PubSub implementation, but that can be done differently.

Renaming the method to can_read_destructive() and simplifying it, getting rid of the "keep any read data for next read" logic, paves the way for further simplifications of the asyncio connection code, and other pull requests in the same ares (getting rid of locking, buffering, simplifying timeouts, etc)

The PR contains elements from Pull Requests #2356 and #2104 as necessary for it to work properly

[codecov-commenter]

Codecov Report

Base: 78.38% // Head: 92.16% // Increases project coverage by +13.77% 🎉

Coverage data is based on head (75f447e) compared to base (6b3e0b4).
Patch coverage: 92.10% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2360       +/-   ##
===========================================
+ Coverage   78.38%   92.16%   +13.77%     
===========================================
  Files         110      110               
  Lines       27976    28857      +881     
===========================================
+ Hits        21929    26595     +4666     
+ Misses       6047     2262     -3785     

Impacted Files	Coverage Δ
redis/asyncio/connection.py	86.17% <86.95%> (+2.28%)	⬆️
redis/asyncio/client.py	92.33% <100.00%> (+0.42%)	⬆️
tests/test_asyncio/test_cluster.py	97.28% <100.00%> (+78.45%)	⬆️
tests/test_asyncio/test_connection_pool.py	99.40% <100.00%> (+0.02%)	⬆️
tests/test_asyncio/test_pubsub.py	99.49% <100.00%> (+0.18%)	⬆️
tests/test_multiprocessing.py	78.78% <0.00%> (-21.22%)	⬇️
redis/connection.py	86.30% <0.00%> (-0.95%)	⬇️
tests/test_retry.py	100.00% <0.00%> (ø)
tests/test_pipeline.py	100.00% <0.00%> (ø)
tests/test_timeseries.py	100.00% <0.00%> (ø)
... and 62 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dvora-h]

@kristjanvalur Can you resolve the conflicts with master?

[dvora-h]

@kristjanvalur again...
Sorry for all of this noise, but I don't have permissions to push to your branches so I can't do it myself.

[kristjanvalur]

No problem, it is best that I resolve these myself anyway.

[Chronial]

This PR broke async PubSub.get_message():

• It changes the interface in an unexpected way: Before, the timeout parameter was a timeout for waiting for a new message. Now it is a timeout for the whole receive and parse operation, i.e. it will interrupt message receiving/parsing if it takes too long.
• If that happens (receive interrupted), the currently processing message gets dropped and the connection corrupted. In the past, this would have been detected and the connection closed. But that check was removed in Catch Exception and not BaseException in the Connection #2104. Now this PR just leaves the Connection of the PubSub in a corrupted state and following operations on that connection will fail in unexpected ways.

The timeout parameter should only affect the wait operation and never cause messages to be dropped.

[kristjanvalur]

Thanks for pointing this out. I've created a separate issue, #2513