Catch Exception and not BaseException in the Connection

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

Description of change

Fixes Issue #2103:
Raising a BaseException inside a Connection's send or receive oprations causes a disconnect(). BaseExceptions are commonly used for out of band messages in Python and normally not be expecially acted upon, only be handled at the top-level

See also: #2356 which focuses on a different aspcect of error handling.

[codecov-commenter]

Codecov Report

Merging #2104 (e482225) into master (b5ebada) will decrease coverage by 0.00%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master    #2104      +/-   ##
==========================================
- Coverage   92.21%   92.20%   -0.01%     
==========================================
  Files         111      111              
  Lines       28781    28853      +72     
==========================================
+ Hits        26539    26605      +66     
- Misses       2242     2248       +6     

Impacted Files	Coverage Δ
redis/asyncio/connection.py	85.84% <50.00%> (-0.28%)	⬇️
redis/connection.py	86.90% <50.00%> (ø)
tests/test_asyncio/test_pubsub.py	99.52% <100.00%> (+0.20%)	⬆️
tests/test_pubsub.py	99.61% <100.00%> (+0.01%)	⬆️
tests/test_timeseries.py	100.00% <100.00%> (ø)
tests/test_cluster.py	96.85% <0.00%> (-0.24%)	⬇️

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

[WisdomPill]

LGTM!

Can you add some tests for it?

[chayim]

@kristjanvalur Why did you catch Exception rather that BaseException? Was there an inheritance issue you saw?

[WisdomPill]

@chayim have a look here #2103

[kristjanvalur]

I can add tests. As I explained in the defect, BaseException is usually not caught except in top level handlers, unless it is to implement something like async_timeout.Timeout() . BaseException was added to Python precisely to be a sort of meta exception that regular exception handling doesn't touch, the Exception being the most general class usually touched in code that isn't trying to be too clever.

[kristjanvalur]

In fact, I don't see why there is a except Exception: self.disconnect(); raise at all. Doesn't seem like something the Connection should do if it gets an unexpected exception. IMHO, this clause should be removed alltogether.

[kristjanvalur]

(There isn't an equivalent non-async test that can be easily done, without writing a thread-based timeout class.)

[kristjanvalur]

Hm, now it is mysteriously failing. it asserts on the "assert pubsub.connection.is_connected"
I´m running tests both on windows and in a linux dev container using
pytest tests/test_async/test_pubsub.py. I don't get the test failures above.

[kristjanvalur]

Well, it seems I cracked the unittest problem I was having. Turned out that only in Python 3.8 did asyncio.CancelledError get promoted to BaseException.
Fixed up the test to skip older python versions and added Mock-based tests for exception propagation for both async and blocking redis.

[kristjanvalur]

any responses? @WisdomPill ?

[kristjanvalur]

I'll clean up the commit history a bit...

[kristjanvalur]

yet another fluke test failure.

[akx]

I bumped into this too – trying to e.g. ctrl+c out of py.test requires a couple of tries because KeyboardInterrupt (a BaseException) is caught where it really must not be.

[kristjanvalur]

woo-hoo!

[dvora-h]

Thanks for this! And sorry this (and your other PR's) waited so long...