launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+411913@xxxxxxxxxxxxxxxxxx>]

Jürgen Gmach has proposed merging ~jugmac00/launchpad:fix-pymemcache-regression-bytes-vs-str into launchpad:master.

Commit message:
Fix pymemcache regression in SnapStoreClient.listChannels

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1950811 in Launchpad itself: "pymemcache regression in SnapStoreClient.listChannels"
  https://bugs.launchpad.net/launchpad/+bug/1950811

For more details, see:
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/411913
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:fix-pymemcache-regression-bytes-vs-str into launchpad:master.

diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index b915da8..37ee026 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -9,7 +9,6 @@ __all__ = [
 
 from datetime import datetime
 from functools import partial
-import json
 import operator
 import os.path
 
@@ -824,8 +823,7 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             # and this should be relatively rare.
             enable_memcache = False
         dirname = os.path.dirname(filename)
-        unset = object()
-        file_list = unset
+        file_list = None
         if enable_memcache:
             memcache_client = getUtility(IMemcacheClient)
             instance_name = urlsplit(
@@ -833,16 +831,12 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             memcache_key = six.ensure_binary(
                 '%s:bzr-file-list:%s:%s:%s' % (
                     instance_name, self.id, revision_id, dirname))
-            cached_file_list = memcache_client.get(memcache_key)
-            if cached_file_list is not None:
-                try:
-                    file_list = json.loads(cached_file_list)
-                except Exception:
-                    logger.exception(
-                        'Cannot load cached file list for %s:%s:%s; deleting' %
-                        (self.unique_name, revision_id, dirname))
-                    memcache_client.delete(memcache_key)
-        if file_list is unset:
+            message = "file list for %s:%s:%s" % (
+                self.unique_name, revision_id, dirname
+            )
+            file_list = memcache_client.get_json(memcache_key, logger, message)
+
+        if file_list is None:
             try:
                 inventory = hosting_client.getInventory(
                     self.id, dirname, rev=revision_id)
@@ -854,7 +848,7 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             if enable_memcache:
                 # Cache the file list in case there's a request for another
                 # file in the same directory.
-                memcache_client.set(memcache_key, json.dumps(file_list))
+                memcache_client.set_json(memcache_key, file_list)
         file_id = (file_list or {}).get(os.path.basename(filename))
         if file_id is None:
             raise BranchFileNotFound(
diff --git a/lib/lp/code/model/gitref.py b/lib/lp/code/model/gitref.py
index ca4c05b..73c877f 100644
--- a/lib/lp/code/model/gitref.py
+++ b/lib/lp/code/model/gitref.py
@@ -9,7 +9,6 @@ __all__ = [
     ]
 
 from functools import partial
-import json
 import re
 
 from lazr.lifecycle.event import ObjectCreatedEvent
@@ -353,23 +352,17 @@ class GitRefMixin:
             if stop is not None:
                 memcache_key += ":stop=%s" % stop
             memcache_key = six.ensure_binary(memcache_key)
-            cached_log = memcache_client.get(memcache_key)
-            if cached_log is not None:
-                try:
-                    log = json.loads(cached_log)
-                except Exception:
-                    if logger is not None:
-                        logger.exception(
-                            "Cannot load cached log information for %s:%s; "
-                            "deleting" % (path, start))
-                    memcache_client.delete(memcache_key)
+
+            message = "log information for %s:%s" % (path, start)
+            log = memcache_client.get_json(memcache_key, logger, message)
+
         if log is None:
             if enable_hosting:
                 hosting_client = getUtility(IGitHostingClient)
                 log = removeSecurityProxy(hosting_client.getLog(
                     path, start, limit=limit, stop=stop, logger=logger))
                 if enable_memcache:
-                    memcache_client.set(memcache_key, json.dumps(log))
+                    memcache_client.set_json(memcache_key, log)
             else:
                 # Fall back to synthesising something reasonable based on
                 # information in our own database.
diff --git a/lib/lp/code/model/tests/test_branch.py b/lib/lp/code/model/tests/test_branch.py
index 9379266..defa405 100644
--- a/lib/lp/code/model/tests/test_branch.py
+++ b/lib/lp/code/model/tests/test_branch.py
@@ -3440,7 +3440,6 @@ class TestBranchGetBlob(TestCaseWithFactory):
         self.assertRaises(
             BranchFileNotFound, branch.getBlob,
             'src/README.txt', revision_id='some-rev')
-        self.assertEqual([], hosting_fixture.getInventory.calls)
         self.assertEqual([], hosting_fixture.getBlob.calls)
 
 
diff --git a/lib/lp/services/memcache/client.py b/lib/lp/services/memcache/client.py
index e04be16..3aa7f06 100644
--- a/lib/lp/services/memcache/client.py
+++ b/lib/lp/services/memcache/client.py
@@ -4,9 +4,11 @@
 """Launchpad Memcache client."""
 
 __all__ = [
+    'JsonClient',
     'memcache_client_factory',
     ]
 
+import json
 import re
 
 from pymemcache.client.hash import HashClient
@@ -18,8 +20,30 @@ from pymemcache.serde import (
 from lp.services.config import config
 
 
+class JsonClient(HashClient):
+    """memcached client with added JSON handling"""
+
+    def get_json(self, key, logger, message):
+        data = self.get(key)
+        if data is not None:
+            try:
+                data = json.loads(data)
+            # the exceptions are chosen deliberately in order to gracefully
+            # handle invalid data
+            except (TypeError, ValueError):
+                if logger and message:
+                    logger.exception(
+                        "Cannot load cached %s;deleting" % message
+                    )
+                self.delete(key)
+        return data
+
+    def set_json(self, key, value, expire=0):
+        self.set(key, json.dumps(value), expire)
+
+
 def memcache_client_factory(timeline=True):
-    """Return a pymemcache HashClient for Launchpad."""
+    """Return an extended pymemcache client for Launchpad."""
     # example value for config.memcache.servers:
     # (127.0.0.1:11242,1)
     servers = [
@@ -31,7 +55,7 @@ def memcache_client_factory(timeline=True):
         from lp.services.memcache.timeline import TimelineRecordingClient
         client_factory = TimelineRecordingClient
     else:
-        client_factory = HashClient
+        client_factory = JsonClient
     return client_factory(
         servers,
         serializer=python_memcache_serializer,
diff --git a/lib/lp/services/memcache/testing.py b/lib/lp/services/memcache/testing.py
index c4809f4..51df536 100644
--- a/lib/lp/services/memcache/testing.py
+++ b/lib/lp/services/memcache/testing.py
@@ -5,6 +5,7 @@ __all__ = [
     'MemcacheFixture',
     ]
 
+import json
 import time as _time
 
 import fixtures
@@ -30,6 +31,19 @@ class MemcacheFixture(fixtures.Fixture):
         else:
             return value
 
+    def get_json(self, key, logger, message):
+        data = self.get(key)
+        if data is not None:
+            try:
+                data = json.loads(data)
+            except (TypeError, ValueError):
+                if logger and message:
+                    logger.exception(
+                        "Cannot load cached %s;deleting" % message
+                    )
+                self.delete(key)
+        return data
+
     def set(self, key, val, expire=0):
         # memcached accepts either delta-seconds from the current time or
         # absolute epoch-seconds, and tells them apart using a magic
@@ -40,6 +54,9 @@ class MemcacheFixture(fixtures.Fixture):
         self._cache[key] = (val, expire)
         return 1
 
+    def set_json(self, key, value, expire=0):
+        self.set(key, json.dumps(value), expire)
+
     def delete(self, key):
         self._cache.pop(key, None)
         return 1
diff --git a/lib/lp/services/memcache/timeline.py b/lib/lp/services/memcache/timeline.py
index 9e1bf3e..74c2c6e 100644
--- a/lib/lp/services/memcache/timeline.py
+++ b/lib/lp/services/memcache/timeline.py
@@ -10,13 +10,13 @@ __all__ = [
 import logging
 
 from lazr.restful.utils import get_current_browser_request
-from pymemcache.client.hash import HashClient
 
 from lp.services import features
+from lp.services.memcache.client import JsonClient
 from lp.services.timeline.requesttimeline import get_request_timeline
 
 
-class TimelineRecordingClient(HashClient):
+class TimelineRecordingClient(JsonClient):
 
     def __get_timeline_action(self, suffix, key):
         request = get_current_browser_request()
@@ -36,7 +36,7 @@ class TimelineRecordingClient(HashClient):
             return None
         action = self.__get_timeline_action("get", key)
         try:
-            return HashClient.get(self, key)
+            return JsonClient.get(self, key)
         finally:
             action.finish()
 
@@ -45,7 +45,7 @@ class TimelineRecordingClient(HashClient):
             return None
         action = self.__get_timeline_action("set", key)
         try:
-            success = HashClient.set(self, key, value, expire=expire)
+            success = JsonClient.set(self, key, value, expire=expire)
             if success:
                 logging.debug("Memcache set succeeded for %s", key)
             else:
diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
index c43d705..30e7c45 100644
--- a/lib/lp/snappy/model/snapstoreclient.py
+++ b/lib/lp/snappy/model/snapstoreclient.py
@@ -390,19 +390,14 @@ class SnapStoreClient:
         """See `ISnapStoreClient`."""
         if config.snappy.store_search_url is None:
             return _default_store_channels
-        channels = None
         memcache_client = getUtility(IMemcacheClient)
         search_host = urlsplit(config.snappy.store_search_url).hostname
         memcache_key = ("%s:channels" % search_host).encode("UTF-8")
-        cached_channels = memcache_client.get(memcache_key)
-        if cached_channels is not None:
-            try:
-                channels = json.loads(cached_channels)
-            except ValueError:
-                log.exception(
-                    "Cannot load cached channels for %s; deleting" %
-                    search_host)
-                memcache_client.delete(memcache_key)
+        message = "channels for %s" % search_host
+
+        channels = memcache_client.get_json(
+            key=memcache_key, logger=log, message=message)
+
         if (channels is None and
                 not getFeatureFlag(u"snap.disable_channel_search")):
             path = "api/v1/channels"
@@ -423,8 +418,8 @@ class SnapStoreClient:
             DAY_IN_SECONDS = 60 * 60 * 24
             # pymemcache's `expire` expects an int
             expire_time = int(time.time()) + DAY_IN_SECONDS
-            memcache_client.set(
-                memcache_key, json.dumps(channels), expire_time)
+            memcache_client.set_json(
+                memcache_key, channels, expire_time)
         if channels is None:
             channels = _default_store_channels
         return channels