automatically reconnect pubsub when reading messages in blocking mode

[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 #2089 demonstrates how a pub-sub connection doesn't automatically reconnect. Further investigation showed that this was due to the fact that the connection only performed a just-in-time connect in the can_read() method, which is only invoked if the operation to read a message has a timeout. In particular, using listen() to get the message, instead of the get_message() with a timeout, the can_read() method isn't called, and so a connect() call is not made.

This issue is present in both non-async and async versions of the library as demonstrated with the unit-test code.

The PR fixes this by adding a just-in-time connect to the read_response call of both the Connection object.

[codecov-commenter]

Codecov Report

Merging #2281 (646df07) into master (99b4df4) will increase coverage by 0.05%.
The diff coverage is 95.83%.

❗ Current head 646df07 differs from pull request most recent head 049919b. Consider uploading reports for the commit 049919b to get more accurate results

@@            Coverage Diff             @@
##           master    #2281      +/-   ##
==========================================
+ Coverage   91.91%   91.97%   +0.05%     
==========================================
  Files         109      109              
  Lines       27991    28171     +180     
==========================================
+ Hits        25729    25911     +182     
+ Misses       2262     2260       -2     

Impacted Files	Coverage Δ
tests/conftest.py	84.94% <58.33%> (-1.57%)	⬇️
tests/test_asyncio/test_pubsub.py	99.13% <97.50%> (-0.27%)	⬇️
tests/test_pubsub.py	99.59% <98.75%> (-0.17%)	⬇️
redis/asyncio/client.py	92.78% <100.00%> (+0.04%)	⬆️
redis/client.py	89.06% <100.00%> (+0.48%)	⬆️
tests/test_asyncio/compat.py	100.00% <100.00%> (ø)
tests/test_asyncio/test_cluster.py	97.44% <0.00%> (-0.14%)	⬇️
tests/test_cluster.py	96.85% <0.00%> (ø)
redis/asyncio/connection.py	84.58% <0.00%> (+0.44%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99b4df4...049919b. Read the comment docs.

[utkarshgupta137]

Shouldn't this be added to the pub/sub code instead of adding this check for every redis request?

[kristjanvalur]

Possibly. read_response() is after all used in other places than the pub-sub. On the other hand, can_read() has the connect internally, which is the reason it works for non-blocking cases.`
I'll see if I can do this differently.

[kristjanvalur]

Thanks for the suggestion. Now the can_read/read_response interaction, along with an extra connect(), is wrapped in a closure passed to PubSub._execute()

[dvora-h]

@kristjanvalur Thanks, great PR!

[dvora-h]

@kristjanvalur Can you resolve the conflicts?

[agronholm]

Why is the async reconnection method different from the sync one? Is either one documented anywhere?

[kristjanvalur]

Not sure what you mean. Both code paths now contain an extra conn.connect() in the PubSub read method.