You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jdpgrailsdev <gi...@git.apache.org> on 2017/03/11 17:52:34 UTC
[GitHub] thrift pull request #1212: Release managed HTTP connection back to the under...
GitHub user jdpgrailsdev opened a pull request:
https://github.com/apache/thrift/pull/1212
Release managed HTTP connection back to the underlying pool
There is a connection leak in the `THttpClient` when using the Apache `HttpClient` with the `PoolingClientConnectionManager`. Without calling `releaseConnection` on the `HttpPost` object, 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:
> In order to ensure correct deallocation of system resources
> the user MUST either fully consume the response content or abort request
> execution by calling HttpGet#releaseConnection().
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jdpgrailsdev/thrift fix-http-connection-leak
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1212.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1212
----
commit ca172da92e275809859f9dbfdea831c5dbade119
Author: Jonathan Pearlin <jp...@newrelic.com>
Date: 2017-03-11T17:46:03Z
Release connection back to managed pool.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1212: Release managed HTTP connection back to the underlying p...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1212
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!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1212: Release managed HTTP connection back to the underlying p...
Posted by jdpgrailsdev <gi...@git.apache.org>.
Github user jdpgrailsdev commented on the issue:
https://github.com/apache/thrift/pull/1212
@jeking3 Sure thing. Will update this PR with the link once I have it created.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1212: [THRIFT-4130] Release managed HTTP connection bac...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1212
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1212: Release managed HTTP connection back to the underlying p...
Posted by jdpgrailsdev <gi...@git.apache.org>.
Github user jdpgrailsdev commented on the issue:
https://github.com/apache/thrift/pull/1212
@jeking3 https://issues.apache.org/jira/browse/THRIFT-4130
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1212: Release managed HTTP connection back to the under...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1212#discussion_r106642062
--- Diff: lib/java/src/org/apache/thrift/transport/THttpClient.java ---
@@ -304,6 +304,9 @@ private void flushUsingHttpClient() throws TTransportException {
throw new TTransportException(ioe);
}
}
+ if (post != null) {
+ post.releaseConnection();
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1212: Release managed HTTP connection back to the under...
Posted by jdpgrailsdev <gi...@git.apache.org>.
Github user jdpgrailsdev commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1212#discussion_r106723251
--- Diff: lib/java/src/org/apache/thrift/transport/THttpClient.java ---
@@ -304,6 +304,9 @@ private void flushUsingHttpClient() throws TTransportException {
throw new TTransportException(ioe);
}
}
+ if (post != null) {
+ post.releaseConnection();
--- End diff --
@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`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---