launchpad-reviewers team mailing list archive

[Colin Watson <mp+412082@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:tolerate-pymemcache-connection-errors into launchpad:master.

Commit message:
Tolerate connection errors from pymemcache as well

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/412082

We should (log and) ignore connection errors from pymemcache for the same reason as `MemcacheError`: it's an opportunistic cache, and a failure shouldn't cause the code trying to use it to fail.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:tolerate-pymemcache-connection-errors into launchpad:master.

diff --git a/lib/lp/services/memcache/client.py b/lib/lp/services/memcache/client.py
index 2eb2b9a..69f7fd1 100644
--- a/lib/lp/services/memcache/client.py
+++ b/lib/lp/services/memcache/client.py
@@ -33,7 +33,7 @@ class MemcacheClient(HashClient):
             return super().get(key, default=None)
         except MemcacheClientError:
             raise
-        except MemcacheError as e:
+        except (MemcacheError, OSError) as e:
             if logger is not None:
                 logger.exception("Cannot get %s from memcached: %s" % (key, e))
             return default
@@ -44,7 +44,7 @@ class MemcacheClient(HashClient):
             return super().set(key, value, expire=expire)
         except MemcacheClientError:
             raise
-        except MemcacheError as e:
+        except (MemcacheError, OSError) as e:
             if logger is not None:
                 logger.exception("Cannot set %s in memcached: %s" % (key, e))
             return False
@@ -55,7 +55,7 @@ class MemcacheClient(HashClient):
             return super().delete(key)
         except MemcacheClientError:
             raise
-        except MemcacheError as e:
+        except (MemcacheError, OSError) as e:
             if logger is not None:
                 logger.exception(
                     "Cannot delete %s from memcached: %s" % (key, e))
diff --git a/lib/lp/services/memcache/tests/test_memcache_client.py b/lib/lp/services/memcache/tests/test_memcache_client.py
index 72400fe..5574d22 100644
--- a/lib/lp/services/memcache/tests/test_memcache_client.py
+++ b/lib/lp/services/memcache/tests/test_memcache_client.py
@@ -66,6 +66,17 @@ class MemcacheClientTestCase(TestCase):
                 "ERROR Cannot get foo from memcached: All servers down\n",
                 logger.content.as_text())
 
+    def test_get_connection_refused(self):
+        logger = BufferLogger()
+        with patch.object(self.client, "_get_client") as mock_get_client:
+            mock_get_client.side_effect = ConnectionRefusedError(
+                "Connection refused")
+            self.assertIsNone(self.client.get("foo"))
+            self.assertIsNone(self.client.get("foo", logger=logger))
+            self.assertEqual(
+                "ERROR Cannot get foo from memcached: Connection refused\n",
+                logger.content.as_text())
+
     def test_set_failure(self):
         logger = BufferLogger()
         with patch.object(self.client, "_get_client") as mock_get_client:
@@ -76,6 +87,17 @@ class MemcacheClientTestCase(TestCase):
                 "ERROR Cannot set foo in memcached: All servers down\n",
                 logger.content.as_text())
 
+    def test_set_connection_refused(self):
+        logger = BufferLogger()
+        with patch.object(self.client, "_get_client") as mock_get_client:
+            mock_get_client.side_effect = ConnectionRefusedError(
+                "Connection refused")
+            self.assertFalse(self.client.set("foo", "bar"))
+            self.assertFalse(self.client.set("foo", "bar", logger=logger))
+            self.assertEqual(
+                "ERROR Cannot set foo in memcached: Connection refused\n",
+                logger.content.as_text())
+
     def test_delete_failure(self):
         logger = BufferLogger()
         with patch.object(self.client, "_get_client") as mock_get_client:
@@ -86,6 +108,17 @@ class MemcacheClientTestCase(TestCase):
                 "ERROR Cannot delete foo from memcached: All servers down\n",
                 logger.content.as_text())
 
+    def test_delete_connection_refused(self):
+        logger = BufferLogger()
+        with patch.object(self.client, "_get_client") as mock_get_client:
+            mock_get_client.side_effect = ConnectionRefusedError(
+                "Connection refused")
+            self.assertFalse(self.client.delete("foo"))
+            self.assertFalse(self.client.delete("foo", logger=logger))
+            self.assertEqual(
+                "ERROR Cannot delete foo from memcached: Connection refused\n",
+                logger.content.as_text())
+
 
 class MemcacheClientFactoryTestCase(TestCase):