From 8aed6242ccc22176dc7160c14f2d9f5c0c8c50cb Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sat, 5 Jul 2025 13:18:37 +0200 Subject: [PATCH] =?UTF-8?q?HTTPCLIENT-2379:=20PerRoutePool.release=20now?= =?UTF-8?q?=20decrements=20the=20counter=20first=20and=20keeps=20the=20ent?= =?UTF-8?q?ry=20in=20the=20map=20until=20the=20count=20reaches=20zero,=20r?= =?UTF-8?q?egardless=20of=20whether=20the=20connection=20is=20still=20open?= =?UTF-8?q?.=20The=20entry=20is=20handed=20back=20to=20the=20parent=20pool?= =?UTF-8?q?=20exactly=20once=E2=80=94on=20that=20final=20release=E2=80=94a?= =?UTF-8?q?nd=20any=20attempt=20to=20release=20an=20entry=20that=20was=20n?= =?UTF-8?q?ever=20leased=20now=20raises=20an=20IllegalStateException.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../http/impl/nio/H2SharingConnPool.java | 41 +++++++++-------- .../http/impl/nio/H2SharingConnPoolTest.java | 45 ++++--------------- .../impl/nio/H2SharingPerRoutePoolTest.java | 6 +-- 3 files changed, 32 insertions(+), 60 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPool.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPool.java index 2efc867fe1..57c219d3c4 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPool.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPool.java @@ -270,16 +270,17 @@ long track(final PoolEntry entry) { PoolEntry lease() { lock.lock(); try { - final PoolEntry 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 lease() { long release(final PoolEntry 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(); } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPoolTest.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPoolTest.java index a53043885e..077db0ff81 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingConnPoolTest.java +++ b/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 poolEntry = new PoolEntry<>(DEFAULT_ROUTE); - final H2SharingConnPool.PerRoutePool routePool = h2SharingPool.getPerRoutePool(DEFAULT_ROUTE); + final HttpConnection conn = Mockito.mock(HttpConnection.class); + Mockito.when(conn.isOpen()).thenReturn(true); + poolEntry.assignConnection(conn); + + final H2SharingConnPool.PerRoutePool routePool = + h2SharingPool.getPerRoutePool(DEFAULT_ROUTE); routePool.track(poolEntry); + final Future> future = + h2SharingPool.lease(DEFAULT_ROUTE, null, Timeout.ONE_MILLISECOND, callback); - final Future> 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 poolEntry = new PoolEntry<>(DEFAULT_ROUTE); - poolEntry.assignConnection(connection); - Mockito.when(connection.isOpen()).thenReturn(true); - final H2SharingConnPool.PerRoutePool 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 poolEntry = new PoolEntry<>(DEFAULT_ROUTE); - poolEntry.assignConnection(connection); - Mockito.when(connection.isOpen()).thenReturn(false); - final H2SharingConnPool.PerRoutePool 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") diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingPerRoutePoolTest.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingPerRoutePoolTest.java index dc1a573fa5..b956fb08c5 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/nio/H2SharingPerRoutePoolTest.java +++ b/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 } }