HTTPCLIENT-2379: PerRoutePool.release now decrements the counter first and keeps the entry in the map until the count reaches zero, regardless of whether the connection is still open. The entry is handed back to the parent pool exactly once—on that final release—and any attempt to release an entry that was never leased now raises an IllegalStateException. (#663)

(cherry picked from commit b1efda3)

Author: arturobernalg

httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPool.java

@@ -270,16 +270,17 @@ long track(final PoolEntry<T, C> entry) {
         PoolEntry<T, C> lease() {
             lock.lock();
             try {
-                final PoolEntry<T, C> entry = entryMap.entrySet().stream()
+                return entryMap.entrySet().stream()
+                        .filter(e -> {
+                            final C conn = e.getKey().getConnection();
+                            return conn != null && conn.isOpen();
+                        })
                         .min(Comparator.comparingLong(e -> e.getValue().get()))
-                        .map(Map.Entry::getKey)
+                        .map(e -> {
+                            e.getValue().incrementAndGet();
+                            return e.getKey();
+                        })
                         .orElse(null);
-                if (entry == null) {
-                    return null;
-                }
-                final AtomicLong counter = getCounter(entry);
-                counter.incrementAndGet();
-                return entry;
             } finally {
                 lock.unlock();
             }
@@ -288,20 +289,18 @@ PoolEntry<T, C> lease() {
         long release(final PoolEntry<T, C> entry, final boolean reusable) {
             lock.lock();
             try {
-                final C connection = entry.getConnection();
-                if (!reusable || connection == null || !connection.isOpen()) {
-                    entryMap.remove(entry);
-                    return 0;
-                } else {
-                    final AtomicLong counter = entryMap.compute(entry, (e, c) -> {
-                        if (c == null) {
-                            return null;
-                        }
-                        final long count = c.decrementAndGet();
-                        return count > 0 ? c : null;
-                    });
-                    return counter != null ? counter.get() : 0L;
+                if (!reusable) {
+                    entry.discardConnection(CloseMode.GRACEFUL);
                 }
+
+                final AtomicLong counter = entryMap.compute(entry, (e, c) -> {
+                    if (c == null) {
+                        return null;
+                    }
+                    final long count = c.decrementAndGet();
+                    return count > 0 ? c : null;
+                });
+                return counter != null ? counter.get() : 0L;
             } finally {
                 lock.unlock();
             }

httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPoolTest.java

@@ -86,10 +86,16 @@ void testLeaseFutureReturned() throws Exception {
     @Test
     void testLeaseExistingConnectionReturned() throws Exception {
         final PoolEntry<String, HttpConnection> poolEntry = new PoolEntry<>(DEFAULT_ROUTE);
-        final H2SharingConnPool.PerRoutePool<String, HttpConnection> routePool = h2SharingPool.getPerRoutePool(DEFAULT_ROUTE);
+        final HttpConnection conn = Mockito.mock(HttpConnection.class);
+        Mockito.when(conn.isOpen()).thenReturn(true);
+        poolEntry.assignConnection(conn);
+
+        final H2SharingConnPool.PerRoutePool<String, HttpConnection> routePool =
+                h2SharingPool.getPerRoutePool(DEFAULT_ROUTE);
         routePool.track(poolEntry);
+        final Future<PoolEntry<String, HttpConnection>> future =
+                h2SharingPool.lease(DEFAULT_ROUTE, null, Timeout.ONE_MILLISECOND, callback);
 
-        final Future<PoolEntry<String, HttpConnection>> future = h2SharingPool.lease(DEFAULT_ROUTE, null, Timeout.ONE_MILLISECOND, callback);
         Assertions.assertNotNull(future);
         Assertions.assertSame(poolEntry, future.get());
 
@@ -98,8 +104,7 @@ void testLeaseExistingConnectionReturned() throws Exception {
                 Mockito.any(),
                 Mockito.any(),
                 Mockito.any());
-        Mockito.verify(callback).completed(
-                Mockito.same(poolEntry));
+        Mockito.verify(callback).completed(Mockito.same(poolEntry));
     }
 
     @Test
@@ -314,38 +319,6 @@ void testReleaseReusableInCacheNotReturnedToPool() throws Exception {
                 Mockito.anyBoolean());
     }
 
-    @Test
-    void testReleaseNonReusableInCacheReturnedToPool() throws Exception {
-        final PoolEntry<String, HttpConnection> poolEntry = new PoolEntry<>(DEFAULT_ROUTE);
-        poolEntry.assignConnection(connection);
-        Mockito.when(connection.isOpen()).thenReturn(true);
-        final H2SharingConnPool.PerRoutePool<String, HttpConnection> routePool = h2SharingPool.getPerRoutePool(DEFAULT_ROUTE);
-        routePool.track(poolEntry);
-        routePool.track(poolEntry);
-
-        h2SharingPool.release(poolEntry, false);
-
-        Mockito.verify(connPool).release(
-                Mockito.same(poolEntry),
-                Mockito.eq(false));
-    }
-
-    @Test
-    void testReleaseReusableAndClosedInCacheReturnedToPool() throws Exception {
-        final PoolEntry<String, HttpConnection> poolEntry = new PoolEntry<>(DEFAULT_ROUTE);
-        poolEntry.assignConnection(connection);
-        Mockito.when(connection.isOpen()).thenReturn(false);
-        final H2SharingConnPool.PerRoutePool<String, HttpConnection> routePool = h2SharingPool.getPerRoutePool(DEFAULT_ROUTE);
-        routePool.track(poolEntry);
-        routePool.track(poolEntry);
-
-        h2SharingPool.release(poolEntry, true);
-
-        Mockito.verify(connPool).release(
-                Mockito.same(poolEntry),
-                Mockito.eq(true));
-    }
-
     /**
      * Same connection can only be released once.
      * Attempting to release it again will throw: IllegalStateException("Pool entry is not present in the set of leased entries")

httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingPerRoutePoolTest.java

@@ -98,7 +98,7 @@ void testReleaseNonReusable() {
         pool.track(poolEntry1);
         pool.track(poolEntry1);
 
-        Assertions.assertEquals(0, pool.release(poolEntry1, false));
+        Assertions.assertEquals(2, pool.release(poolEntry1, false)); // 3 → 2
     }
 
     @Test
@@ -114,7 +114,7 @@ void testReleaseConnectionClosed() {
         pool.track(poolEntry1);
 
         Mockito.when(poolEntry1.getConnection().isOpen()).thenReturn(false);
-        Assertions.assertEquals(0, pool.release(poolEntry1, true));
+        Assertions.assertEquals(2, pool.release(poolEntry1, true));  // 3 → 2
     }
 
     @Test
@@ -124,7 +124,7 @@ void testReleaseConnectionMissing() {
         pool.track(poolEntry1);
 
         poolEntry1.discardConnection(CloseMode.IMMEDIATE);
-        Assertions.assertEquals(0, pool.release(poolEntry1, true));
+        Assertions.assertEquals(2, pool.release(poolEntry1, true));  // 3 → 2
     }
 
 }