Skip to content

Commit

Permalink
HTTPCLIENT-2318 - Enhance PoolingHttpClientConnectionManager with isS…
Browse files Browse the repository at this point in the history
…hutdown 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.
  • Loading branch information
arturobernalg authored and ok2c committed Feb 23, 2024
1 parent 8a7f707 commit 407b915
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.");
}


}

0 comments on commit 407b915

Please sign in to comment.