Fix corner case with dead server coming back alive (#275)

Looks like clients/hash behaves incorrectly when dead server comes back alive - it's never removed from dead_clients and thus retried indefinitely.

Failing test and fix are visible in 35ee9de, rest of PR simply adds some test coverage and refactors existing code a little.

2 files changed, 58 insertions, 14 deletions

diff --git a/pymemcache/client/hash.py b/pymemcache/client/hash.py
index c2cb23e..fccb060 100644
--- a/pymemcache/client/hash.py
+++ b/pymemcache/client/hash.py
@@ -121,22 +121,28 @@ class HashClient(object):
         key = '%s:%s' % (server, port)
         self.hasher.remove_node(key)
 
+    def _retry_dead(self):
+        current_time = time.time()
+        ldc = self._last_dead_check_time
+        # We have reached the retry timeout
+        if current_time - ldc > self.dead_timeout:
+            candidates = []
+            for server, dead_time in self._dead_clients.items():
+                if current_time - dead_time > self.dead_timeout:
+                    candidates.append(server)
+            for server in candidates:
+                logger.debug(
+                    'bringing server back into rotation %s',
+                    server
+                )
+                self.add_server(*server)
+                del self._dead_clients[server]
+            self._last_dead_check_time = current_time
+
     def _get_client(self, key):
         check_key_helper(key, self.allow_unicode_keys, self.key_prefix)
-        if len(self._dead_clients) > 0:
-            current_time = time.time()
-            ldc = self._last_dead_check_time
-            # we have dead clients and we have reached the
-            # timeout retry
-            if current_time - ldc > self.dead_timeout:
-                for server, dead_time in self._dead_clients.items():
-                    if current_time - dead_time > self.dead_timeout:
-                        logger.debug(
-                            'bringing server back into rotation %s',
-                            server
-                        )
-                        self.add_server(*server)
-                        self._last_dead_check_time = current_time
+        if self._dead_clients:
+            self._retry_dead()
 
         server = self.hasher.get_node(key)
         # We've ran out of servers to try
diff --git a/pymemcache/test/test_client_hash.py b/pymemcache/test/test_client_hash.py
index 311e382..e0edfb7 100644
--- a/pymemcache/test/test_client_hash.py
+++ b/pymemcache/test/test_client_hash.py
@@ -295,4 +295,42 @@ class TestHashClient(ClientTestMixin, unittest.TestCase):
         for client in hash_client.clients.values():
             assert client.encoding == encoding
 
+    @mock.patch("pymemcache.client.hash.Client")
+    def test_dead_server_comes_back(self, client_patch):
+        client = HashClient([], dead_timeout=0, retry_attempts=0)
+        client.add_server("127.0.0.1", 11211)
+
+        test_client = client_patch.return_value
+        test_client.server = ("127.0.0.1", 11211)
+
+        test_client.get.side_effect = socket.timeout()
+        with pytest.raises(socket.timeout):
+            client.get(b"key", noreply=False)
+        # Client gets removed because of socket timeout
+        assert ("127.0.0.1", 11211) in client._dead_clients
+
+        test_client.get.side_effect = lambda *_: "Some value"
+        # Client should be retried and brought back
+        assert client.get(b"key") == "Some value"
+        assert ("127.0.0.1", 11211) not in client._dead_clients
+
+    @mock.patch("pymemcache.client.hash.Client")
+    def test_failed_is_retried(self, client_patch):
+        client = HashClient([], retry_attempts=1, retry_timeout=0)
+        client.add_server("127.0.0.1", 11211)
+
+        assert client_patch.call_count == 1
+
+        test_client = client_patch.return_value
+        test_client.server = ("127.0.0.1", 11211)
+
+        test_client.get.side_effect = socket.timeout()
+        with pytest.raises(socket.timeout):
+            client.get(b"key", noreply=False)
+
+        test_client.get.side_effect = lambda *_: "Some value"
+        assert client.get(b"key") == "Some value"
+
+        assert client_patch.call_count == 1
+
     # TODO: Test failover logic