fix(sentinel): disconnect on read errors by default

redis/sentinel.py

@@ -70,7 +70,7 @@ def read_response(
         disable_decoding=False,
         *,
         timeout: Union[float, object] = SENTINEL,
-        disconnect_on_error: Optional[bool] = False,
+        disconnect_on_error: Optional[bool] = True,
         push_request: Optional[bool] = False,
     ):
         try:

tests/test_asyncio/test_sentinel_managed_connection.py

@@ -36,3 +36,29 @@ async def mock_connect():
     assert conn._connect.call_count == 3
     assert connection_pool.get_master_address.call_count == 3
     await conn.disconnect()
+
+
+@pytest.mark.fixed_client
+async def test_read_response_disconnects_on_base_exception(connect_args):
+    """
+    Mirror of the sync test: a BaseException at the socket read leaves the reply
+    unread, so the connection must be closed rather than reused. See #1128.
+    """
+    connection_pool = mock.AsyncMock()
+    connection_pool.get_master_address = mock.AsyncMock(
+        return_value=(connect_args["host"], connect_args["port"])
+    )
+    connection_pool.is_master = True
+    connection_pool.check_connection = False
+    conn = SentinelManagedConnection(connection_pool=connection_pool)
+    await conn.connect()
+    try:
+        await conn.send_command("PING")
+        with mock.patch.object(
+            conn, "_read_response_from_parser", side_effect=KeyboardInterrupt
+        ):
+            with pytest.raises(KeyboardInterrupt):
+                await conn.read_response()
+        assert conn._reader is None
+    finally:
+        await conn.disconnect()

tests/test_sentinel_managed_connection.py

@@ -1,7 +1,9 @@
+import inspect
 import socket
 
 import pytest
 
+from redis.connection import Connection
 from redis.retry import Retry
 from redis.sentinel import SentinelManagedConnection
 from redis.backoff import NoBackoff
@@ -65,7 +67,7 @@ def test_read_response_accepts_timeout_parameter(self, master_host):
             mock_read_response.assert_called_once_with(
                 disable_decoding=False,
                 timeout=0.5,
-                disconnect_on_error=False,
+                disconnect_on_error=True,
                 push_request=False,
             )
 
@@ -90,7 +92,7 @@ def test_read_response_timeout_default_is_sentinel(self, master_host):
             mock_read_response.assert_called_once_with(
                 disable_decoding=False,
                 timeout=SENTINEL,
-                disconnect_on_error=False,
+                disconnect_on_error=True,
                 push_request=False,
             )
 
@@ -115,7 +117,7 @@ def test_read_response_timeout_none_passed_through(self, master_host):
             mock_read_response.assert_called_once_with(
                 disable_decoding=False,
                 timeout=None,
-                disconnect_on_error=False,
+                disconnect_on_error=True,
                 push_request=False,
             )
 
@@ -140,7 +142,7 @@ def test_read_response_timeout_zero_passed_through(self, master_host):
             mock_read_response.assert_called_once_with(
                 disable_decoding=False,
                 timeout=0,
-                disconnect_on_error=False,
+                disconnect_on_error=True,
                 push_request=False,
             )
 
@@ -173,3 +175,97 @@ def test_read_response_all_parameters_passed_through(self, master_host):
                 disconnect_on_error=True,
                 push_request=True,
             )
+
+
+@pytest.mark.fixed_client
+class TestSentinelManagedConnectionDisconnectOnError:
+    """
+    Tests for the disconnect-on-error behaviour of
+    SentinelManagedConnection.read_response().
+
+    These assert on the socket state and on the next reply rather than on the
+    arguments forwarded to the base class, so they observe what the flag
+    actually does.
+    """
+
+    def _connect(self, master_host):
+        connection_pool = mock.Mock()
+        connection_pool.get_master_address = mock.Mock(
+            return_value=(master_host[0], master_host[1])
+        )
+        connection_pool.is_master = True
+        connection_pool.check_connection = False
+        conn = SentinelManagedConnection(connection_pool=connection_pool)
+        conn.connect()
+        return conn
+
+    def test_default_disconnect_on_error_matches_base_connection(self):
+        """
+        The default is inherited behaviour, so it must equal the base class
+        default rather than a value of its own.
+        """
+        sentinel_default = (
+            inspect.signature(SentinelManagedConnection.read_response)
+            .parameters["disconnect_on_error"]
+            .default
+        )
+        base_default = (
+            inspect.signature(Connection.read_response)
+            .parameters["disconnect_on_error"]
+            .default
+        )
+        assert base_default is True
+        assert sentinel_default is True
+
+    def test_base_exception_during_read_disconnects(self, master_host):
+        """
+        A BaseException raised at the socket read leaves the reply unread, so
+        the connection must be closed instead of being reused. See #1128.
+        """
+        conn = self._connect(master_host)
+        try:
+            conn.send_command("PING")
+            with mock.patch.object(
+                conn._parser, "read_response", side_effect=KeyboardInterrupt
+            ):
+                with pytest.raises(KeyboardInterrupt):
+                    conn.read_response()
+            assert conn._sock is None
+        finally:
+            conn.disconnect()
+
+    def test_reply_after_interrupted_read_is_not_stale(self, master_host):
+        """
+        The next command on a reused connection must get its own reply, not the
+        reply left in the socket buffer by the interrupted one.
+        """
+        conn = self._connect(master_host)
+        try:
+            conn.send_command("ECHO", "interrupted")
+            with mock.patch.object(
+                conn._parser, "read_response", side_effect=KeyboardInterrupt
+            ):
+                with pytest.raises(KeyboardInterrupt):
+                    conn.read_response()
+
+            conn.send_command("PING")
+            assert conn.read_response() == b"PONG"
+        finally:
+            conn.disconnect()
+
+    def test_explicit_disconnect_on_error_false_is_honoured(self, master_host):
+        """
+        PubSub reads pass disconnect_on_error=False on purpose; an explicit
+        False must still keep the connection open.
+        """
+        conn = self._connect(master_host)
+        try:
+            conn.send_command("PING")
+            with mock.patch.object(
+                conn._parser, "read_response", side_effect=KeyboardInterrupt
+            ):
+                with pytest.raises(KeyboardInterrupt):
+                    conn.read_response(disconnect_on_error=False)
+            assert conn._sock is not None
+        finally:
+            conn.disconnect()