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 331339 b2c38c677cc7ba0b2b5ceb8f84cdace57ecd8121
parent 331338 6a623b9d45990722da19fc864c7ef820821bd4b0
child 331340 88a642111cc13023c64ecca920312e1b3e8b372c
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmgoodwin, MattN
bugs1257533, 1259145
milestone48.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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];
 }