Bug 1151077 - Make the desktop reading list sync module batch its POST /batch requests. r=markh, a=sledru
authorDrew Willcoxon <adw@mozilla.com>
Tue, 07 Apr 2015 15:49:28 -0700
changeset 258435 1b6ba1cb52f6
parent 258434 ab0337907115
child 258436 dffb5c867f47
push id4668
push userryanvm@gmail.com
push date2015-04-13 16:23 +0000
treeherdermozilla-beta@002faed66e96 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, sledru
bugs1151077
milestone38.0
Bug 1151077 - Make the desktop reading list sync module batch its POST /batch requests. r=markh, a=sledru
browser/components/readinglist/Sync.jsm
--- a/browser/components/readinglist/Sync.jsm
+++ b/browser/components/readinglist/Sync.jsm
@@ -16,16 +16,20 @@ Cu.import("resource://gre/modules/XPCOMU
 
 XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
   "resource://gre/modules/Preferences.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "ReadingList",
   "resource:///modules/readinglist/ReadingList.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "ServerClient",
   "resource:///modules/readinglist/ServerClient.jsm");
 
+// The maximum number of sub-requests per POST /batch supported by the server.
+// See http://readinglist.readthedocs.org/en/latest/api/batch.html.
+const BATCH_REQUEST_LIMIT = 25;
+
 // The Last-Modified header of server responses is stored here.
 const SERVER_LAST_MODIFIED_HEADER_PREF = "readinglist.sync.serverLastModified";
 
 // Maps local record properties to server record properties.
 const SERVER_PROPERTIES_BY_LOCAL_PROPERTIES = {
   guid: "id",
   serverLastModified: "last_modified",
   url: "url",
@@ -175,26 +179,24 @@ SyncImpl.prototype = {
     }, { syncStatus: syncStatus });
     if (!requests.length) {
       log.debug("No local changes to upload");
       return;
     }
 
     // Send the request.
     let request = {
-      method: "POST",
-      path: "/batch",
       body: {
         defaults: {
           method: "PATCH",
         },
         requests: requests,
       },
     };
-    let batchResponse = yield this._sendRequest(request);
+    let batchResponse = yield this._postBatch(request);
     if (batchResponse.status != 200) {
       this._handleUnexpectedResponse("uploading changes", batchResponse);
       return;
     }
 
     // Update local items based on the response.
     for (let response of batchResponse.body.responses) {
       if (response.status == 404) {
@@ -239,27 +241,25 @@ SyncImpl.prototype = {
     }, { syncStatus: ReadingList.SyncStatus.NEW });
     if (!requests.length) {
       log.debug("No new local items to upload");
       return;
     }
 
     // Send the request.
     let request = {
-      method: "POST",
-      path: "/batch",
       body: {
         defaults: {
           method: "POST",
           path: "/articles",
         },
         requests: requests,
       },
     };
-    let batchResponse = yield this._sendRequest(request);
+    let batchResponse = yield this._postBatch(request);
     if (batchResponse.status != 200) {
       this._handleUnexpectedResponse("uploading new items", batchResponse);
       return;
     }
 
     // Update local items based on the response.
     for (let response of batchResponse.body.responses) {
       if (response.status == 303) {
@@ -303,26 +303,24 @@ SyncImpl.prototype = {
     });
     if (!requests.length) {
       log.debug("No local deleted synced items to upload");
       return;
     }
 
     // Send the request.
     let request = {
-      method: "POST",
-      path: "/batch",
       body: {
         defaults: {
           method: "DELETE",
         },
         requests: requests,
       },
     };
-    let batchResponse = yield this._sendRequest(request);
+    let batchResponse = yield this._postBatch(request);
     if (batchResponse.status != 200) {
       this._handleUnexpectedResponse("uploading deleted items", batchResponse);
       return;
     }
 
     // Delete local items based on the response.
     for (let response of batchResponse.body.responses) {
       // A 404 means the item was already deleted on the server, which is OK.
@@ -499,16 +497,60 @@ SyncImpl.prototype = {
    */
   _sendRequest: Task.async(function* (req) {
     log.debug("Sending request", req);
     let response = yield this._client.request(req);
     log.debug("Received response", response);
     return response;
   }),
 
+  /**
+   * The server limits the number of sub-requests in POST /batch'es to
+   * BATCH_REQUEST_LIMIT.  This method takes an arbitrarily big batch request
+   * and breaks it apart into many individual batch requests in order to stay
+   * within the limit.
+   *
+   * @param bigRequest The same type of request object that _sendRequest takes.
+   *        Since it's a POST /batch request, its `body` should have a
+   *        `requests` property whose value is an array of sub-requests.
+   *        `method` and `path` are automatically filled.
+   * @return Promise<response> Resolved when all requests complete with 200s, or
+   *         when the first response that is not a 200 is received.  In the
+   *         first case, the resolved response is a combination of all the
+   *         server responses, and response.body.responses contains the sub-
+   *         responses for all the sub-requests in bigRequest.  In the second
+   *         case, the resolved response is the non-200 response straight from
+   *         the server.
+   */
+  _postBatch: Task.async(function* (bigRequest) {
+    log.debug("Sending batch requests");
+    let allSubResponses = [];
+    let remainingSubRequests = bigRequest.body.requests;
+    while (remainingSubRequests.length) {
+      let request = Object.assign({}, bigRequest);
+      request.method = "POST";
+      request.path = "/batch";
+      request.body.requests =
+        remainingSubRequests.splice(0, BATCH_REQUEST_LIMIT);
+      let response = yield this._sendRequest(request);
+      if (response.status != 200) {
+        return response;
+      }
+      allSubResponses = allSubResponses.concat(response.body.responses);
+    }
+    let bigResponse = {
+      status: 200,
+      body: {
+        responses: allSubResponses,
+      },
+    };
+    log.debug("All batch requests successfully sent");
+    return bigResponse;
+  }),
+
   _handleUnexpectedResponse(contextMsgFragment, response) {
     log.error(`Unexpected response ${contextMsgFragment}`, response);
   },
 
   // TODO: Wipe this pref when user logs out.
   get _serverLastModifiedHeader() {
     if (!("__serverLastModifiedHeader" in this)) {
       this.__serverLastModifiedHeader =