Repository: redis/redis-py
Pull request #2271 (2022-07-12 11:21:15)

Use retry mechanism in async version of Connection objects
### 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)?
- [x] 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

The retry mechanism was not used in an async version of Connection(and SentinelManagedConnection). I've added the use of 'call_with_retry' - the same as in the sync version. 
See related issue: https://github.com/redis/redis-py/issues/2211 


codecov-commenter: # [Codecov](https://codecov.io/gh/redis/redis-py/pull/2271?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) Report
> Merging [#2271](https://codecov.io/gh/redis/redis-py/pull/2271?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) (84e00ee) into [master](https://codecov.io/gh/redis/redis-py/commit/4b0543d567aef36ac467ce495d831a24575d8d5b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) (4b0543d) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head 84e00ee differs from pull request most recent head 80a64e8. Consider uploading reports for the commit 80a64e8 to get more accurate results

```diff
@@           Coverage Diff           @@
##           master    #2271   +/-   ##
=======================================
  Coverage   91.87%   91.88%           
=======================================
  Files         108      109    +1     
  Lines       27783    27837   +54     
=======================================
+ Hits        25527    25578   +51     
- Misses       2256     2259    +3     
```


| [Impacted Files](https://codecov.io/gh/redis/redis-py/pull/2271?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) | Coverage Δ | |
|---|---|---|
| [redis/asyncio/connection.py](https://codecov.io/gh/redis/redis-py/pull/2271/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvYXN5bmNpby9jb25uZWN0aW9uLnB5) | `84.21% <100.00%> (+0.14%)` | :arrow_up: |
| [redis/asyncio/sentinel.py](https://codecov.io/gh/redis/redis-py/pull/2271/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvYXN5bmNpby9zZW50aW5lbC5weQ==) | `87.57% <100.00%> (+0.15%)` | :arrow_up: |
| [tests/test\_asyncio/test\_connection.py](https://codecov.io/gh/redis/redis-py/pull/2271/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9hc3luY2lvL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `98.52% <100.00%> (+1.16%)` | :arrow_up: |
| [...s/test\_asyncio/test\_sentinel\_managed\_connection.py](https://codecov.io/gh/redis/redis-py/pull/2271/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9hc3luY2lvL3Rlc3Rfc2VudGluZWxfbWFuYWdlZF9jb25uZWN0aW9uLnB5) | `100.00% <100.00%> (ø)` | |
| [tests/conftest.py](https://codecov.io/gh/redis/redis-py/pull/2271/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvY29uZnRlc3QucHk=) | `86.50% <0.00%> (-0.80%)` | :arrow_down: |
| [tests/test\_asyncio/test\_cluster.py](https://codecov.io/gh/redis/redis-py/pull/2271/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9hc3luY2lvL3Rlc3RfY2x1c3Rlci5weQ==) | `97.44% <0.00%> (-0.14%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redis/redis-py/pull/2271?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/redis/redis-py/pull/2271?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis). Last update [4b0543d...80a64e8](https://codecov.io/gh/redis/redis-py/pull/2271?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).

szumka: @dvora-h is it possible to rerun CI checks? "CI / Validate building and installing the package (who)" hung for 6hrs and was canceled. I don't think the issue is related to this PR. 

Patch:
diff --git a/CHANGES b/CHANGES
index 0af421b4..b4841ddb 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,4 +1,5 @@
 
+    * Add retry mechanism to async version of Connection
     * Compare commands case-insensitively in the asyncio command parser
     * Allow negative `retries` for `Retry` class to retry forever
     * Add `items` parameter to `hset` signature
diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py
index 35536fc8..5bf2a0fd 100644
--- a/redis/asyncio/connection.py
+++ b/redis/asyncio/connection.py
@@ -637,6 +637,8 @@ class Connection:
             retry_on_error = []
         if retry_on_timeout:
             retry_on_error.append(TimeoutError)
+            retry_on_error.append(socket.timeout)
+            retry_on_error.append(asyncio.TimeoutError)
         self.retry_on_error = retry_on_error
         if retry_on_error:
             if not retry:
@@ -706,7 +708,9 @@ class Connection:
         if self.is_connected:
             return
         try:
-            await self._connect()
+            await self.retry.call_with_retry(
+                lambda: self._connect(), lambda error: self.disconnect()
+            )
         except asyncio.CancelledError:
             raise
         except (socket.timeout, asyncio.TimeoutError):
diff --git a/redis/asyncio/sentinel.py b/redis/asyncio/sentinel.py
index 5aefd09e..99c50749 100644
--- a/redis/asyncio/sentinel.py
+++ b/redis/asyncio/sentinel.py
@@ -44,7 +44,7 @@ class SentinelManagedConnection(Connection):
             if str_if_bytes(await self.read_response()) != "PONG":
                 raise ConnectionError("PING failed")
 
-    async def connect(self):
+    async def _connect_retry(self):
         if self._reader:
             return  # already connected
         if self.connection_pool.is_master:
@@ -57,6 +57,12 @@ class SentinelManagedConnection(Connection):
                     continue
             raise SlaveNotFoundError  # Never be here
 
+    async def connect(self):
+        return await self.retry.call_with_retry(
+            self._connect_retry,
+            lambda error: asyncio.sleep(0),
+        )
+
     async def read_response(self, disable_decoding: bool = False):
         try:
             return await super().read_response(disable_decoding=disable_decoding)


Test patch:
diff --git a/tests/test_asyncio/test_connection.py b/tests/test_asyncio/test_connection.py
index f6259adb..78a3efd2 100644
--- a/tests/test_asyncio/test_connection.py
+++ b/tests/test_asyncio/test_connection.py
@@ -1,10 +1,18 @@
 import asyncio
+import socket
 import types
+from unittest.mock import patch
 
 import pytest
 
-from redis.asyncio.connection import PythonParser, UnixDomainSocketConnection
-from redis.exceptions import InvalidResponse
+from redis.asyncio.connection import (
+    Connection,
+    PythonParser,
+    UnixDomainSocketConnection,
+)
+from redis.asyncio.retry import Retry
+from redis.backoff import NoBackoff
+from redis.exceptions import ConnectionError, InvalidResponse, TimeoutError
 from redis.utils import HIREDIS_AVAILABLE
 from tests.conftest import skip_if_server_version_lt
 
@@ -60,3 +68,44 @@ async def test_socket_param_regression(r):
 async def test_can_run_concurrent_commands(r):
     assert await r.ping() is True
     assert all(await asyncio.gather(*(r.ping() for _ in range(10))))
+
+
+async def test_connect_retry_on_timeout_error():
+    """Test that the _connect function is retried in case of a timeout"""
+    conn = Connection(retry_on_timeout=True, retry=Retry(NoBackoff(), 3))
+    origin_connect = conn._connect
+    conn._connect = mock.AsyncMock()
+
+    async def mock_connect():
+        # connect only on the last retry
+        if conn._connect.call_count <= 2:
+            raise socket.timeout
+        else:
+            return await origin_connect()
+
+    conn._connect.side_effect = mock_connect
+    await conn.connect()
+    assert conn._connect.call_count == 3
+
+
+async def test_connect_without_retry_on_os_error():
+    """Test that the _connect function is not being retried in case of a OSError"""
+    with patch.object(Connection, "_connect") as _connect:
+        _connect.side_effect = OSError("")
+        conn = Connection(retry_on_timeout=True, retry=Retry(NoBackoff(), 2))
+        with pytest.raises(ConnectionError):
+            await conn.connect()
+        assert _connect.call_count == 1
+
+
+async def test_connect_timeout_error_without_retry():
+    """Test that the _connect function is not being retried if retry_on_timeout is
+    set to False"""
+    conn = Connection(retry_on_timeout=False)
+    conn._connect = mock.AsyncMock()
+    conn._connect.side_effect = socket.timeout
+
+    with pytest.raises(TimeoutError) as e:
+        await conn.connect()
+    assert conn._connect.call_count == 1
+    assert str(e.value) == "Timeout connecting to server"
diff --git a/tests/test_asyncio/test_sentinel_managed_connection.py b/tests/test_asyncio/test_sentinel_managed_connection.py
new file mode 100644
index 00000000..a6e9f37a
--- /dev/null
+++ b/tests/test_asyncio/test_sentinel_managed_connection.py
@@ -0,0 +1,37 @@
+import socket
+
+import pytest
+
+from redis.asyncio.retry import Retry
+from redis.asyncio.sentinel import SentinelManagedConnection
+from redis.backoff import NoBackoff
+
+from .compat import mock
+
+pytestmark = pytest.mark.asyncio
+
+
+async def test_connect_retry_on_timeout_error():
+    """Test that the _connect function is retried in case of a timeout"""
+    connection_pool = mock.AsyncMock()
+    connection_pool.get_master_address = mock.AsyncMock(
+        return_value=("localhost", 6379)
+    )
+    conn = SentinelManagedConnection(
+        retry_on_timeout=True,
+        retry=Retry(NoBackoff(), 3),
+        connection_pool=connection_pool,
+    )
+    origin_connect = conn._connect
+    conn._connect = mock.AsyncMock()
+
+    async def mock_connect():
+        # connect only on the last retry
+        if conn._connect.call_count <= 2:
+            raise socket.timeout
+        else:
+            return await origin_connect()
+
+    conn._connect.side_effect = mock_connect
+    await conn.connect()
+    assert conn._connect.call_count == 3