From 8666714280d5595ed7e22aec4ac831bf555fd5af Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Fri, 6 Mar 2026 10:14:29 +0100 Subject: [PATCH] Fix lease timeout race to prevent pool entry leak --- .../org/apache/hc/core5/pool/LaxConnPool.java | 6 +- .../hc/core5/pool/RouteSegmentedConnPool.java | 13 ++ .../apache/hc/core5/pool/StrictConnPool.java | 6 +- .../TestStrictConnPoolLeaseTimeoutRace.java | 160 ++++++++++++++++++ 4 files changed, 181 insertions(+), 4 deletions(-) create mode 100644 httpcore5/src/test/java/org/apache/hc/core5/pool/TestStrictConnPoolLeaseTimeoutRace.java diff --git a/httpcore5/src/main/java/org/apache/hc/core5/pool/LaxConnPool.java b/httpcore5/src/main/java/org/apache/hc/core5/pool/LaxConnPool.java index 9478fdcfa..57e6bd40f 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/pool/LaxConnPool.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/pool/LaxConnPool.java @@ -466,8 +466,10 @@ public PoolEntry get( try { return super.get(timeout, unit); } catch (final TimeoutException ex) { - cancel(); - throw ex; + if (cancel()) { + throw ex; + } + return super.get(); } } diff --git a/httpcore5/src/main/java/org/apache/hc/core5/pool/RouteSegmentedConnPool.java b/httpcore5/src/main/java/org/apache/hc/core5/pool/RouteSegmentedConnPool.java index 667519137..4f9545994 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/pool/RouteSegmentedConnPool.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/pool/RouteSegmentedConnPool.java @@ -212,6 +212,19 @@ public boolean cancel(final boolean mayInterruptIfRunning) { maybeCleanupSegment(this.route, this.seg); return super.cancel(mayInterruptIfRunning); } + + @Override + public PoolEntry get(final long timeout, final TimeUnit unit) + throws InterruptedException, java.util.concurrent.ExecutionException, TimeoutException { + try { + return super.get(timeout, unit); + } catch (final TimeoutException ex) { + if (cancel(true)) { + throw ex; + } + return super.get(); + } + } } @Override diff --git a/httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java b/httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java index f5c3e4285..15eb844e5 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java @@ -177,8 +177,10 @@ public PoolEntry get( try { return super.get(timeout, unit); } catch (final TimeoutException ex) { - cancel(); - throw ex; + if (cancel()) { + throw ex; + } + return super.get(); } } diff --git a/httpcore5/src/test/java/org/apache/hc/core5/pool/TestStrictConnPoolLeaseTimeoutRace.java b/httpcore5/src/test/java/org/apache/hc/core5/pool/TestStrictConnPoolLeaseTimeoutRace.java new file mode 100644 index 000000000..f2a19a6b5 --- /dev/null +++ b/httpcore5/src/test/java/org/apache/hc/core5/pool/TestStrictConnPoolLeaseTimeoutRace.java @@ -0,0 +1,160 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ +package org.apache.hc.core5.pool; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; +import java.util.stream.Stream; + +import org.apache.hc.core5.http.SocketModalCloseable; +import org.apache.hc.core5.io.CloseMode; +import org.apache.hc.core5.util.TimeValue; +import org.apache.hc.core5.util.Timeout; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +class TestConnPoolLeaseTimeout { + + private static final Timeout TIMEOUT = Timeout.ofSeconds(30); + + static final class DummyConn implements SocketModalCloseable { + + private volatile Timeout socketTimeout; + + @Override + public Timeout getSocketTimeout() { + return socketTimeout; + } + + @Override + public void setSocketTimeout(final Timeout timeout) { + this.socketTimeout = timeout; + } + + @Override + public void close(final CloseMode closeMode) { + } + + @Override + public void close() throws IOException { + } + } + + static final class PoolCase { + final String name; + final Supplier> supplier; + + PoolCase(final String name, final Supplier> supplier) { + this.name = name; + this.supplier = supplier; + } + + @Override + public String toString() { + return name; + } + } + + static Stream pools() { + return Stream.of( + new PoolCase("STRICT", () -> new StrictConnPool<>(1, 1)), + new PoolCase("LAX", () -> new LaxConnPool<>(1)), + new PoolCase("OFFLOCK", () -> new RouteSegmentedConnPool<>( + 1, + 1, + TimeValue.NEG_ONE_MILLISECOND, + PoolReusePolicy.LIFO, + new DefaultDisposalCallback<>())) + ); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("pools") + @org.junit.jupiter.api.Timeout(60) + void testLeaseTimeoutDoesNotLeakLeasedEntries(final PoolCase poolCase) throws Exception { + final ManagedConnPool pool = poolCase.supplier.get(); + + final String route = "route-1"; + final Timeout requestTimeout = Timeout.ofMicroseconds(1); + + final int concurrentThreads = 10; + final CountDownLatch countDownLatch = new CountDownLatch(concurrentThreads); + final AtomicLong n = new AtomicLong(concurrentThreads * 100); + + final ExecutorService executorService = Executors.newFixedThreadPool(concurrentThreads); + final AtomicReference unexpectedException = new AtomicReference<>(); + try { + for (int i = 0; i < concurrentThreads; i++) { + executorService.execute(() -> { + try { + while (n.decrementAndGet() > 0) { + final Future> f = pool.lease(route, null, requestTimeout, null); + try { + final PoolEntry entry = + f.get(requestTimeout.getDuration(), requestTimeout.getTimeUnit()); + pool.release(entry, true); + } catch (final InterruptedException ex) { + Thread.currentThread().interrupt(); + unexpectedException.compareAndSet(null, ex); + } catch (final TimeoutException ex) { + f.cancel(true); + } catch (final ExecutionException ex) { + f.cancel(true); + if (!(ex.getCause() instanceof TimeoutException)) { + unexpectedException.compareAndSet(null, ex); + } + } + } + } finally { + countDownLatch.countDown(); + } + }); + } + + Assertions.assertTrue(countDownLatch.await(TIMEOUT.getDuration(), TIMEOUT.getTimeUnit())); + Assertions.assertTrue(n.get() <= 0); + Assertions.assertNull(unexpectedException.get()); + + final PoolStats stats = pool.getStats(route); + Assertions.assertEquals(0, stats.getLeased()); + + } finally { + executorService.shutdownNow(); + executorService.awaitTermination(TIMEOUT.getDuration(), TIMEOUT.getTimeUnit()); + pool.close(CloseMode.GRACEFUL); + } + } +} \ No newline at end of file