launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/longpoll-408-errors into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/launchpad/longpoll-408-errors/+merge/77979

This branch fixes the LongPoll js library so that when a request timeouts (408), the error is simply swallowed and another connection is launched.

= Tests =

lib/lp/app/longpoll/javascript/tests/test_longpoll.html
-- 
https://code.launchpad.net/~rvb/launchpad/longpoll-408-errors/+merge/77979
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/longpoll-408-errors into lp:launchpad.

=== modified file 'lib/lp/app/longpoll/javascript/longpoll.js'
--- lib/lp/app/longpoll/javascript/longpoll.js	2011-09-28 08:08:37 +0000
+++ lib/lp/app/longpoll/javascript/longpoll.js	2011-10-03 16:56:30 +0000
@@ -129,8 +129,10 @@
      * @method repoll
      * @param {Boolean} failed: whether or not the previous connection
      *     has failed.
-     */
-    repoll : function(failed) {
+     * @param {Boolean} incr_sequence: whether or not the sequence should
+     *     be incremented.
+      */
+    repoll : function(failed, incr_sequence) {
         if (!failed) {
             if (this._failed_attempts >=
                     namespace.MAX_SHORT_DELAY_FAILED_ATTEMPTS) {
@@ -138,34 +140,45 @@
             }
             this._failed_attempts = 0;
             if (this._repoll) {
-                this.poll();
+                this.poll(incr_sequence);
             }
         }
         else {
             var delay = this._pollDelay();
             if (this._repoll) {
-                Y.later(delay, this, this.poll);
+                Y.later(delay, this, this.poll, [incr_sequence]);
             }
         }
     },
 
-    poll : function() {
+    poll : function(incr_sequence) {
         var that = this;
         var config = {
             method: "GET",
             sync: false,
             on: {
-                failure: function() {
-                    that.failurePoll();
-                    that.repoll(true);
+                failure: function(id, response) {
+                    if (Y.Lang.isValue(response) &&
+                        Y.Lang.isValue(response.status) &&
+                        response.status === 408) {
+                        // The error is a "Request timeout": start the
+                        // polling again without incrementing the sequence.
+                        that.repoll(false, false);
+                    }
+                    else {
+                        that.failurePoll();
+                        that.repoll(true, true);
+                    }
                 },
                 success: function(id, response) {
                     var res = that.successPoll(id, response);
-                    that.repoll(res);
+                    that.repoll(res, true);
                 }
             }
         };
-        this._sequence = this._sequence + 1;
+        if (incr_sequence) {
+            this._sequence = this._sequence + 1;
+        }
         var queue_uri = this.uri +
             "?uuid=" + this.key +
             "&sequence=" + this._sequence;
@@ -192,7 +205,7 @@
         var uri = LP.cache.longpoll.uri;
         var longpollmanager = namespace.getLongPollManager();
         longpollmanager.setConnectionInfos(key, uri);
-        longpollmanager.poll();
+        longpollmanager.poll(true);
         return longpollmanager;
     }
 };

=== modified file 'lib/lp/app/longpoll/javascript/tests/test_longpoll.js'
--- lib/lp/app/longpoll/javascript/tests/test_longpoll.js	2011-09-27 21:27:57 +0000
+++ lib/lp/app/longpoll/javascript/tests/test_longpoll.js	2011-10-03 16:56:30 +0000
@@ -182,12 +182,12 @@
             };
             // After a success, longpoll.longpoll_shortdelay is fired.
             Y.Assert.isFalse(shortdelay_event_fired);
-            delay = manager.poll();
+            delay = manager.poll(true);
             Y.Assert.isTrue(shortdelay_event_fired);
-           },
+        },
 
         testPollUriSequence: function() {
-            // Each new polling increses the sequence parameter:
+            // Each new polling increases the sequence parameter:
             // /+longpoll/?uuid=key&sequence=1
             // /+longpoll/?uuid=key&sequence=2
             // /+longpoll/?uuid=key&sequence=3
@@ -206,7 +206,23 @@
                 config.on.success(2, response);
             };
             this.setupLongPoll();
-            Y.Assert.isTrue(count === 1, "Uri not requested.");
+            var request;
+            for (request=1; request<10; request++) {
+                Y.Assert.isTrue(count === request, "Uri not requested.");
+                manager.poll(true);
+            }
+        },
+
+        testRequestTimeoutHandling: function() {
+            var manager = longpoll.getLongPollManager();
+            // Monkeypatch io to simulate a request timeout.
+            manager._io = function(uri, config) {
+                response = {status: 408};
+                config.on.failure(4, response);
+            };
+            Y.Assert.areEqual(0, manager._failed_attempts);
+            this.setupLongPoll();
+            Y.Assert.areEqual(0, manager._failed_attempts);
         },
 
         testPollPayLoadBad: function() {