add a persistent cache of URL lookups, and fix up the in-memory one to work

synapse/http/client.py

@@ -251,8 +251,8 @@ class SimpleHttpClient(object):
             url (str): The URL to GET
             output_stream (file): File to write the response body to.
         Returns:
-            A (int,dict) tuple of the file length and a dict of the response
+            A (int,dict,string,int) tuple of the file length, dict of the response
-            headers.
+            headers, absolute URI of the response and HTTP response code.
         """
 
         response = yield self.request(
@@ -287,7 +287,7 @@ class SimpleHttpClient(object):
             logger.exception("Failed to download body")
             raise
 
-        defer.returnValue((length, headers, response.request.absoluteURI))
+        defer.returnValue((length, headers, response.request.absoluteURI, response.code))
 
 
 # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient.

synapse/rest/media/v1/preview_url_resource.py

@@ -37,6 +37,8 @@ class PreviewUrlResource(BaseMediaResource):
     def __init__(self, hs, filepaths):
         BaseMediaResource.__init__(self, hs, filepaths)
         self.client = SpiderHttpClient(hs)
+
+        # simple memory cache mapping urls to OG metadata
         self.cache = ExpiringCache(
             cache_name = "url_previews",
             clock = self.clock,
@@ -56,17 +58,41 @@ class PreviewUrlResource(BaseMediaResource):
             # XXX: if get_user_by_req fails, what should we do in an async render?
             requester = yield self.auth.get_user_by_req(request)
             url = request.args.get("url")[0]
-
+            ts = request.args.get("ts")[0] if "ts" in request.args else self.clock.time_msec()
-            if self.cache:
-                og = self.cache.get(url)
-                respond_with_json_bytes(request, 200, json.dumps(og), send_cors=True)
-                return
 
             # TODO: keep track of whether there's an ongoing request for this preview
             # and block and return their details if there is one.
 
+            # first check the memory cache - good to handle all the clients on this
+            # HS thundering away to preview the same URL at the same time.
+            try:
+                og = self.cache[url]
+                respond_with_json_bytes(request, 200, json.dumps(og), send_cors=True)
+                return
+            except:
+                pass
+
+            # then check the URL cache in the DB (which will also provide us with
+            # historical previews, if we have any)
+            cache_result = yield self.store.get_url_cache(url, ts)
+            if (
+                cache_result and
+                cache_result["download_ts"] + cache_result["expires"] > ts and
+                cache_result["response_code"] / 100 == 2
+            ):
+                respond_with_json_bytes(
+                    request, 200, cache_result["og"].encode('utf-8'),
+                    send_cors=True
+                )
+                return
+
             media_info = yield self._download_url(url, requester.user)
 
+            # FIXME: we should probably update our cache now anyway, so that
+            # even if the OG calculation raises, we don't keep hammering on the
+            # remote server.  For now, leave it uncached to aid debugging OG
+            # calculation problems
+
             logger.debug("got media_info of '%s'" % media_info)
 
             if self._is_media(media_info['media_type']):
@@ -105,10 +131,21 @@ class PreviewUrlResource(BaseMediaResource):
                 logger.warn("Failed to find any OG data in %s", url)
                 og = {}
 
-            if self.cache:
+            logger.debug("Calculated OG for %s as %s" % (url, og));
+
+            # store OG in ephemeral in-memory cache
             self.cache[url] = og
 
-            logger.warn(og);
+            # store OG in history-aware DB cache
+            yield self.store.store_url_cache(
+                url,
+                media_info["response_code"],
+                media_info["etag"],
+                media_info["expires"],
+                json.dumps(og),
+                media_info["filesystem_id"],
+                media_info["created_ts"],
+            )
 
             respond_with_json_bytes(request, 200, json.dumps(og), send_cors=True)
         except:
@@ -187,6 +224,9 @@ class PreviewUrlResource(BaseMediaResource):
                     og['og:image'] = self._rebase_url(images[0].attrib['src'], media_info['uri'])
 
         # pre-cache the image for posterity
+        # FIXME: it might be cleaner to use the same flow as the main /preview_url request itself
+        # and benefit from the same caching etc.  But for now we just rely on the caching
+        # of the master request to speed things up.
         if 'og:image' in og and og['og:image']:
             image_info = yield self._download_url(og['og:image'], requester.user)
 
@@ -226,7 +266,6 @@ class PreviewUrlResource(BaseMediaResource):
                 text = text.strip()[:500]
                 og['og:description'] = text if text else None
 
-        # TODO: persist a cache mapping { url, etag } -> { og, mxc of url (if we bother keeping it around), age }
         # TODO: delete the url downloads to stop diskfilling, as we only ever cared about its OG
         defer.returnValue(og);
 
@@ -256,7 +295,7 @@ class PreviewUrlResource(BaseMediaResource):
         try:
             with open(fname, "wb") as f:
                 logger.debug("Trying to get url '%s'" % url)
-                length, headers, uri = yield self.client.get_file(
+                length, headers, uri, code = yield self.client.get_file(
                     url, output_stream=f, max_size=self.max_spider_size,
                 )
                 # FIXME: pass through 404s and other error messages nicely
@@ -311,6 +350,11 @@ class PreviewUrlResource(BaseMediaResource):
             "filesystem_id": file_id,
             "filename": fname,
             "uri": uri,
+            "response_code": code,
+            # FIXME: we should calculate a proper expiration based on the
+            # Cache-Control and Expire headers.  But for now, assume 1 hour.
+            "expires": 60 * 60 * 1000,
+            "etag": headers["ETag"] if "ETag" in headers else None,
         })
 
     def _is_media(self, content_type):

synapse/storage/media_repository.py

@@ -25,7 +25,7 @@ class MediaRepositoryStore(SQLBaseStore):
     def get_local_media(self, media_id):
         """Get the metadata for a local piece of media
         Returns:
-            None if the meia_id doesn't exist.
+            None if the media_id doesn't exist.
         """
         return self._simple_select_one(
             "local_media_repository",
@@ -50,6 +50,58 @@ class MediaRepositoryStore(SQLBaseStore):
             desc="store_local_media",
         )
 
+    def get_url_cache(self, url, ts):
+        """Get the media_id and ts for a cached URL as of the given timestamp
+        Returns:
+            None if the URL isn't cached.
+        """
+        def get_url_cache_txn(txn):
+            # get the most recently cached result (relative to the given ts)
+            sql = (
+                "SELECT response_code, etag, expires, og, media_id, max(download_ts)"
+                " FROM local_media_repository_url_cache"
+                " WHERE url = ? AND download_ts <= ?"
+            )
+            txn.execute(sql, (url, ts))
+            row = txn.fetchone()
+
+            if not row[3]:
+                # ...or if we've requested a timestamp older than the oldest
+                # copy in the cache, return the oldest copy (if any)
+                sql = (
+                    "SELECT response_code, etag, expires, og, media_id, min(download_ts)"
+                    " FROM local_media_repository_url_cache"
+                    " WHERE url = ? AND download_ts > ?"
+                )
+                txn.execute(sql, (url, ts))
+                row = txn.fetchone()
+
+            if not row[3]:
+                return None
+
+            return dict(zip((
+                'response_code', 'etag', 'expires', 'og', 'media_id', 'download_ts'
+            ), row))
+
+        return self.runInteraction(
+            "get_url_cache", get_url_cache_txn
+        )
+
+    def store_url_cache(self, url, response_code, etag, expires, og, media_id, download_ts):
+        return self._simple_insert(
+            "local_media_repository_url_cache",
+            {
+                "url": url,
+                "response_code": response_code,
+                "etag": etag,
+                "expires": expires,
+                "og": og,
+                "media_id": media_id,
+                "download_ts": download_ts,
+            },
+            desc="store_url_cache",
+        )
+
     def get_local_media_thumbnails(self, media_id):
         return self._simple_select_list(
             "local_media_repository_thumbnails",

synapse/storage/schema/delta/30/local_media_repository_url_cache.sql

@@ -0,0 +1,27 @@
+/* Copyright 2016 OpenMarket Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+CREATE TABLE local_media_repository_url_cache(
+    url TEXT,              -- the URL being cached
+    response_code INTEGER, -- the HTTP response code of this download attempt
+    etag TEXT,             -- the etag header of this response
+    expires INTEGER,       -- the number of ms this response was valid for
+    og TEXT,               -- cache of the OG metadata of this URL as JSON
+    media_id TEXT,         -- the media_id, if any, of the URL's content in the repo
+    download_ts BIGINT     -- the timestamp of this download attempt
+);
+
+CREATE INDEX local_media_repository_url_cache_by_url_download_ts
+    ON local_media_repository_url_cache(url, download_ts);