Repository: redis/redis-py (python)

Issue:
Question: resolving hostname when retry on connection timeout
I wonder if Redis client resolve again the hostname if connection timeout occurred while setting `retry_on_timeout=True`.
I'm asking since sometime the mapping between hostname to ip can be invalid since the DNS changed the mapping.

Pull request:
 Added retry mechanism on socket timeouts when connecting to the server
### 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)?
- [ ] Is there an example added to the examples folder (if applicable)?

_NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open._

### Description of change

The current option to add retry object for timeouts works only at the execute command level and do not apply to the socket timeouts that may occur when trying to connect to the server.
Added support for retrying on timeout to connection::connect function.

Closes #1833 


Patch:
diff --git a/README.md b/README.md
index 166e80c2..867f921d 100644
--- a/README.md
+++ b/README.md
@@ -968,25 +968,25 @@ instance can be created:
 - Using 'host' and 'port' arguments:
 
 ``` pycon
-    >>> from redis.cluster import RedisCluster as Redis
-    >>> rc = Redis(host='localhost', port=6379)
-    >>> print(rc.get_nodes())
+>>> from redis.cluster import RedisCluster as Redis
+>>> rc = Redis(host='localhost', port=6379)
+>>> print(rc.get_nodes())
     [[host=127.0.0.1,port=6379,name=127.0.0.1:6379,server_type=primary,redis_connection=Redis<ConnectionPool<Connection<host=127.0.0.1,port=6379,db=0>>>], [host=127.0.0.1,port=6378,name=127.0.0.1:6378,server_type=primary,redis_connection=Redis<ConnectionPool<Connection<host=127.0.0.1,port=6378,db=0>>>], [host=127.0.0.1,port=6377,name=127.0.0.1:6377,server_type=replica,redis_connection=Redis<ConnectionPool<Connection<host=127.0.0.1,port=6377,db=0>>>]]
 ```
 - Using the Redis URL specification:
 
 ``` pycon
-    >>> from redis.cluster import RedisCluster as Redis
-    >>> rc = Redis.from_url("redis://localhost:6379/0")
+>>> from redis.cluster import RedisCluster as Redis
+>>> rc = Redis.from_url("redis://localhost:6379/0")
 ```
 
 - Directly, via the ClusterNode class:
 
 ``` pycon
-    >>> from redis.cluster import RedisCluster as Redis
-    >>> from redis.cluster import ClusterNode
-    >>> nodes = [ClusterNode('localhost', 6379), ClusterNode('localhost', 6378)]
-    >>> rc = Redis(startup_nodes=nodes)
+>>> from redis.cluster import RedisCluster as Redis
+>>> from redis.cluster import ClusterNode
+>>> nodes = [ClusterNode('localhost', 6379), ClusterNode('localhost', 6378)]
+>>> rc = Redis(startup_nodes=nodes)
 ```
 
 When a RedisCluster instance is being created it first attempts to establish a
@@ -1016,18 +1016,18 @@ The 'target_nodes' parameter is explained in the following section,
 'Specifying Target Nodes'.
 
 ``` pycon
-    >>> # target-nodes: the node that holds 'foo1's key slot
-    >>> rc.set('foo1', 'bar1')
-    >>> # target-nodes: the node that holds 'foo2's key slot
-    >>> rc.set('foo2', 'bar2')
-    >>> # target-nodes: the node that holds 'foo1's key slot
-    >>> print(rc.get('foo1'))
-    b'bar'
-    >>> # target-node: default-node
-    >>> print(rc.keys())
-    [b'foo1']
-    >>> # target-node: default-node
-    >>> rc.ping()
+>>> # target-nodes: the node that holds 'foo1's key slot
+>>> rc.set('foo1', 'bar1')
+>>> # target-nodes: the node that holds 'foo2's key slot
+>>> rc.set('foo2', 'bar2')
+>>> # target-nodes: the node that holds 'foo1's key slot
+>>> print(rc.get('foo1'))
+b'bar'
+>>> # target-node: default-node
+>>> print(rc.keys())
+[b'foo1']
+>>> # target-node: default-node
+>>> rc.ping()
 ```
 
 **Specifying Target Nodes:**
@@ -1043,18 +1043,18 @@ the client will be able to resolve the nodes flag again with the new topology
 and attempt to retry executing the command.
 
 ``` pycon
-    >>> from redis.cluster import RedisCluster as Redis
-    >>> # run cluster-meet command on all of the cluster's nodes
-    >>> rc.cluster_meet('127.0.0.1', 6379, target_nodes=Redis.ALL_NODES)
-    >>> # ping all replicas
-    >>> rc.ping(target_nodes=Redis.REPLICAS)
-    >>> # ping a random node
-    >>> rc.ping(target_nodes=Redis.RANDOM)
-    >>> # get the keys from all cluster nodes
-    >>> rc.keys(target_nodes=Redis.ALL_NODES)
-    [b'foo1', b'foo2']
-    >>> # execute bgsave in all primaries
-    >>> rc.bgsave(Redis.PRIMARIES)
+>>> from redis.cluster import RedisCluster as Redis
+>>> # run cluster-meet command on all of the cluster's nodes
+>>> rc.cluster_meet('127.0.0.1', 6379, target_nodes=Redis.ALL_NODES)
+>>> # ping all replicas
+>>> rc.ping(target_nodes=Redis.REPLICAS)
+>>> # ping a random node
+>>> rc.ping(target_nodes=Redis.RANDOM)
+>>> # get the keys from all cluster nodes
+>>> rc.keys(target_nodes=Redis.ALL_NODES)
+[b'foo1', b'foo2']
+>>> # execute bgsave in all primaries
+>>> rc.bgsave(Redis.PRIMARIES)
 ```
 
 You could also pass ClusterNodes directly if you want to execute a command on a
@@ -1064,12 +1064,12 @@ will not be made, since the passed target node/s may no longer be valid, and
 the relevant cluster or connection error will be returned.
 
 ``` pycon
-    >>> node = rc.get_node('localhost', 6379)
-    >>> # Get the keys only for that specific node
-    >>> rc.keys(target_nodes=node)
-    >>> # get Redis info from a subset of primaries
-    >>> subset_primaries = [node for node in rc.get_primaries() if node.port > 6378]
-    >>> rc.info(target_nodes=subset_primaries)
+>>> node = rc.get_node('localhost', 6379)
+>>> # Get the keys only for that specific node
+>>> rc.keys(target_nodes=node)
+>>> # get Redis info from a subset of primaries
+>>> subset_primaries = [node for node in rc.get_primaries() if node.port > 6378]
+>>> rc.info(target_nodes=subset_primaries)
 ```
 
 In addition, the RedisCluster instance can query the Redis instance of a
@@ -1077,15 +1077,15 @@ specific node and execute commands on that node directly. The Redis client,
 however, does not handle cluster failures and retries.
 
 ``` pycon
-    >>> cluster_node = rc.get_node(host='localhost', port=6379)
-    >>> print(cluster_node)
-    [host=127.0.0.1,port=6379,name=127.0.0.1:6379,server_type=primary,redis_connection=Redis<ConnectionPool<Connection<host=127.0.0.1,port=6379,db=0>>>]
-    >>> r = cluster_node.redis_connection
-    >>> r.client_list()
-    [{'id': '276', 'addr': '127.0.0.1:64108', 'fd': '16', 'name': '', 'age': '0', 'idle': '0', 'flags': 'N', 'db': '0', 'sub': '0', 'psub': '0', 'multi': '-1', 'qbuf': '26', 'qbuf-free': '32742', 'argv-mem': '10', 'obl': '0', 'oll': '0', 'omem': '0', 'tot-mem': '54298', 'events': 'r', 'cmd': 'client', 'user': 'default'}]
-    >>> # Get the keys only for that specific node
-    >>> r.keys()
-    [b'foo1']
+>>> cluster_node = rc.get_node(host='localhost', port=6379)
+>>> print(cluster_node)
+[host=127.0.0.1,port=6379,name=127.0.0.1:6379,server_type=primary,redis_connection=Redis<ConnectionPool<Connection<host=127.0.0.1,port=6379,db=0>>>]
+>>> r = cluster_node.redis_connection
+>>> r.client_list()
+[{'id': '276', 'addr': '127.0.0.1:64108', 'fd': '16', 'name': '', 'age': '0', 'idle': '0', 'flags': 'N', 'db': '0', 'sub': '0', 'psub': '0', 'multi': '-1', 'qbuf': '26', 'qbuf-free': '32742', 'argv-mem': '10', 'obl': '0', 'oll': '0', 'omem': '0', 'tot-mem': '54298', 'events': 'r', 'cmd': 'client', 'user': 'default'}]
+>>> # Get the keys only for that specific node
+>>> r.keys()
+[b'foo1']
 ```
 
 **Multi-key commands:**
@@ -1105,14 +1105,14 @@ operations batch the keys according to their hash value, and then each batch is
 sent separately to the slot's owner.
 
 ``` pycon
-    #  Atomic operations can be used when all keys are mapped to the same slot
-    >>> rc.mset({'{foo}1': 'bar1', '{foo}2': 'bar2'})
-    >>> rc.mget('{foo}1', '{foo}2')
-    [b'bar1', b'bar2']
-    # Non-atomic multi-key operations splits the keys into different slots
-    >>> rc.mset_nonatomic({'foo': 'value1', 'bar': 'value2', 'zzz': 'value3')
-    >>> rc.mget_nonatomic('foo', 'bar', 'zzz')
-    [b'value1', b'value2', b'value3']
+# Atomic operations can be used when all keys are mapped to the same slot
+>>> rc.mset({'{foo}1': 'bar1', '{foo}2': 'bar2'})
+>>> rc.mget('{foo}1', '{foo}2')
+[b'bar1', b'bar2']
+# Non-atomic multi-key operations splits the keys into different slots
+>>> rc.mset_nonatomic({'foo': 'value1', 'bar': 'value2', 'zzz': 'value3')
+>>> rc.mget_nonatomic('foo', 'bar', 'zzz')
+[b'value1', b'value2', b'value3']
 ```
 
 **Cluster PubSub:**
@@ -1135,11 +1135,11 @@ See [redis-py-cluster documentation](https://redis-py-cluster.readthedocs.io/en/
  for more.
 
 ``` pycon
-    >>> p1 = rc.pubsub()
-    # p1 connection will be set to the node that holds 'foo' keyslot
-    >>> p1.subscribe('foo')
-    # p2 connection will be set to node 'localhost:6379'
-    >>> p2 = rc.pubsub(rc.get_node('localhost', 6379))
+>>> p1 = rc.pubsub()
+# p1 connection will be set to the node that holds 'foo' keyslot
+>>> p1.subscribe('foo')
+# p2 connection will be set to node 'localhost:6379'
+>>> p2 = rc.pubsub(rc.get_node('localhost', 6379))
 ```
 
 **Read Only Mode**
@@ -1157,20 +1157,20 @@ target_nodes='replicas', and read-write access can be restored by calling the
 readwrite() method.
 
 ``` pycon
-    >>> from cluster import RedisCluster as Redis
-    # Use 'debug' log level to print the node that the command is executed on
-    >>> rc_readonly = Redis(startup_nodes=startup_nodes,
-                    read_from_replicas=True)
-    >>> rc_readonly.set('{foo}1', 'bar1')
-    >>> for i in range(0, 4):
-            # Assigns read command to the slot's hosts in a Round-Robin manner
-    >>>     rc_readonly.get('{foo}1')
-    # set command would be directed only to the slot's primary node
-    >>> rc_readonly.set('{foo}2', 'bar2')
-    # reset READONLY flag
-    >>> rc_readonly.readwrite(target_nodes='replicas')
-    # now the get command would be directed only to the slot's primary node
-    >>> rc_readonly.get('{foo}1')
+>>> from cluster import RedisCluster as Redis
+# Use 'debug' log level to print the node that the command is executed on
+>>> rc_readonly = Redis(startup_nodes=startup_nodes,
+...                     read_from_replicas=True)
+>>> rc_readonly.set('{foo}1', 'bar1')
+>>> for i in range(0, 4):
+...     # Assigns read command to the slot's hosts in a Round-Robin manner
+...     rc_readonly.get('{foo}1')
+# set command would be directed only to the slot's primary node
+>>> rc_readonly.set('{foo}2', 'bar2')
+# reset READONLY flag
+>>> rc_readonly.readwrite(target_nodes='replicas')
+# now the get command would be directed only to the slot's primary node
+>>> rc_readonly.get('{foo}1')
 ```
 
 **Cluster Pipeline**
@@ -1187,16 +1187,17 @@ by significantly reducing the the number of network round trips between the
 client and the server.
 
 ``` pycon
-    >>> with rc.pipeline() as pipe:
-    >>>     pipe.set('foo', 'value1')
-    >>>     pipe.set('bar', 'value2')
-    >>>     pipe.get('foo')
-    >>>     pipe.get('bar')
-    >>>     print(pipe.execute())
-    [True, True, b'value1', b'value2']
-    >>>     pipe.set('foo1', 'bar1').get('foo1').execute()
-    [True, b'bar1']
+>>> with rc.pipeline() as pipe:
+...     pipe.set('foo', 'value1')
+...     pipe.set('bar', 'value2')
+...     pipe.get('foo')
+...     pipe.get('bar')
+...     print(pipe.execute())
+[True, True, b'value1', b'value2']
+...     pipe.set('foo1', 'bar1').get('foo1').execute()
+[True, b'bar1']
 ```
+
 Please note:
 - RedisCluster pipelines currently only support key-based commands.
 - The pipeline gets its 'read_from_replicas' value from the cluster's parameter.
diff --git a/docker/base/Dockerfile.cluster b/docker/base/Dockerfile.cluster
index 70df5bab..5d419801 100644
--- a/docker/base/Dockerfile.cluster
+++ b/docker/base/Dockerfile.cluster
@@ -6,4 +6,4 @@ RUN chmod +x /create_cluster.sh
 
 EXPOSE 16379 16380 16381 16382 16383 16384
 
-CMD [ "/create_cluster.sh"]
+CMD /create_cluster.sh  16379  16384
diff --git a/docker/base/Dockerfile.unstable_cluster b/docker/base/Dockerfile.unstable_cluster
new file mode 100644
index 00000000..31e4275d
--- /dev/null
+++ b/docker/base/Dockerfile.unstable_cluster
@@ -0,0 +1,9 @@
+# produces redisfab/redis-py-cluster:6.2.6
+FROM redisfab/redis-py:unstable-bionic
+
+COPY create_cluster.sh /create_cluster.sh
+RUN chmod +x /create_cluster.sh
+
+EXPOSE 6372 6373 6374 6375 6376 6377
+
+CMD /create_cluster.sh  6372 6377
diff --git a/docker/base/create_cluster.sh b/docker/base/create_cluster.sh
index 82a79c80..72332944 100644
--- a/docker/base/create_cluster.sh
+++ b/docker/base/create_cluster.sh
@@ -1,7 +1,9 @@
 #! /bin/bash
 mkdir -p /nodes
 touch /nodes/nodemap
-for PORT in $(seq 16379 16384); do
+START_NODE=$1
+END_NODE=$2
+for PORT in `seq ${START_NODE} ${END_NODE}`; do
   mkdir -p /nodes/$PORT
   if [[ -e /redis.conf ]]; then
     cp /redis.conf /nodes/$PORT/redis.conf
@@ -22,5 +24,5 @@ EOF
   fi
   echo 127.0.0.1:$PORT >> /nodes/nodemap
 done
-echo yes | redis-cli --cluster create $(seq -f 127.0.0.1:%g 16379 16384) --cluster-replicas 1
+echo yes | redis-cli --cluster create $(seq -f 127.0.0.1:%g ${START_NODE} ${END_NODE}) --cluster-replicas 1
 tail -f /redis.log
diff --git a/docker/unstable_cluster/redis.conf b/docker/unstable_cluster/redis.conf
new file mode 100644
index 00000000..f307a637
--- /dev/null
+++ b/docker/unstable_cluster/redis.conf
@@ -0,0 +1,4 @@
+# Redis Cluster config file will be shared across all nodes.
+# Do not change the following configurations that are already set:
+# port, cluster-enabled, daemonize, logfile, dir
+protected-mode no
diff --git a/docs/index.rst b/docs/index.rst
index 51b38a2b..2e42acb8 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -44,7 +44,7 @@ There are two quick ways to connect to Redis.
    r = redis.from_url('redis://foo.bar.com:12345')
    r.ping()
 
-After that, you probably want to `run redis commands <redis_core_commands.html>`_.
+After that, you probably want to `run redis commands <commands.html>`_.
 
 .. toctree::
    :hidden:
diff --git a/redis/connection.py b/redis/connection.py
index 5fdac54c..508c1961 100755
--- a/redis/connection.py
+++ b/redis/connection.py
@@ -604,7 +604,9 @@ class Connection:
         if self._sock:
             return
         try:
-            sock = self._connect()
+            sock = self.retry.call_with_retry(
+                lambda: self._connect(), lambda error: self.disconnect(error)
+            )
         except socket.timeout:
             raise TimeoutError("Timeout connecting to server")
         except OSError as e:
@@ -721,7 +723,7 @@ class Connection:
             if str_if_bytes(self.read_response()) != "OK":
                 raise ConnectionError("Invalid Database")
 
-    def disconnect(self):
+    def disconnect(self, *args):
         "Disconnects from the Redis server"
         self._parser.on_disconnect()
         if self._sock is None:
diff --git a/redis/retry.py b/redis/retry.py
index 6147fbd9..3dced35d 100644
--- a/redis/retry.py
+++ b/redis/retry.py
@@ -1,3 +1,4 @@
+import socket
 from time import sleep
 
 from redis.exceptions import ConnectionError, TimeoutError
@@ -7,7 +8,10 @@ class Retry:
     """Retry a specific number of times after a failure"""
 
     def __init__(
-        self, backoff, retries, supported_errors=(ConnectionError, TimeoutError)
+        self,
+        backoff,
+        retries,
+        supported_errors=(ConnectionError, TimeoutError, socket.timeout),
     ):
         """
         Initialize a `Retry` object with a `Backoff` object
diff --git a/tox.ini b/tox.ini
index abebf004..0d72f916 100644
--- a/tox.ini
+++ b/tox.ini
@@ -41,6 +41,19 @@ healtcheck_cmd = python -c "import socket;print(True) if 0 == socket.socket(sock
 volumes =
     bind:rw:{toxinidir}/docker/unstable/redis.conf:/redis.conf
 
+[docker:unstable_cluster]
+name = unstable_cluster
+image = redisfab/redis-py-cluster:unstable-bionic
+ports =
+    6372:6372/tcp
+    6373:6373/tcp
+    6374:6374/tcp
+    6375:6375/tcp
+    6376:6376/tcp
+    6377:6377/tcp
+healtcheck_cmd = python -c "import socket;print(True) if all([0 == socket.socket(socket.AF_INET, socket.SOCK_STREAM).connect_ex(('127.0.0.1',port)) for port in range(6372,6377)]) else False"
+volumes =
+    bind:rw:{toxinidir}/docker/cluster/redis.conf:/redis.conf
 
 [docker:sentinel_1]
 name = sentinel_1
@@ -118,6 +131,7 @@ deps =
     -r {toxinidir}/dev_requirements.txt
 docker =
     unstable
+    unstable_cluster
     master
     replica
     sentinel_1


Test patch:
diff --git a/tests/test_connection.py b/tests/test_connection.py
index d94a8151..d9251c31 100644
--- a/tests/test_connection.py
+++ b/tests/test_connection.py
@@ -1,10 +1,14 @@
+import socket
 import types
 from unittest import mock
+from unittest.mock import patch
 
 import pytest
 
+from redis.backoff import NoBackoff
 from redis.connection import Connection
-from redis.exceptions import InvalidResponse
+from redis.exceptions import ConnectionError, InvalidResponse, TimeoutError
+from redis.retry import Retry
 from redis.utils import HIREDIS_AVAILABLE
 
 from .conftest import skip_if_server_version_lt
@@ -74,3 +78,47 @@ class TestConnection:
         mock_sock.shutdown.assert_called_once()
         mock_sock.close.assert_called_once()
         assert conn._sock is None
+
+    def clear(self, conn):
+        conn.retry_on_error.clear()
+
+    def test_retry_connect_on_timeout_error(self):
+        """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.Mock()
+
+        def mock_connect():
+            # connect only on the last retry
+            if conn._connect.call_count <= 2:
+                raise socket.timeout
+            else:
+                return origin_connect()
+
+        conn._connect.side_effect = mock_connect
+        conn.connect()
+        assert conn._connect.call_count == 3
+        self.clear(conn)
+
+    def test_connect_without_retry_on_os_error(self):
+        """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):
+                conn.connect()
+            assert _connect.call_count == 1
+            self.clear(conn)
+
+    def test_connect_timeout_error_without_retry(self):
+        """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.Mock()
+        conn._connect.side_effect = socket.timeout
+
+        with pytest.raises(TimeoutError) as e:
+            conn.connect()
+        assert conn._connect.call_count == 1
+        assert str(e.value) == "Timeout connecting to server"
+        self.clear(conn)