HTTPCLIENT-2416 - Fix pool entry leak on late lease completion

[arturobernalg]

This patch fixes a race in PoolingHttpClientConnectionManager#lease().

When LeaseRequest#get(...) exits with TimeoutException or InterruptedException, the current code calls leaseFuture.cancel(true). If the underlying lease completes just before cancellation is observed, cancel(true) returns false and the PoolEntry can remain leased, because no ConnectionEndpoint was returned to the caller and nothing releases it.

The fix handles that case in the exceptional path: if cancel(true) returns false, the code attempts a zero-timeout get() on the lease future and, if a late-completed PoolEntry is obtained, releases it immediately back to the pool.

[ok2c]

@arturobernalg If the method fails once the proposed fix to call another time? That does not look right. There has to be a better fix.

[arturobernalg]

@ok2c I dropped the second get() and reworked it to use the pool.lease(..., FutureCallback) to capture late completions and release the PoolEntry on timeout/interrupt/cancel.

[ok2c]

@arturobernalg This hardly looks any better to me. I like @benjaminp approach better (other than the exception handling part) but I would try to push the fix into PoolingHttpClientConnectionManager or possibly even into core.

I will try to look into it on the weekend

[arturobernalg]

@ok2c Then I'll check the core and the 3 ConnPool

[benjaminp]

Thanks, this does seem to resolve my examples raised on the ticket.

I do have a suggested fix of my own, which is a bit less code:

diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/InternalExecRuntime.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/InternalExecRuntime.java
index 7813bc171..dbc7d17b1 100644
--- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/InternalExecRuntime.java
+++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/InternalExecRuntime.java
@@ -28,6 +28,7 @@
 package org.apache.hc.client5.http.impl.classic;
 
 import java.io.IOException;
+import java.util.concurrent.CancellationException;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicReference;
@@ -118,10 +119,10 @@ public void acquireEndpoint(
                     log.debug("{} acquired endpoint {}", id, ConnPoolSupport.getId(connectionEndpoint));
                 }
             } catch (final TimeoutException ex) {
-                connRequest.cancel();
+                cancelLeaklessly(connRequest);
                 throw new ConnectionRequestTimeoutException(ex.getMessage());
             } catch (final InterruptedException interrupted) {
-                connRequest.cancel();
+                cancelLeaklessly(connRequest);
                 Thread.currentThread().interrupt();
                 throw new RequestFailedException("Request aborted", interrupted);
             } catch (final ExecutionException ex) {
@@ -137,6 +138,18 @@ public void acquireEndpoint(
         }
     }
 
+    private void cancelLeaklessly(final LeaseRequest request) {
+        if (!request.cancel()) {
+            try {
+                discardEndpoint(request.get(Timeout.INFINITE));
+            } catch (final InterruptedException | TimeoutException e) {
+                throw new IllegalStateException(e);
+            } catch (final ExecutionException | CancellationException e) {
+                // ignore
+            }
+        }
+    }
+
     ConnectionEndpoint ensureValid() {
         final ConnectionEndpoint endpoint = endpointRef.get();
         if (endpoint == null) {

The idea is the check if cancelation on InterruptedException or TimeoutException actually mutates future. If it did not, it means the future was already completed and may have a connection that must be released.

[arturobernalg]

Close it in favor of 4b5836b