Simplify async timeouts

[kristjanvalur]

Pull Request check-list

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

• [x] Does $ tox pass with this change (including linting)?
• [x] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• [x] Is the new or changed code fully tested?
• [x] 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)?
• [x] 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

async PubSub no longer does a separate can_read for every message read. Instead async timeout mechanisms are used at the highest level to time out operations.

The use of can_read is a holdover from the non-async times, when timeout had to happen at the lowest level, at the actual blocking socket call. With asyncio, timeouts happen higher up in the stack. This allows for simplifications, because there is no longer any need to have timeout code lower down in the stack.

can_read() is still kept around, but its only use now is assertions in the ConnectionPool. This could be simplified further, since having can_read() around requires additional code paths for read, but not consumed, data.

Obsolete code pertaining to blocking exceptions was also removed.

Additionally, get_message() now accepts timeout=None argument to wait indefinitely for a message.

[codecov-commenter]

Codecov Report

Base: 92.16% // Head: 92.20% // Increases project coverage by +0.03% :tada:

Coverage data is based on head (38351fe) compared to base (cdbc662). Patch coverage: 97.61% of modified lines in pull request are covered.

:exclamation: Current head 38351fe differs from pull request most recent head 305f848. Consider uploading reports for the commit 305f848 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2295      +/-   ##
==========================================
+ Coverage   92.16%   92.20%   +0.03%     
==========================================
  Files         110      110              
  Lines       28925    28899      -26     
==========================================
- Hits        26659    26645      -14     
+ Misses       2266     2254      -12     

Impacted Files	Coverage Δ
redis/asyncio/connection.py	87.13% <97.22%> (+0.98%)	:arrow_up:
redis/asyncio/client.py	92.06% <100.00%> (-0.27%)	:arrow_down:
redis/client.py	89.06% <100.00%> (ø)
tests/test_asyncio/test_pubsub.py	99.37% <100.00%> (ø)
tests/test_asyncio/test_search.py	98.75% <0.00%> (+0.31%)	:arrow_up:

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

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

[kristjanvalur]

I'm submitting this for review, even though there are one or two weird sporatic test failures that don't seem to have anything to do with this. (Note the added code for killing containers, seems to have fixed test stability when container ports were in-use when starting tox run, probably due to rest-runner being re-used.)

[kristjanvalur]

A separate PR, #2308, proposes to remove this read lock entirely, and would simplify this PR further.

[dvora-h]

@kristjanvalur Can you merge master in so I can see if tests pass and merge this PR?

[chayim]

@kristjanvalur I think a better title for this may be your comment in the CHANGES file - which completely changes how I think about this PR. IMHO this becomes a feature. For 4.4.0-rc3. WDYT?

[kristjanvalur]

Well, the timeout=None was actually just something I threw in there as a bonus. My primary purpose with this PR was to remove some of the cruft in the async code which exists because it is a direct copy of the non-async code. Timeouts in blocking code need to be handled at the IO level, while in async they are generally free, and implemented via the event loop.

I had actually been thinking about removing this "feature" and adding it separately, post-hoc, to make the PR more "clean".

But whatever you think is best, I'm happy with. I think it is important to straighten out and simplify the async connection code so that we can more easily work on simplifications closer to the metal, for example, remove double buffering, etc.

Would you like me to change the title?

[dvora-h]

@kristjanvalur Same here, conflicts...

[chayim]

Given that we'd like to get this into 4.4.0 rc2... I'm voting for take as is. GitHub really needs a "100%" reaction. I think this is great :)

[dvora-h]

@kristjanvalur last one...

[kristjanvalur]

Ok, this one was a bit trickier, lots of related commits that went in... lets see how it does in CI

[kristjanvalur]

Thanks. That was a bunch of related PRs that all sort of complemented each other. I hope with this in place to be able to do further work, particularly on the hiredis parser, to reduce overhead.