From 407b9152ccafa120ce9e9cd14325c3cc78eb20c1 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Tue, 20 Feb 2024 20:39:10 +0100 Subject: [PATCH] HTTPCLIENT-2318 - Enhance PoolingHttpClientConnectionManager with isShutdown State Check. This commit introduces an `isShutdown` method to the `PoolingHttpClientConnectionManager` class, providing a thread-safe way to check whether the connection manager has been closed. The addition leverages an existing `AtomicBoolean` closed flag to ensure that the shutdown state can be queried reliably in concurrent environments. --- .../io/BasicHttpClientConnectionManager.java | 3 +- .../PoolingHttpClientConnectionManager.java | 24 +++++++++ .../PoolingAsyncClientConnectionManager.java | 12 ++++- ...estPoolingHttpClientConnectionManager.java | 54 +++++++++++++++++++ 4 files changed, 91 insertions(+), 2 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java index 116b18303..ef99e7872 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java @@ -711,8 +711,9 @@ public EndpointInfo getInfo() { * * @return {@code true} if the connection manager has been shut down and is closed, otherwise * return {@code false}. + * @since 5.4 */ - boolean isClosed() { + public boolean isClosed() { return this.closed.get(); } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java index 8fb5f6505..a60e9ebd6 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java @@ -423,6 +423,11 @@ public void release(final ConnectionEndpoint endpoint, final Object state, final if (LOG.isDebugEnabled()) { LOG.debug("{} releasing endpoint", ConnPoolSupport.getId(endpoint)); } + + if (this.isClosed()) { + return; + } + final ManagedHttpClientConnection conn = entry.getConnection(); if (conn != null && keepAlive == null) { conn.close(CloseMode.GRACEFUL); @@ -522,11 +527,17 @@ public void closeIdle(final TimeValue idleTime) { if (LOG.isDebugEnabled()) { LOG.debug("Closing connections idle longer than {}", idleTime); } + if (isClosed()) { + return; + } this.pool.closeIdle(idleTime); } @Override public void closeExpired() { + if (isClosed()) { + return; + } LOG.debug("Closing expired connections"); this.pool.closeExpired(); } @@ -796,4 +807,17 @@ public EndpointInfo getInfo() { } + /** + * Method that can be called to determine whether the connection manager has been shut down and + * is closed or not. + * + * @return {@code true} if the connection manager has been shut down and is closed, otherwise + * return {@code false}. + * @since 5.4 + */ + public boolean isClosed() { + return this.closed.get(); + } + + } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java index ce842c38b..c7a542d6a 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java @@ -394,6 +394,9 @@ public void release(final AsyncConnectionEndpoint endpoint, final Object state, if (LOG.isDebugEnabled()) { LOG.debug("{} releasing endpoint", ConnPoolSupport.getId(endpoint)); } + if (this.isClosed()) { + return; + } final ManagedAsyncClientConnection connection = entry.getConnection(); boolean reusable = connection != null && connection.isOpen(); try { @@ -575,11 +578,17 @@ public int getMaxPerRoute(final HttpRoute route) { @Override public void closeIdle(final TimeValue idletime) { + if (isClosed()) { + return; + } pool.closeIdle(idletime); } @Override public void closeExpired() { + if (isClosed()) { + return; + } pool.closeExpired(); } @@ -773,8 +782,9 @@ public EndpointInfo getInfo() { * * @return {@code true} if the connection manager has been shut down and is closed, otherwise * return {@code false}. + * @since 5.4 */ - boolean isClosed() { + public boolean isClosed() { return this.closed.get(); } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java index d5be5c162..b70b72213 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java @@ -30,6 +30,8 @@ import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.Socket; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -351,4 +353,56 @@ public void testProxyConnectAndUpgrade() throws Exception { socket, "somehost", 443, tlsConfig, context); } + @Test + public void testIsShutdownInitially() { + Assertions.assertFalse(mgr.isClosed(), "Connection manager should not be shutdown initially."); + } + + @Test + public void testShutdownIdempotency() { + mgr.close(); + Assertions.assertTrue(mgr.isClosed(), "Connection manager should remain shutdown after the first call to shutdown."); + mgr.close(); // Second call to shutdown + Assertions.assertTrue(mgr.isClosed(), "Connection manager should still be shutdown after subsequent calls to shutdown."); + } + + @Test + public void testLeaseAfterShutdown() { + mgr.close(); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + // Attempt to lease a connection after shutdown + mgr.lease("some-id", new HttpRoute(new HttpHost("localhost")), null); + }, "Attempting to lease a connection after shutdown should throw an exception."); + } + + + @Test + public void testIsShutdown() { + // Setup phase + Mockito.when(pool.isShutdown()).thenReturn(false, true); // Simulate changing states + + // Execution phase: Initially, the manager should not be shutdown + Assertions.assertFalse(mgr.isClosed(), "Connection manager should not be shutdown initially."); + + // Simulate shutting down the manager + mgr.close(); + + // Verification phase: Now, the manager should be reported as shutdown + Assertions.assertTrue(mgr.isClosed(), "Connection manager should be shutdown after close() is called."); + } + + + @Test + public void testConcurrentShutdown() throws InterruptedException { + final ExecutorService executor = Executors.newFixedThreadPool(2); + // Submit two shutdown tasks to be run in parallel, explicitly calling close() with no arguments + executor.submit(() -> mgr.close()); + executor.submit(() -> mgr.close()); + executor.shutdown(); + executor.awaitTermination(1, TimeUnit.SECONDS); + + Assertions.assertTrue(mgr.isClosed(), "Connection manager should be shutdown after concurrent calls to shutdown."); + } + + }