[GitHub] cbickel closed pull request #3836: Use a PoolingConnectionManager even for single connection use-cases.

[GitBox <gi...@apache.org>]

cbickel closed pull request #3836: Use a PoolingConnectionManager even for single connection use-cases.
URL: https://github.com/apache/incubator-openwhisk/pull/3836
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/common/scala/src/main/scala/whisk/core/containerpool/HttpUtils.scala b/common/scala/src/main/scala/whisk/core/containerpool/HttpUtils.scala
index 4ff205d7e6..c5faab4d43 100644
--- a/common/scala/src/main/scala/whisk/core/containerpool/HttpUtils.scala
+++ b/common/scala/src/main/scala/whisk/core/containerpool/HttpUtils.scala
@@ -38,7 +38,7 @@ import org.apache.http.client.utils.{HttpClientUtils, URIBuilder}
 import org.apache.http.conn.HttpHostConnectException
 import org.apache.http.entity.StringEntity
 import org.apache.http.impl.client.HttpClientBuilder
-import org.apache.http.impl.conn.{BasicHttpClientConnectionManager, PoolingHttpClientConnectionManager}
+import org.apache.http.impl.conn.PoolingHttpClientConnectionManager
 import spray.json._
 import whisk.common.Logging
 import whisk.common.TransactionId
@@ -62,6 +62,12 @@ import whisk.core.entity.size.SizeLong
 protected class HttpUtils(hostname: String, timeout: FiniteDuration, maxResponse: ByteSize, maxConcurrent: Int = 1)(
   implicit logging: Logging) {
 
+  /**
+   * Closes the HttpClient and all resources allocated by it.
+   *
+   * This will close the HttpClient that is generated for this instance of HttpUtils. That will also cause the
+   * ConnectionManager to be closed alongside.
+   */
   def close() = Try(connection.close())
 
   /**
@@ -159,15 +165,21 @@ protected class HttpUtils(hostname: String, timeout: FiniteDuration, maxResponse
 
   private val connection = HttpClientBuilder.create
     .setDefaultRequestConfig(httpconfig)
-    .setConnectionManager(if (maxConcurrent > 1) {
-      // Use PoolingHttpClientConnectionManager so that concurrent activation processing (if enabled) will reuse connections
-      val cm = new PoolingHttpClientConnectionManager
-      // Increase default max connections per route (default is 2)
+    .setConnectionManager({
+      // A PoolingHttpClientConnectionManager is the default when not specifying any ConnectionManager.
+      // The PoolingHttpClientConnectionManager has the benefit of actively checking if a connection has become stale,
+      // which is very important because pausing/resuming containers can cause a connection to become silently broken.
+      // This causes very subtle bugs, especially when containers are reused after a pretty long time (like > 5 minutes).
+      //
+      // The BasicHttpClientConnectionManager (which would be alternative here) doesn't have such a mechanism and thus
+      // isn't suitable for our usage.
+      val cm = new PoolingHttpClientConnectionManager()
+      // perRoute effectively means per host in our use-case, which means setting it to the same value as the maximum
+      // total of all connections in the pool is appropriate here.
       cm.setDefaultMaxPerRoute(maxConcurrent)
-      // Increase max total connections (default is 20)
       cm.setMaxTotal(maxConcurrent)
       cm
-    } else new BasicHttpClientConnectionManager()) // set the Pooling connection manager IFF maxConcurrent > 1
+    })
     .useSystemProperties()
     .disableAutomaticRetries()
     .build


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services