[THRIFT-4130] Release managed HTTP connection back to the underlying pool #1212
Conversation
| @@ -304,6 +304,9 @@ private void flushUsingHttpClient() throws TTransportException { | |||
| throw new TTransportException(ioe); | |||
| } | |||
| } | |||
| if (post != null) { | |||
| post.releaseConnection(); | |||
There was a problem hiding this comment.
The code on line 295 appears to want to take care of this condition. Perhaps other exceptions are occurring, or something else is happening like we're not actually consuming all the data that was sent?
From my read of this method "post" will never be null, so the check here and on line 294 are unnecessary.
I would check into the IOException handler above and see if it needs to be expanded, perhaps.
There was a problem hiding this comment.
@jeking3 From what we have seen in testing (when using the pooled connection manager), the call to releaseConnection has to happen even when the request/response is successful. If you think it makes more sense to move this into the try block, that can be done. Also, I believe the null check is necessary if a) this code stays in finally block and b) if the call to create the HttpPost object fails for some reason (first line in the try).
|
Would you be able to open an Apache Jira ticket against the THRIFT project for this issue? Please see: https://thrift.apache.org/docs/HowToContribute Thanks for helping us improve thrift! |
|
@jeking3 Sure thing. Will update this PR with the link once I have it created. |
jdpgrailsdev
changed the title
Client: C++ This closes apache#1212
There is a connection leak in the
THttpClientwhen using the ApacheHttpClientwith thePoolingClientConnectionManager. Without callingreleaseConnectionon theHttpPostobject, the connections are never returned to the pool. Under heavy load, this can lead to both failures for subsequent calls to be able to get a connection from the pool and connections being held by the underlying OS, eventually resulting in the inability to grab another client port for outgoing connections. Per the Apache HttpClient examples/documentation:This might have not been an issue when using the 3.x version of the HttpClient, but it's definitely an issue in the 4.x line. See https://hc.apache.org/httpcomponents-client-4.2.x/quickstart.html for more details.