You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by GitBox <gi...@apache.org> on 2021/06/20 19:52:07 UTC
[GitHub] [httpcomponents-client] ok2c opened a new pull request #309: Connection lease request cancellation bug
ok2c opened a new pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309
@rschmitt Fundamentally, the core problem here is whose responsibility it is to release a pool entry back to the pool if the lease request simultaneously succeeds and gets cancelled.
This is not a _final_ fix but merely a concept. I still need to figure the back way of dealing with the problem. I also need to fix the same defect in the async connection manager implementation.
What truly baffles me is how come this issue has never been reported with HttpClient 4.5 given it appears to be equally affected.
--
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.
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
[GitHub] [httpcomponents-client] rschmitt commented on a change in pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
rschmitt commented on a change in pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#discussion_r655653290
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -329,7 +331,22 @@ public synchronized ConnectionEndpoint get(
@Override
public boolean cancel() {
- return leaseFuture.cancel(true);
+ if (cancelled.compareAndSet(false, true)) {
+ final boolean result = leaseFuture.cancel(true);
+ if (!result) {
+ // The lease request has already completed successfully and cannot be cancelled
+ try {
+ final PoolEntry<HttpRoute, ManagedHttpClientConnection> poolEntry = leaseFuture.get(1, TimeUnit.MILLISECONDS);
Review comment:
I'd prefer to see `leaseFuture.get(0, TimeUnit.MILLISECONDS)` here, by way of asserting that this call should not block
--
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.
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
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#discussion_r655674228
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -329,7 +331,22 @@ public synchronized ConnectionEndpoint get(
@Override
public boolean cancel() {
- return leaseFuture.cancel(true);
+ if (cancelled.compareAndSet(false, true)) {
+ final boolean result = leaseFuture.cancel(true);
+ if (!result) {
+ // The lease request has already completed successfully and cannot be cancelled
+ try {
+ final PoolEntry<HttpRoute, ManagedHttpClientConnection> poolEntry = leaseFuture.get(1, TimeUnit.MILLISECONDS);
Review comment:
@rschmitt This call should never block given at this point that the future is complete but you are absolutely right. Will do so.
--
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.
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
[GitHub] [httpcomponents-client] ok2c commented on pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#issuecomment-865317031
> This proposal passes all of my regression tests. Would it allow us to also simplify the cancellation checks in classes like `InternalExecRuntime`?
Great. Please allow me a few more day to work on the test coverage and also fix the same defect in the async code. I am also going to review and clean up `InternalExecRuntime` at the same time.
--
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.
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
[GitHub] [httpcomponents-client] carterkozak commented on pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#issuecomment-865238036
déjà vu, I've run into this flavor of leak a number of times because it's incredibly difficult to handle `cancel()` on any `Future<? extends AutoCloseable>`.
> the core problem here is whose responsibility it is to release a pool entry back to the pool if the lease request simultaneously succeeds and gets cancelled.
This exactly matches my experience, it's nontrivial to determine the singular 'owner' for a given future instance.
--
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.
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
[GitHub] [httpcomponents-client] rschmitt commented on a change in pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
rschmitt commented on a change in pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#discussion_r655653290
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -329,7 +331,22 @@ public synchronized ConnectionEndpoint get(
@Override
public boolean cancel() {
- return leaseFuture.cancel(true);
+ if (cancelled.compareAndSet(false, true)) {
+ final boolean result = leaseFuture.cancel(true);
+ if (!result) {
+ // The lease request has already completed successfully and cannot be cancelled
+ try {
+ final PoolEntry<HttpRoute, ManagedHttpClientConnection> poolEntry = leaseFuture.get(1, TimeUnit.MILLISECONDS);
Review comment:
I'd prefer to see `leaseFuture.get(0, TimeUnit.MILLISECONDS)` here, by way of asserting that this call should not block
--
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.
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
[GitHub] [httpcomponents-client] ok2c commented on pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#issuecomment-865317031
> This proposal passes all of my regression tests. Would it allow us to also simplify the cancellation checks in classes like `InternalExecRuntime`?
Great. Please allow me a few more day to work on the test coverage and also fix the same defect in the async code. I am also going to review and clean up `InternalExecRuntime` at the same time.
--
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.
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
[GitHub] [httpcomponents-client] carterkozak commented on pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#issuecomment-865238036
déjà vu, I've run into this flavor of leak a number of times because it's incredibly difficult to handle `cancel()` on any `Future<? extends AutoCloseable>`.
> the core problem here is whose responsibility it is to release a pool entry back to the pool if the lease request simultaneously succeeds and gets cancelled.
This exactly matches my experience, it's nontrivial to determine the singular 'owner' for a given future instance.
--
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.
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
[GitHub] [httpcomponents-client] ok2c commented on pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#issuecomment-868724887
@rschmitt I came up with what I beleive is a better fix. I have also added test cases to verify correctness of new code.
Essentially I was going about it all wrong. I was trying to ensure that request execution gets aborted as early as possible and by doing so I was creating more points of potential thread race conditions. Now I simplified things by letting the request execution procced to reach a point where it is easier to avoid race conditions and to ensure correct resource deallocation. Several chunks of ugly code is also now gone from `InternalExecRuntime`.
Please review.
--
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
[GitHub] [httpcomponents-client] ok2c merged pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
ok2c merged pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309
--
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
[GitHub] [httpcomponents-client] ok2c merged pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
ok2c merged pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309
--
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
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#discussion_r655674228
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -329,7 +331,22 @@ public synchronized ConnectionEndpoint get(
@Override
public boolean cancel() {
- return leaseFuture.cancel(true);
+ if (cancelled.compareAndSet(false, true)) {
+ final boolean result = leaseFuture.cancel(true);
+ if (!result) {
+ // The lease request has already completed successfully and cannot be cancelled
+ try {
+ final PoolEntry<HttpRoute, ManagedHttpClientConnection> poolEntry = leaseFuture.get(1, TimeUnit.MILLISECONDS);
Review comment:
@rschmitt This call should never block given at this point that the future is complete but you are absolutely right. Will do so.
--
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.
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
[GitHub] [httpcomponents-client] rschmitt commented on pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
rschmitt commented on pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#issuecomment-865298551
This proposal passes all of my regression tests. Would it allow us to also simplify the cancellation checks in classes like `InternalExecRuntime`?
--
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.
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
[GitHub] [httpcomponents-client] rschmitt commented on pull request #309: Connection lease request cancellation bug
Posted by GitBox <gi...@apache.org>.
rschmitt commented on pull request #309:
URL: https://github.com/apache/httpcomponents-client/pull/309#issuecomment-865298551
This proposal passes all of my regression tests. Would it allow us to also simplify the cancellation checks in classes like `InternalExecRuntime`?
--
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.
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