[incubator-openwhisk] branch master updated: Make HttpUtils actually use only one connection and harden connection release. (#3818)

[du...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

dubeejw pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-openwhisk.git


The following commit(s) were added to refs/heads/master by this push:
     new cd91b4f  Make HttpUtils actually use only one connection and harden connection release. (#3818)
cd91b4f is described below

commit cd91b4fc024b16b9cc89b1aef77deb494da939e2
Author: Markus Thömmes <ma...@me.com>
AuthorDate: Mon Jul 2 16:31:38 2018 +0200

    Make HttpUtils actually use only one connection and harden connection release. (#3818)
    
    When not specifying any ConnectionManager for the HttpClient used by HttpUtils, it defaults to an arbitrary PoolingManager with 5 connections. This can hide subtle bugs in connection management, since we expect it to only have one connection to the specific container, we'll default to the BasicConnectionManager.
    
    Furthermore, there are subtle issues when an entity returned by a HttpRequest is not consumed fully (which could happen if it gets truncated or content-length is not returned correctly). That can cause the ConnectionPool (which shouldn't be there in the first place) to go stale.
    
    We also never closed the entity's stream.
---
 .../src/main/scala/whisk/core/containerpool/HttpUtils.scala    | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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 d9bdbdc..4ff205d 100644
--- a/common/scala/src/main/scala/whisk/core/containerpool/HttpUtils.scala
+++ b/common/scala/src/main/scala/whisk/core/containerpool/HttpUtils.scala
@@ -29,17 +29,16 @@ import scala.util.control.NoStackTrace
 import scala.util.Failure
 import scala.util.Success
 import scala.util.Try
-
 import org.apache.commons.io.IOUtils
 import org.apache.http.HttpHeaders
 import org.apache.http.client.config.RequestConfig
 import org.apache.http.client.methods.HttpPost
 import org.apache.http.client.methods.HttpRequestBase
-import org.apache.http.client.utils.URIBuilder
+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.PoolingHttpClientConnectionManager
+import org.apache.http.impl.conn.{BasicHttpClientConnectionManager, PoolingHttpClientConnectionManager}
 import spray.json._
 import whisk.common.Logging
 import whisk.common.TransactionId
@@ -116,7 +115,8 @@ protected class HttpUtils(hostname: String, timeout: FiniteDuration, maxResponse
           Left(NoResponseReceived())
         }
 
-      response.close()
+      // Fully consumes the entity and closes the response
+      HttpClientUtils.closeQuietly(response)
       containerResponse
     } recoverWith {
       // The route to target socket as well as the target socket itself may need some time to be available -
@@ -167,7 +167,7 @@ protected class HttpUtils(hostname: String, timeout: FiniteDuration, maxResponse
       // Increase max total connections (default is 20)
       cm.setMaxTotal(maxConcurrent)
       cm
-    } else null) //set the Pooling connection manager IFF maxConcurrent > 1
+    } else new BasicHttpClientConnectionManager()) // set the Pooling connection manager IFF maxConcurrent > 1
     .useSystemProperties()
     .disableAutomaticRetries()
     .build