Bug 1257533 - Optimize and add safety checks in Kinto updater. r=mgoodwin,MattN
authorMathieu Leplatre <mathieu@mozilla.com>
Fri, 15 Apr 2016 16:50:51 +0200
changeset 293401 b2c38c677cc7ba0b2b5ceb8f84cdace57ecd8121
parent 293400 6a623b9d45990722da19fc864c7ef820821bd4b0
child 293402 88a642111cc13023c64ecca920312e1b3e8b372c
push id18757
push usermozilla@noorenberghe.ca
push dateFri, 15 Apr 2016 20:48:15 +0000
treeherderfx-team@b2c38c677cc7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmgoodwin, MattN
bugs1257533, 1259145
milestone48.0a1
Bug 1257533 - Optimize and add safety checks in Kinto updater. r=mgoodwin,MattN - Skip changes from other bucket - Leverage ETag to limit bandwidth - Use setting for collection name - Add safety check when server is failing. This also fixes Bug 1259145. MozReview-Commit-ID: 5YSVCrZirzQ
services/common/kinto-updater.js
services/common/tests/unit/test_kinto_updater.js
--- a/services/common/kinto-updater.js
+++ b/services/common/kinto-updater.js
@@ -7,74 +7,110 @@ this.EXPORTED_SYMBOLS = ["checkVersions"
 const { classes: Cc, Constructor: CC, interfaces: Ci, utils: Cu } = Components;
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.importGlobalProperties(['fetch']);
 
 const PREF_KINTO_CHANGES_PATH = "services.kinto.changes.path";
 const PREF_KINTO_BASE = "services.kinto.base";
+const PREF_KINTO_BUCKET = "services.kinto.bucket";
 const PREF_KINTO_LAST_UPDATE = "services.kinto.last_update_seconds";
+const PREF_KINTO_LAST_ETAG = "services.kinto.last_etag";
 const PREF_KINTO_CLOCK_SKEW_SECONDS = "services.kinto.clock_skew_seconds";
+const PREF_KINTO_ONECRL_COLLECTION = "services.kinto.onecrl.collection";
 
 const kintoClients = {
 };
 
 // This is called by the ping mechanism.
 // returns a promise that rejects if something goes wrong
 this.checkVersions = function() {
+
   return Task.spawn(function *() {
     // Fetch a versionInfo object that looks like:
     // {"data":[
     //   {
     //     "host":"kinto-ota.dev.mozaws.net",
     //     "last_modified":1450717104423,
     //     "bucket":"blocklists",
     //     "collection":"certificates"
     //    }]}
     // Right now, we only use the collection name and the last modified info
     let kintoBase = Services.prefs.getCharPref(PREF_KINTO_BASE);
     let changesEndpoint = kintoBase + Services.prefs.getCharPref(PREF_KINTO_CHANGES_PATH);
+    let blocklistsBucket = Services.prefs.getCharPref(PREF_KINTO_BUCKET);
 
-    let response = yield fetch(changesEndpoint);
+    // Use ETag to obtain a `304 Not modified` when no change occurred.
+    const headers = {};
+    if (Services.prefs.prefHasUserValue(PREF_KINTO_LAST_ETAG)) {
+      const lastEtag = Services.prefs.getCharPref(PREF_KINTO_LAST_ETAG);
+      if (lastEtag) {
+        headers["If-None-Match"] = lastEtag;
+      }
+    }
+
+    let response = yield fetch(changesEndpoint, {headers});
+
+    let versionInfo;
+    // No changes since last time. Go on with empty list of changes.
+    if (response.status == 304) {
+      versionInfo = {data: []};
+    } else {
+      versionInfo = yield response.json();
+    }
+
+    // If the server is failing, the JSON response might not contain the
+    // expected data (e.g. error response - Bug 1259145)
+    if (!versionInfo.hasOwnProperty("data")) {
+      throw new Error("Polling for changes failed.");
+    }
 
     // Record new update time and the difference between local and server time
     let serverTimeMillis = Date.parse(response.headers.get("Date"));
     let clockDifference = Math.abs(Date.now() - serverTimeMillis) / 1000;
+    Services.prefs.setIntPref(PREF_KINTO_CLOCK_SKEW_SECONDS, clockDifference);
     Services.prefs.setIntPref(PREF_KINTO_LAST_UPDATE, serverTimeMillis / 1000);
-    Services.prefs.setIntPref(PREF_KINTO_CLOCK_SKEW_SECONDS, clockDifference);
-
-    let versionInfo = yield response.json();
 
     let firstError;
     for (let collectionInfo of versionInfo.data) {
+      // Skip changes that don't concern configured blocklist bucket.
+      if (collectionInfo.bucket != blocklistsBucket) {
+        continue;
+      }
+
       let collection = collectionInfo.collection;
       let kintoClient = kintoClients[collection];
       if (kintoClient && kintoClient.maybeSync) {
         let lastModified = 0;
         if (collectionInfo.last_modified) {
-          lastModified = collectionInfo.last_modified
+          lastModified = collectionInfo.last_modified;
         }
         try {
           yield kintoClient.maybeSync(lastModified, serverTimeMillis);
         } catch (e) {
           if (!firstError) {
             firstError = e;
           }
         }
       }
     }
     if (firstError) {
       // cause the promise to reject by throwing the first observed error
       throw firstError;
     }
+
+    // Save current Etag for next poll.
+    if (response.headers.has("ETag")) {
+      const currentEtag = response.headers.get("ETag");
+      Services.prefs.setCharPref(PREF_KINTO_LAST_ETAG, currentEtag);
+    }
   });
 };
 
 // Add a kintoClient for testing purposes. Do not use for any other purpose
 this.addTestKintoClient = function(name, kintoClient) {
   kintoClients[name] = kintoClient;
 };
 
 // Add the various things that we know want updates
-kintoClients.certificates =
-  Cu.import("resource://services-common/KintoCertificateBlocklist.js", {})
-  .OneCRLClient;
+const KintoBlocklist = Cu.import("resource://services-common/KintoCertificateBlocklist.js", {});
+kintoClients[Services.prefs.getCharPref(PREF_KINTO_ONECRL_COLLECTION)]  = KintoBlocklist.OneCRLClient;
--- a/services/common/tests/unit/test_kinto_updater.js
+++ b/services/common/tests/unit/test_kinto_updater.js
@@ -3,16 +3,17 @@
 
 Cu.import("resource://services-common/kinto-updater.js")
 Cu.import("resource://testing-common/httpd.js");
 
 var server;
 
 const PREF_KINTO_BASE = "services.kinto.base";
 const PREF_LAST_UPDATE = "services.kinto.last_update_seconds";
+const PREF_LAST_ETAG = "services.kinto.last_etag";
 const PREF_CLOCK_SKEW_SECONDS = "services.kinto.clock_skew_seconds";
 
 // Check to ensure maybeSync is called with correct values when a changes
 // document contains information on when a collection was last modified
 add_task(function* test_check_maybeSync(){
   const changesPath = "/v1/buckets/monitor/collections/changes/records";
 
   // register a handler
@@ -26,45 +27,46 @@ add_task(function* test_check_maybeSync(
       response.setStatusLine(null, sampled.status.status,
                              sampled.status.statusText);
       // send the headers
       for (let headerLine of sampled.sampleHeaders) {
         let headerElements = headerLine.split(':');
         response.setHeader(headerElements[0], headerElements[1].trimLeft());
       }
 
-      // set the
+      // set the server date
       response.setHeader("Date", (new Date(2000)).toUTCString());
 
       response.write(sampled.responseBody);
     } catch (e) {
       dump(`${e}\n`);
     }
   }
 
   server.registerPathHandler(changesPath, handleResponse);
 
   // set up prefs so the kinto updater talks to the test server
   Services.prefs.setCharPref(PREF_KINTO_BASE,
     `http://localhost:${server.identity.primaryPort}/v1`);
 
   // set some initial values so we can check these are updated appropriately
-  Services.prefs.setIntPref("services.kinto.last_update", 0);
-  Services.prefs.setIntPref("services.kinto.clock_difference", 0);
+  Services.prefs.setIntPref(PREF_LAST_UPDATE, 0);
+  Services.prefs.setIntPref(PREF_CLOCK_SKEW_SECONDS, 0);
+  Services.prefs.clearUserPref(PREF_LAST_ETAG);
 
 
   let startTime = Date.now();
 
+  let updater = Cu.import("resource://services-common/kinto-updater.js");
+
   let syncPromise = new Promise(function(resolve, reject) {
-    let updater = Cu.import("resource://services-common/kinto-updater.js");
     // add a test kinto client that will respond to lastModified information
     // for a collection called 'test-collection'
     updater.addTestKintoClient("test-collection", {
-      "maybeSync": function(lastModified, serverTime){
-        // ensire the lastModified and serverTime values are as expected
+      maybeSync(lastModified, serverTime) {
         do_check_eq(lastModified, 1000);
         do_check_eq(serverTime, 2000);
         resolve();
       }
     });
     updater.checkVersions();
   });
 
@@ -75,16 +77,54 @@ add_task(function* test_check_maybeSync(
   do_check_eq(Services.prefs.getIntPref(PREF_LAST_UPDATE), 2);
 
   // How does the clock difference look?
   let endTime = Date.now();
   let clockDifference = Services.prefs.getIntPref(PREF_CLOCK_SKEW_SECONDS);
   // we previously set the serverTime to 2 (seconds past epoch)
   do_check_eq(clockDifference <= endTime / 1000
               && clockDifference >= Math.floor(startTime / 1000) - 2, true);
+  // Last timestamp was saved. An ETag header value is a quoted string.
+  let lastEtag = Services.prefs.getCharPref(PREF_LAST_ETAG);
+  do_check_eq(lastEtag, "\"1100\"");
+
+
+  // Simulate a poll with up-to-date collection.
+  Services.prefs.setIntPref(PREF_LAST_UPDATE, 0);
+  // If server has no change, a 304 is received, maybeSync() is not called.
+  updater.addTestKintoClient("test-collection", {
+    maybeSync: () => {throw new Error("Should not be called");}
+  });
+  yield updater.checkVersions();
+  // Last update is overwritten
+  do_check_eq(Services.prefs.getIntPref(PREF_LAST_UPDATE), 2);
+
+
+  // Simulate a server error.
+  function simulateErrorResponse (request, response) {
+    response.setHeader("Date", (new Date(3000)).toUTCString());
+    response.setHeader("Content-Type", "application/json; charset=UTF-8");
+    response.write(JSON.stringify({
+      code: 503,
+      errno: 999,
+      error: "Service Unavailable",
+    }));
+    response.setStatusLine(null, 503, "Service Unavailable");
+  }
+  server.registerPathHandler(changesPath, simulateErrorResponse);
+  // checkVersions() fails with adequate error.
+  let error;
+  try {
+    yield updater.checkVersions();
+  } catch (e) {
+    error = e;
+  }
+  do_check_eq(error.message, "Polling for changes failed.");
+  // When an error occurs, last update was not overwritten (see Date header above).
+  do_check_eq(Services.prefs.getIntPref(PREF_LAST_UPDATE), 2);
 });
 
 function run_test() {
   // Set up an HTTP Server
   server = new HttpServer();
   server.start(-1);
 
   run_next_test();
@@ -94,17 +134,34 @@ function run_test() {
   });
 }
 
 // get a response for a given request from sample data
 function getSampleResponse(req, port) {
   const responses = {
     "GET:/v1/buckets/monitor/collections/changes/records?": {
       "sampleHeaders": [
-        "Content-Type: application/json; charset=UTF-8"
+        "Content-Type: application/json; charset=UTF-8",
+        "ETag: \"1100\""
       ],
       "status": {status: 200, statusText: "OK"},
-      "responseBody": JSON.stringify({"data":[{"host":"localhost","last_modified":1000,"bucket":"blocklists","id":"330a0c5f-fadf-ff0b-40c8-4eb0d924ff6a","collection":"test-collection"}]})
+      "responseBody": JSON.stringify({"data": [{
+        "host": "localhost",
+        "last_modified": 1100,
+        "bucket": "blocklists:aurora",
+        "id": "330a0c5f-fadf-ff0b-40c8-4eb0d924ff6a",
+        "collection": "test-collection"
+      }, {
+        "host": "localhost",
+        "last_modified": 1000,
+        "bucket": "blocklists",
+        "id": "254cbb9e-6888-4d9f-8e60-58b74faa8778",
+        "collection": "test-collection"
+      }]})
     }
   };
+
+  if (req.hasHeader("if-none-match") && req.getHeader("if-none-match", "") == "\"1100\"")
+    return {sampleHeaders: [], status: {status: 304, statusText: "Not Modified"}, responseBody: ""};
+
   return responses[`${req.method}:${req.path}?${req.queryString}`] ||
          responses[req.method];
 }