launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/launchpad/jslongpoll-504 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #904623 in Launchpad itself: "Longpoll javascript should treat 504 (Gateway errors) the way it treats 408 (Request timeout) errors."
  https://bugs.launchpad.net/launchpad/+bug/904623

For more details, see:
https://code.launchpad.net/~rvb/launchpad/jslongpoll-504/+merge/85813

This branch adds support for 504 errors (Gateway error) to the longpoll javascript client.  These errors should be treated like 408 errors (i.e. a new connection should be started immediately without recording the failure as a 'failed connection attempt').

= Tests =

firefox lib/lp/app/longpoll/javascript/tests/test_longpoll.html

= Q/A =

After 5 failed connection attempts, the delay between each connection is changed from 1s to 3 minutes (the rationale behind this is that we want to avoid hammering the server in case something goes really wrong server side).  Before this change, a 504 error (the error returned by HA-Proxy after (currently) 50s) would be considered a proper connection error and thus the delay between each connection would be raised to 3 minutes after 5 failed connections (http://people.canonical.com/~rvb/failed_attempts.png).  This should not happen with this change as the Gateway error will not be considered an error any more.
-- 
https://code.launchpad.net/~rvb/launchpad/jslongpoll-504/+merge/85813
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/jslongpoll-504 into lp:launchpad.

=== modified file 'lib/lp/app/longpoll/javascript/longpoll.js'
--- lib/lp/app/longpoll/javascript/longpoll.js	2011-10-04 10:39:15 +0000
+++ lib/lp/app/longpoll/javascript/longpoll.js	2011-12-15 09:01:33 +0000
@@ -158,8 +158,12 @@
                 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
+                        (response.status === 408 ||
+                         response.status === 504)) {
+                        // If the error code is:
+                        // - 408 Request timeout
+                        // - 504 Gateway timeout
+                        // Then ignore the error and start
                         // polling again.
                         that.repoll(false);
                     }

=== modified file 'lib/lp/app/longpoll/javascript/tests/test_longpoll.js'
--- lib/lp/app/longpoll/javascript/tests/test_longpoll.js	2011-10-04 10:38:29 +0000
+++ lib/lp/app/longpoll/javascript/tests/test_longpoll.js	2011-12-15 09:01:33 +0000
@@ -213,11 +213,14 @@
             }
         },
 
-        testRequestTimeoutHandling: function() {
+        _testDoesNotFail: function(error_code) {
+            // Assert that, when the longpoll request receives an error
+            // with code error_code, it is not treated as a failed
+            // connection attempt.
             var manager = longpoll.getLongPollManager();
             // Monkeypatch io to simulate a request timeout.
             manager._io = function(uri, config) {
-                response = {status: 408};
+                response = {status: error_code};
                 config.on.failure(4, response);
             };
             Y.Assert.areEqual(0, manager._failed_attempts);
@@ -225,6 +228,15 @@
             Y.Assert.areEqual(0, manager._failed_attempts);
         },
 
+        test408RequestTimeoutHandling: function() {
+            this._testDoesNotFail(408);
+        },
+
+        test504GatewayTimeoutHandling: function() {
+            this._testDoesNotFail(504);
+        },
+
+
         testPollPayLoadBad: function() {
             // If a non valid response is returned, longpoll_fail_event
             // is fired.