diff options
author | Kristján Valur Jónsson <sweskman@gmail.com> | 2022-09-29 11:03:54 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-09-29 14:03:54 +0300 |
commit | 9fe836698ca5930e39633b86839b6c1bae07237e (patch) | |
tree | e1915d05f852ff45125fb89933bc369001b48744 | |
parent | fbf68dd217b4bf7d91a0e07ca73c8a39dfbc1066 (diff) | |
download | redis-py-9fe836698ca5930e39633b86839b6c1bae07237e.tar.gz |
Catch `Exception` and not `BaseException` in the `Connection` (#2104)
* Add failing unittests for passing BaseException through
* Resolve failing unittest
* Remove redundant checks for asyncio.CancelledError
-rw-r--r-- | redis/asyncio/connection.py | 8 | ||||
-rwxr-xr-x | redis/connection.py | 4 | ||||
-rw-r--r-- | tests/test_asyncio/test_pubsub.py | 74 | ||||
-rw-r--r-- | tests/test_pubsub.py | 42 |
4 files changed, 121 insertions, 7 deletions
diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index 16f33e2..a470b6f 100644 --- a/redis/asyncio/connection.py +++ b/redis/asyncio/connection.py @@ -502,8 +502,6 @@ class HiredisParser(BaseParser): # data was read from the socket and added to the buffer. # return True to indicate that data was read. return True - except asyncio.CancelledError: - raise except (socket.timeout, asyncio.TimeoutError): if raise_on_timeout: raise TimeoutError("Timeout reading from socket") from None @@ -721,7 +719,7 @@ class Connection: lambda: self._connect(), lambda error: self.disconnect() ) except asyncio.CancelledError: - raise + raise # in 3.7 and earlier, this is an Exception, not BaseException except (socket.timeout, asyncio.TimeoutError): raise TimeoutError("Timeout connecting to server") except OSError as e: @@ -916,7 +914,7 @@ class Connection: raise ConnectionError( f"Error {err_no} while writing to socket. {errmsg}." ) from e - except BaseException: + except Exception: await self.disconnect() raise @@ -958,7 +956,7 @@ class Connection: raise ConnectionError( f"Error while reading from {self.host}:{self.port} : {e.args}" ) - except BaseException: + except Exception: await self.disconnect() raise diff --git a/redis/connection.py b/redis/connection.py index 491df8e..2e33e31 100755 --- a/redis/connection.py +++ b/redis/connection.py @@ -766,7 +766,7 @@ class Connection: errno = e.args[0] errmsg = e.args[1] raise ConnectionError(f"Error {errno} while writing to socket. {errmsg}.") - except BaseException: + except Exception: self.disconnect() raise @@ -804,7 +804,7 @@ class Connection: except OSError as e: self.disconnect() raise ConnectionError(f"Error while reading from {hosterr}" f" : {e.args}") - except BaseException: + except Exception: self.disconnect() raise diff --git a/tests/test_asyncio/test_pubsub.py b/tests/test_asyncio/test_pubsub.py index 2ff0c72..555cfdb 100644 --- a/tests/test_asyncio/test_pubsub.py +++ b/tests/test_asyncio/test_pubsub.py @@ -1,7 +1,9 @@ import asyncio import functools import socket +import sys from typing import Optional +from unittest.mock import patch import async_timeout import pytest @@ -914,3 +916,75 @@ class TestPubSubAutoReconnect: return True except asyncio.TimeoutError: return False + + +@pytest.mark.onlynoncluster +class TestBaseException: + @pytest.mark.skipif( + sys.version_info < (3, 8), reason="requires python 3.8 or higher" + ) + async def test_outer_timeout(self, r: redis.Redis): + """ + Using asyncio_timeout manually outside the inner method timeouts works. + This works on Python versions 3.8 and greater, at which time asyncio. + CancelledError became a BaseException instead of an Exception before. + """ + pubsub = r.pubsub() + await pubsub.subscribe("foo") + assert pubsub.connection.is_connected + + async def get_msg_or_timeout(timeout=0.1): + async with async_timeout.timeout(timeout): + # blocking method to return messages + while True: + response = await pubsub.parse_response(block=True) + message = await pubsub.handle_message( + response, ignore_subscribe_messages=False + ) + if message is not None: + return message + + # get subscribe message + msg = await get_msg_or_timeout(10) + assert msg is not None + # timeout waiting for another message which never arrives + assert pubsub.connection.is_connected + with pytest.raises(asyncio.TimeoutError): + await get_msg_or_timeout() + # the timeout on the read should not cause disconnect + assert pubsub.connection.is_connected + + async def test_base_exception(self, r: redis.Redis): + """ + Manually trigger a BaseException inside the parser's .read_response method + and verify that it isn't caught + """ + pubsub = r.pubsub() + await pubsub.subscribe("foo") + assert pubsub.connection.is_connected + + async def get_msg(): + # blocking method to return messages + while True: + response = await pubsub.parse_response(block=True) + message = await pubsub.handle_message( + response, ignore_subscribe_messages=False + ) + if message is not None: + return message + + # get subscribe message + msg = await get_msg() + assert msg is not None + # timeout waiting for another message which never arrives + assert pubsub.connection.is_connected + with patch("redis.asyncio.connection.PythonParser.read_response") as mock1: + mock1.side_effect = BaseException("boom") + with patch("redis.asyncio.connection.HiredisParser.read_response") as mock2: + mock2.side_effect = BaseException("boom") + + with pytest.raises(BaseException): + await get_msg() + + # the timeout on the read should not cause disconnect + assert pubsub.connection.is_connected diff --git a/tests/test_pubsub.py b/tests/test_pubsub.py index 26df598..5d86934 100644 --- a/tests/test_pubsub.py +++ b/tests/test_pubsub.py @@ -735,3 +735,45 @@ class TestPubSubAutoReconnect: for message in self.pubsub.listen(): self.messages.put(message) return True + + +@pytest.mark.onlynoncluster +class TestBaseException: + def test_base_exception(self, r: redis.Redis): + """ + Manually trigger a BaseException inside the parser's .read_response method + and verify that it isn't caught + """ + pubsub = r.pubsub() + pubsub.subscribe("foo") + + def is_connected(): + return pubsub.connection._sock is not None + + assert is_connected() + + def get_msg(): + # blocking method to return messages + while True: + response = pubsub.parse_response(block=True) + message = pubsub.handle_message( + response, ignore_subscribe_messages=False + ) + if message is not None: + return message + + # get subscribe message + msg = get_msg() + assert msg is not None + # timeout waiting for another message which never arrives + assert is_connected() + with patch("redis.connection.PythonParser.read_response") as mock1: + mock1.side_effect = BaseException("boom") + with patch("redis.connection.HiredisParser.read_response") as mock2: + mock2.side_effect = BaseException("boom") + + with pytest.raises(BaseException): + get_msg() + + # the timeout on the read should not cause disconnect + assert is_connected() |