Bug 1024015 - Only enable HTTP response timeout when TCP Keepalives are disabled for HTTP r=mcmanus

modules/libpref/src/init/all.js

@@ -988,7 +988,7 @@ pref("network.http.default-socket-type", "");
 // the packet is lost or delayed on the route.
 pref("network.http.keep-alive.timeout", 115);
 
-// Timeout connections if an initial response is not received after 10 mins.
+// Timeout connections if an initial response is not received after 5 mins.
 pref("network.http.response.timeout", 300);
 
 // Limit the absolute number of http connections.

netwerk/protocol/http/nsHttpConnection.cpp

@@ -417,7 +417,8 @@ nsHttpConnection::Activate(nsAHttpTransaction *trans, uint32_t caps, int32_t pri
     // The overflow state is not needed between activations
     mInputOverflow = nullptr;
 
-    mResponseTimeoutEnabled = mTransaction->ResponseTimeout() > 0 &&
+    mResponseTimeoutEnabled = gHttpHandler->ResponseTimeoutEnabled() &&
+                              mTransaction->ResponseTimeout() > 0 &&
                               mTransaction->ResponseTimeoutEnabled();
 
     rv = StartShortLivedTCPKeepalives();
@@ -1110,6 +1111,9 @@ nsHttpConnection::ReadTimeoutTick(PRIntervalTime now)
     uint32_t nextTickAfter = UINT32_MAX;
     // Timeout if the response is taking too long to arrive.
     if (mResponseTimeoutEnabled) {
+        NS_WARN_IF_FALSE(gHttpHandler->ResponseTimeoutEnabled(),
+                         "Timing out a response, but response timeout is disabled!");
+
         PRIntervalTime initialResponseDelta = now - mLastWriteTime;
 
         if (initialResponseDelta > mTransaction->ResponseTimeout()) {

netwerk/protocol/http/nsHttpHandler.cpp

@@ -142,7 +142,8 @@ nsHttpHandler::nsHttpHandler()
     , mProxyPipelining(true)
     , mIdleTimeout(PR_SecondsToInterval(10))
     , mSpdyTimeout(PR_SecondsToInterval(180))
-    , mResponseTimeout(PR_SecondsToInterval(600))
+    , mResponseTimeout(PR_SecondsToInterval(300))
+    , mResponseTimeoutEnabled(false)
     , mMaxRequestAttempts(10)
     , mMaxRequestDelay(10)
     , mIdleSynTimeout(250)
@@ -1455,6 +1456,10 @@ nsHttpHandler::PrefsChanged(nsIPrefBranch *prefs, const char *pref)
                                                       1, kMaxTCPKeepIdle);
     }
 
+    // Enable HTTP response timeout if TCP Keepalives are disabled.
+    mResponseTimeoutEnabled = !mTCPKeepaliveShortLivedEnabled &&
+                              !mTCPKeepaliveLongLivedEnabled;
+
 #undef PREF_CHANGED
 #undef MULTI_PREF_CHANGED
 }

netwerk/protocol/http/nsHttpHandler.h

@@ -76,7 +76,10 @@ public:
     uint8_t        RedirectionLimit()        { return mRedirectionLimit; }
     PRIntervalTime IdleTimeout()             { return mIdleTimeout; }
     PRIntervalTime SpdyTimeout()             { return mSpdyTimeout; }
-    PRIntervalTime ResponseTimeout()         { return mResponseTimeout; }
+    PRIntervalTime ResponseTimeout() {
+      return mResponseTimeoutEnabled ? mResponseTimeout : 0;
+    }
+    PRIntervalTime ResponseTimeoutEnabled()  { return mResponseTimeoutEnabled; }
     uint16_t       MaxRequestAttempts()      { return mMaxRequestAttempts; }
     const char    *DefaultSocketType()       { return mDefaultSocketType.get(); /* ok to return null */ }
     uint32_t       PhishyUserPassLength()    { return mPhishyUserPassLength; }
@@ -362,6 +365,7 @@ private:
     PRIntervalTime mIdleTimeout;
     PRIntervalTime mSpdyTimeout;
     PRIntervalTime mResponseTimeout;
+    bool mResponseTimeoutEnabled;
 
     uint16_t mMaxRequestAttempts;
     uint16_t mMaxRequestDelay;

netwerk/test/unit/test_httpResponseTimeout.js

@@ -1,17 +1,28 @@
+/* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
 Cu.import("resource://testing-common/httpd.js");
 
 var baseURL;
 const kResponseTimeoutPref = "network.http.response.timeout";
 const kResponseTimeout = 1;
+const kShortLivedKeepalivePref =
+  "network.http.tcp_keepalive.short_lived_connections";
+const kLongLivedKeepalivePref =
+  "network.http.tcp_keepalive.long_lived_connections";
 
 const prefService = Cc["@mozilla.org/preferences-service;1"]
                        .getService(Ci.nsIPrefBranch);
 
 var server = new HttpServer();
 
-function TimeoutListener(enableTimeout, nextTest) {
-  this.enableTimeout = enableTimeout;
-  this.nextTest = nextTest;
+function TimeoutListener(expectResponse) {
+  this.expectResponse = expectResponse;
 }
 
 TimeoutListener.prototype = {
@@ -22,17 +33,13 @@ TimeoutListener.prototype = {
   },
 
   onStopRequest: function (request, ctx, status) {
-    if (this.enableTimeout) {
-      do_check_eq(status, Cr.NS_ERROR_NET_TIMEOUT);
-    } else {
+    if (this.expectResponse) {
       do_check_eq(status, Cr.NS_OK);
+    } else {
+      do_check_eq(status, Cr.NS_ERROR_NET_TIMEOUT);
     }
 
-    if (this.nextTest) {
-      this.nextTest();
-    } else {
-      server.stop(serverStopListener);
-    }
+    run_next_test();
   },
 };
 
@@ -40,7 +47,7 @@ function serverStopListener() {
   do_test_finished();
 }
 
-function testTimeout(timeoutEnabled, nextTest) {
+function testTimeout(timeoutEnabled, expectResponse) {
   // Set timeout pref.
   if (timeoutEnabled) {
     prefService.setIntPref(kResponseTimeoutPref, kResponseTimeout);
@@ -52,20 +59,83 @@ function testTimeout(timeoutEnabled, nextTest) {
   .getService(Ci.nsIIOService);
   var chan = ios.newChannel(baseURL, null, null)
   .QueryInterface(Ci.nsIHttpChannel);
-  var listener = new TimeoutListener(timeoutEnabled, nextTest);
+  var listener = new TimeoutListener(expectResponse);
   chan.asyncOpen(listener, null);
 }
 
 function testTimeoutEnabled() {
-  testTimeout(true, testTimeoutDisabled);
+  // Set a timeout value; expect a timeout and no response.
+  testTimeout(true, false);
 }
 
 function testTimeoutDisabled() {
-  testTimeout(false, null);
+  // Set a timeout value of 0; expect a response.
+  testTimeout(false, true);
 }
 
-function run_test() {
-  // Start server; will be stopped after second test.
+function testTimeoutDisabledByShortLivedKeepalives() {
+  // Enable TCP Keepalives for short lived HTTP connections.
+  prefService.setBoolPref(kShortLivedKeepalivePref, true);
+  prefService.setBoolPref(kLongLivedKeepalivePref, false);
+
+  // Try to set a timeout value, but expect a response without timeout.
+  testTimeout(true, true);
+}
+
+function testTimeoutDisabledByLongLivedKeepalives() {
+  // Enable TCP Keepalives for long lived HTTP connections.
+  prefService.setBoolPref(kShortLivedKeepalivePref, false);
+  prefService.setBoolPref(kLongLivedKeepalivePref, true);
+
+  // Try to set a timeout value, but expect a response without timeout.
+  testTimeout(true, true);
+}
+
+function testTimeoutDisabledByBothKeepalives() {
+  // Enable TCP Keepalives for short and long lived HTTP connections.
+  prefService.setBoolPref(kShortLivedKeepalivePref, true);
+  prefService.setBoolPref(kLongLivedKeepalivePref, true);
+
+  // Try to set a timeout value, but expect a response without timeout.
+  testTimeout(true, true);
+}
+
+function setup_tests() {
+  // Start tests with timeout enabled, i.e. disable TCP keepalives for HTTP.
+  // Reset pref in cleanup.
+  if (prefService.getBoolPref(kShortLivedKeepalivePref)) {
+    prefService.setBoolPref(kShortLivedKeepalivePref, false);
+    do_register_cleanup(function() {
+      prefService.setBoolPref(kShortLivedKeepalivePref, true);
+    });
+  }
+  if (prefService.getBoolPref(kLongLivedKeepalivePref)) {
+    prefService.setBoolPref(kLongLivedKeepalivePref, false);
+    do_register_cleanup(function() {
+      prefService.setBoolPref(kLongLivedKeepalivePref, true);
+    });
+  }
+
+  var tests = [
+    // Enable with a timeout value >0;
+    testTimeoutEnabled,
+    // Disable with a timeout value of 0;
+    testTimeoutDisabled,
+    // Disable by enabling TCP keepalive for short-lived HTTP connections.
+    testTimeoutDisabledByShortLivedKeepalives,
+    // Disable by enabling TCP keepalive for long-lived HTTP connections.
+    testTimeoutDisabledByLongLivedKeepalives,
+    // Disable by enabling TCP keepalive for both HTTP connection types.
+    testTimeoutDisabledByBothKeepalives
+  ];
+
+  for (var i=0; i < tests.length; i++) {
+    add_test(tests[i]);
+  }
+}
+
+function setup_http_server() {
+  // Start server; will be stopped at test cleanup time.
   server.start(-1);
   baseURL = "http://localhost:" + server.identity.primaryPort + "/";
   do_print("Using baseURL: " + baseURL);
@@ -79,11 +149,15 @@ function run_test() {
       response.finish();
     });
   });
-
-  // First test checks timeout enabled.
-  // Second test (in callback) checks timeout disabled.
-  testTimeoutEnabled();
-
-  do_test_pending();
+  do_register_cleanup(function() {
+    server.stop(serverStopListener);
+  });
 }
 
+function run_test() {
+  setup_http_server();
+
+  setup_tests();
+
+  run_next_test();
+}