You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by overquota <gi...@git.apache.org> on 2018/05/11 10:24:40 UTC
[GitHub] httpcomponents-client pull request #103: fixed incorrect connection close on...
GitHub user overquota opened a pull request:
https://github.com/apache/httpcomponents-client/pull/103
fixed incorrect connection close on shutdown + fixed corresponding test
due to https://issues.apache.org/jira/browse/HTTPCLIENT-1923
close() method was changed due to Oleg@ suggestion
But thats was not enough, because it isnt called at all when connection manager is shutting down [entering in connection manager shutdown on client close](https://github.com/apache/httpcomponents-client/blob/4.5.x/httpclient/src/main/java/org/apache/http/impl/client/HttpClientBuilder.java#L1233) -> [shutdown method called](https://github.com/apache/httpcomponents-client/blob/4.5.x/httpclient/src/main/java/org/apache/http/impl/conn/BasicHttpClientConnectionManager.java#L376)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/overquota/httpcomponents-client 4.5.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/httpcomponents-client/pull/103.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 #103
----
commit ebada8ff73cc70053a0462dd549ffcd687f3e6ee
Author: Aleksei Arsenev <aa...@...>
Date: 2018-05-11T10:14:47Z
fixed incorrect connection close on shutdown + fixed corresponding test
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #103: fixed incorrect connection close on...
Posted by overquota <gi...@git.apache.org>.
Github user overquota closed the pull request at:
https://github.com/apache/httpcomponents-client/pull/103
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #103: fixed incorrect connection close on...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/103#discussion_r187589736
--- Diff: httpclient/src/main/java/org/apache/http/impl/conn/BasicHttpClientConnectionManager.java ---
@@ -375,7 +375,7 @@ public synchronized void closeIdleConnections(final long idletime, final TimeUni
@Override
public synchronized void shutdown() {
if (this.isShutdown.compareAndSet(false, true)) {
- shutdownConnection();
--- End diff --
@overquota Should not `#close` do `#closeConnection` and `#shutdown` do `#shutdownConnection`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #103: fixed incorrect connection close on...
Posted by overquota <gi...@git.apache.org>.
Github user overquota commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/103#discussion_r187594314
--- Diff: httpclient/src/main/java/org/apache/http/impl/conn/BasicHttpClientConnectionManager.java ---
@@ -375,7 +375,7 @@ public synchronized void closeIdleConnections(final long idletime, final TimeUni
@Override
public synchronized void shutdown() {
if (this.isShutdown.compareAndSet(false, true)) {
- shutdownConnection();
--- End diff --
I think no because `#shutdownConnection` closes connection with `RST` (https://github.com/apache/httpcomponents-core/blob/4.4.x/httpcore/src/main/java/org/apache/http/impl/BHttpConnectionBase.java#L304) - this is not normal tcp connection closing
While `#closeConnection` doing it more correctly (FIN+ACK): https://github.com/apache/httpcomponents-core/blob/4.4.x/httpcore/src/main/java/org/apache/http/impl/BHttpConnectionBase.java#L315
`#shutdown` may do `#shutdownConnection` only if `#shutdownConnection` will do close() on connection instance i think... but if so - i think there are no difference what shutdown method will do close instead of shutdown =)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #103: fixed incorrect connection close on...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/103#discussion_r187595171
--- Diff: httpclient/src/main/java/org/apache/http/impl/conn/BasicHttpClientConnectionManager.java ---
@@ -375,7 +375,7 @@ public synchronized void closeIdleConnections(final long idletime, final TimeUni
@Override
public synchronized void shutdown() {
if (this.isShutdown.compareAndSet(false, true)) {
- shutdownConnection();
--- End diff --
@overquota I am not sure I agree. `#shutdown` is supposed to be used to execute an abnormal connection termination (with `RST`), while `#close` is to be used for orderly connection termination (with `FIN+ACK`).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #103: fixed incorrect connection close on...
Posted by overquota <gi...@git.apache.org>.
Github user overquota commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/103#discussion_r187599165
--- Diff: httpclient/src/main/java/org/apache/http/impl/conn/BasicHttpClientConnectionManager.java ---
@@ -375,7 +375,7 @@ public synchronized void closeIdleConnections(final long idletime, final TimeUni
@Override
public synchronized void shutdown() {
if (this.isShutdown.compareAndSet(false, true)) {
- shutdownConnection();
--- End diff --
just for example:
`#shutdown` in [PoolingHttpClientConnectionManager](https://github.com/apache/httpcomponents-client/blob/4.5.x/httpclient/src/main/java/org/apache/http/impl/conn/PoolingHttpClientConnectionManager.java#L410) really do `this.pool.shutdown();`
*BUT*
`this.pool.shutdown` doing [entry.close](https://github.com/apache/httpcomponents-core/blob/4.4.x/httpcore/src/main/java/org/apache/http/pool/AbstractConnPool.java#L148) - which is the same normal `conn.close()`
In addition: [HttpClientBuilder](https://github.com/apache/httpcomponents-client/blob/4.5.x/httpclient/src/main/java/org/apache/http/impl/client/HttpClientBuilder.java#L1232) knows only about `#close`, and on close it is doing... `#shutdown` :) - thats the main reason i've changed `#shutdown` method
In addition 2: [ClientConnectionManager declaration](https://github.com/apache/httpcomponents-client/blob/4.5.x/httpclient/src/main/java/org/apache/http/conn/ClientConnectionManager.java#L112) does not know anything about `#close`, only `#shutdown`. And there are no docs about how connections would be terminated
Do you want me to refactor all these misleadings? No problems, really, but I don't know is it worth it?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client issue #103: fixed incorrect connection close on shutdo...
Posted by overquota <gi...@git.apache.org>.
Github user overquota commented on the issue:
https://github.com/apache/httpcomponents-client/pull/103
Thank you very much! Is there any deadline for publishing version with this fix in maven repo?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #103: fixed incorrect connection close on...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/103#discussion_r187599055
--- Diff: httpclient/src/main/java/org/apache/http/impl/conn/BasicHttpClientConnectionManager.java ---
@@ -375,7 +375,7 @@ public synchronized void closeIdleConnections(final long idletime, final TimeUni
@Override
public synchronized void shutdown() {
if (this.isShutdown.compareAndSet(false, true)) {
- shutdownConnection();
--- End diff --
@overquota Actually I just realized that the pooling connection manager does not distinguish between normal or abnormal pool shutdown. Persistent connections get closed out regardless whether the manager gets closed or shut down. For consistency sake I'll make the basic manager behave the same.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client issue #103: fixed incorrect connection close on shutdo...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-client/pull/103
Q2/Q3 2018
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client issue #103: fixed incorrect connection close on shutdo...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-client/pull/103
@overquota Committed to 4.5.x and 4.6.x with some minor tweaks. Please review and close the PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org