You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2023/11/04 18:42:43 UTC
[PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
arturobernalg opened a new pull request, #502:
URL: https://github.com/apache/httpcomponents-client/pull/502
This pull request addresses an issue in `BasicHttpClientConnectionManager` where stale connections were not being handled correctly. Previously, a stale connection could potentially be reused, leading to unexpected behavior or errors. The changes ensure that any stale connection is closed and a new connection is created whenever `getConnection` is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502#discussion_r1382593572
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java:
##########
@@ -357,7 +357,19 @@ ManagedHttpClientConnection getConnection(final HttpRoute route, final Object st
this.conn = this.connFactory.createConnection(null);
this.created = System.currentTimeMillis();
} else {
- this.conn.activate();
+ // Check if the existing connection is stale
+ if (this.conn.isStale()) {
Review Comment:
@arturobernalg Now I am confused. The `#validate` method performs the check only if the period of inactivity exceeds `ValidateAfterInactivity` setting. This is intentional and by design. One does not want to execute the stale check too often due to its cost and impact on performance. The change you are proposing seems to be breaking that. Is that really what you want? If one wants the stale check done every time one can set `ValidateAfterInactivity` to -1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502#discussion_r1388240441
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java:
##########
@@ -390,22 +391,15 @@ public void release(final ConnectionEndpoint endpoint, final Object state, final
return;
}
try {
- if (keepAlive == null) {
- this.conn.close(CloseMode.GRACEFUL);
+ if (keepAlive == null && conn != null) {
+ conn.close(CloseMode.GRACEFUL);
}
this.updated = System.currentTimeMillis();
- if (!this.conn.isOpen() && !this.conn.isConsistent()) {
- this.route = null;
- this.conn = null;
- this.expiry = Long.MAX_VALUE;
- if (LOG.isDebugEnabled()) {
- LOG.debug("{} Connection is not kept alive", id);
- }
- } else {
+ final boolean reusable = conn != null && conn.isOpen() && conn.isConsistent();
Review Comment:
changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg merged PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502#discussion_r1386181346
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java:
##########
@@ -390,22 +391,15 @@ public void release(final ConnectionEndpoint endpoint, final Object state, final
return;
}
try {
- if (keepAlive == null) {
- this.conn.close(CloseMode.GRACEFUL);
+ if (keepAlive == null && conn != null) {
+ conn.close(CloseMode.GRACEFUL);
}
this.updated = System.currentTimeMillis();
- if (!this.conn.isOpen() && !this.conn.isConsistent()) {
- this.route = null;
- this.conn = null;
- this.expiry = Long.MAX_VALUE;
- if (LOG.isDebugEnabled()) {
- LOG.debug("{} Connection is not kept alive", id);
- }
- } else {
+ final boolean reusable = conn != null && conn.isOpen() && conn.isConsistent();
Review Comment:
@arturobernalg The variable seems redundant. Looks OK otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502#discussion_r1385636960
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java:
##########
@@ -357,7 +357,19 @@ ManagedHttpClientConnection getConnection(final HttpRoute route, final Object st
this.conn = this.connFactory.createConnection(null);
this.created = System.currentTimeMillis();
} else {
- this.conn.activate();
+ // Check if the existing connection is stale
+ if (this.conn.isStale()) {
Review Comment:
Upon reviewing your comment, I realize that I overlooked the intentional design behind the validate method's frequency of execution I've committed a new potential fix this. I have now submitted a new commit that aims to rectify this. The updated code ensures that the connection being released is indeed the one that was detached from the `InternalConnectionEndpoint`. This should maintain the intended behavior while addressing the issue at hand.
While I have conducted extensive testing to validate this fix, I must admit that connection management is not my primary area of expertise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502#discussion_r1382565133
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java:
##########
@@ -357,7 +357,19 @@ ManagedHttpClientConnection getConnection(final HttpRoute route, final Object st
this.conn = this.connFactory.createConnection(null);
this.created = System.currentTimeMillis();
} else {
- this.conn.activate();
+ // Check if the existing connection is stale
+ if (this.conn.isStale()) {
Review Comment:
> Should not the connection have already been checked for staleness in `#validate` line 355? Why doing again? Am I missing something?
@ok2c
The staleness check was only being performed when the deadline had expired. In our case, the deadline wasn't expired, so the connection's staleness wasn't being evaluated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502#discussion_r1382557118
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java:
##########
@@ -357,7 +357,19 @@ ManagedHttpClientConnection getConnection(final HttpRoute route, final Object st
this.conn = this.connFactory.createConnection(null);
this.created = System.currentTimeMillis();
} else {
- this.conn.activate();
+ // Check if the existing connection is stale
+ if (this.conn.isStale()) {
Review Comment:
@arturobernalg Should not the connection have already been checked for staleness in `#validate` line 355? Why doing again? Am I missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2301. Fix stale connection handling in BasicHttpClientConnectionManager [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #502:
URL: https://github.com/apache/httpcomponents-client/pull/502#discussion_r1386178639
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java:
##########
@@ -357,7 +357,19 @@ ManagedHttpClientConnection getConnection(final HttpRoute route, final Object st
this.conn = this.connFactory.createConnection(null);
this.created = System.currentTimeMillis();
} else {
- this.conn.activate();
+ // Check if the existing connection is stale
+ if (this.conn.isStale()) {
Review Comment:
@arturobernalg There is no problem of what so ever. That is why we have reviews
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org