Bug 582083 - Should inspect POST responses for failed WBOs [r=Mardak]
authorPhilipp von Weitershausen <philipp@weitershausen.de>
Sat, 31 Jul 2010 13:28:00 +0200
changeset 48773 b39aabb7bed36ec9b5df3e3d49170ec1b8444467
parent 48330 f5db8839aaec8b1fd0e023e5ca9d1ff378e0994c
child 48774 0b8b9f147452215c5a4c59fef0c4ba1e35903b6c
push id14809
push userposhannessy@mozilla.com
push dateTue, 03 Aug 2010 06:50:00 +0000
treeherdermozilla-central@51051a9791ee [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMardak
bugs582083
Bug 582083 - Should inspect POST responses for failed WBOs [r=Mardak] Make sure records that failed to upload continue to be marked in the tracker so that they'll be uploaded again in the next sync.
services/sync/modules/engines.js
services/sync/tests/unit/head_http_server.js
services/sync/tests/unit/test_syncengine_sync.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -634,16 +634,17 @@ SyncEngine.prototype = {
       this._handleDupe(item, dupeId);
 
     // Apply the incoming item (now that the dupe is the right id)
     return true;
   },
 
   // Upload outgoing records
   _uploadOutgoing: function SyncEngine__uploadOutgoing() {
+    let failed = {};
     let outnum = [i for (i in this._tracker.changedIDs)].length;
     if (outnum) {
       this._log.trace("Preparing " + outnum + " outgoing records");
 
       // collection we'll upload
       let up = new Collection(this.engineURL);
       let count = 0;
 
@@ -657,16 +658,28 @@ SyncEngine.prototype = {
           throw resp;
         }
 
         // Record the modified time of the upload
         let modified = resp.headers["x-weave-timestamp"];
         if (modified > this.lastSync)
           this.lastSync = modified;
 
+        // Remember changed IDs and timestamp of failed items so we
+        // can mark them changed again.
+        let failed_ids = [];
+        for (let id in resp.obj.failed) {
+          failed[id] = this._tracker.changedIDs[id];
+          failed_ids.push(id);
+        }
+        if (failed_ids.length)
+          this._log.debug("Records that will be uploaded again because "
+                          + "the server couldn't store them: "
+                          + failed_ids.join(", "));
+
         up.clearRecords();
       });
 
       for (let id in this._tracker.changedIDs) {
         try {
           let out = this._createRecord(id);
           if (this._log.level <= Log4Moz.Level.Trace)
             this._log.trace("Outgoing: " + out);
@@ -685,16 +698,21 @@ SyncEngine.prototype = {
         Sync.sleep(0);
       }
 
       // Final upload
       if (count % MAX_UPLOAD_RECORDS > 0)
         doUpload(count >= MAX_UPLOAD_RECORDS ? "last batch" : "all");
     }
     this._tracker.clearChangedIDs();
+
+    // Mark failed WBOs as changed again so they are reuploaded next time.
+    for (let id in failed) {
+      this._tracker.addChangedID(id, failed[id]);
+    }
   },
 
   // Any cleanup necessary.
   // Save the current snapshot so as to calculate changes at next sync
   _syncFinish: function SyncEngine__syncFinish() {
     this._log.trace("Finishing up sync");
     this._tracker.resetScore();
 
--- a/services/sync/tests/unit/head_http_server.js
+++ b/services/sync/tests/unit/head_http_server.js
@@ -148,31 +148,32 @@ ServerCollection.prototype = {
       result = JSON.stringify(data);
     }
     return result;
   },
 
   post: function(input) {
     input = JSON.parse(input);
     let success = [];
-    let failed = [];
+    let failed = {};
 
     // This will count records where we have an existing ServerWBO
     // registered with us as successful and all other records as failed.
     for each (let record in input) {
       let wbo = this.wbos[record.id];
       if (wbo) {
         wbo.payload = record.payload;
         wbo.modified = Date.now() / 1000;
         success.push(record.id);
       } else {
-        failed.push(record.id);
+        failed[record.id] = "no wbo configured";
       }
     }
-    return {success: success,
+    return {modified: Date.now() / 1000,
+            success: success,
             failed: failed};
   },
 
   delete: function(options) {
     for (let [id, wbo] in Iterator(this.wbos)) {
       if (this._inResultSet(wbo, options)) {
         wbo.delete();
       }
--- a/services/sync/tests/unit/test_syncengine_sync.js
+++ b/services/sync/tests/unit/test_syncengine_sync.js
@@ -832,16 +832,75 @@ function test_uploadOutgoing_toEmptyServ
     Svc.Prefs.resetBranch("");
     Records.clearCache();
     CryptoMetas.clearCache();
     syncTesting = new SyncTestingInfrastructure(makeSteamEngine);
   }
 }
 
 
+function test_uploadOutgoing_failed() {
+  _("SyncEngine._uploadOutgoing doesn't clear the tracker of objects that failed to upload.");
+
+  Svc.Prefs.set("clusterURL", "http://localhost:8080/");
+  Svc.Prefs.set("username", "foo");
+  let crypto_steam = new ServerWBO('steam');
+  let collection = new ServerCollection();
+  // We only define the "flying" WBO on the server, not the "scotsman"
+  // and "peppercorn" ones.
+  collection.wbos.flying = new ServerWBO('flying');
+
+  let server = sync_httpd_setup({
+      "/1.0/foo/storage/crypto/steam": crypto_steam.handler(),
+      "/1.0/foo/storage/steam": collection.handler()
+  });
+  createAndUploadKeypair();
+  createAndUploadSymKey("http://localhost:8080/1.0/foo/storage/crypto/steam");
+
+  let engine = makeSteamEngine();
+  engine._store.items = {flying: "LNER Class A3 4472",
+                         scotsman: "Flying Scotsman",
+                         peppercorn: "Peppercorn Class"};
+  // Mark one of these records as changed 
+  const FLYING_CHANGED = 12345;
+  const SCOTSMAN_CHANGED = 23456;
+  const PEPPERCORN_CHANGED = 34567;
+  engine._tracker.addChangedID('flying', FLYING_CHANGED);
+  engine._tracker.addChangedID('scotsman', SCOTSMAN_CHANGED);
+  engine._tracker.addChangedID('peppercorn', PEPPERCORN_CHANGED);
+
+  try {
+
+    // Confirm initial environment
+    do_check_eq(collection.wbos.flying.payload, undefined);
+    do_check_eq(engine._tracker.changedIDs['flying'], FLYING_CHANGED);
+    do_check_eq(engine._tracker.changedIDs['scotsman'], SCOTSMAN_CHANGED);
+    do_check_eq(engine._tracker.changedIDs['peppercorn'], PEPPERCORN_CHANGED);
+
+    engine._uploadOutgoing();
+
+    // Ensure the 'flying' record has been uploaded and is no longer marked.
+    do_check_true(!!collection.wbos.flying.payload);
+    do_check_eq(engine._tracker.changedIDs['flying'], undefined);
+
+    // The 'scotsman' and 'peppercorn' records couldn't be uploaded so
+    // they weren't cleared from the tracker.
+    do_check_eq(engine._tracker.changedIDs['scotsman'], SCOTSMAN_CHANGED);
+    do_check_eq(engine._tracker.changedIDs['peppercorn'], PEPPERCORN_CHANGED);
+
+  } finally {
+    server.stop(function() {});
+    Svc.Prefs.resetBranch("");
+    Records.clearCache();
+    CryptoMetas.clearCache();
+    syncTesting = new SyncTestingInfrastructure(makeSteamEngine);
+  }
+}
+
+
 function test_uploadOutgoing_MAX_UPLOAD_RECORDS() {
   _("SyncEngine._uploadOutgoing uploads in batches of MAX_UPLOAD_RECORDS");
 
   Svc.Prefs.set("clusterURL", "http://localhost:8080/");
   Svc.Prefs.set("username", "foo");
   let crypto_steam = new ServerWBO('steam');
   let collection = new ServerCollection();
 
@@ -1029,13 +1088,14 @@ function run_test() {
   test_syncStartup_serverHasNewerVersion();
   test_syncStartup_syncIDMismatchResetsClient();
   test_syncStartup_badKeyWipesServerData();
   test_processIncoming_emptyServer();
   test_processIncoming_createFromServer();
   test_processIncoming_reconcile();
   test_processIncoming_fetchNum();
   test_uploadOutgoing_toEmptyServer();
+  test_uploadOutgoing_failed();
   test_uploadOutgoing_MAX_UPLOAD_RECORDS();
   test_syncFinish_noDelete();
   test_syncFinish_deleteByIds();
   test_syncFinish_deleteLotsInBatches();
 }