From 7724432894209f95dd70708de478465b9fac5e6f Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Thu, 9 Nov 2023 17:17:20 +0100 Subject: [PATCH] HTTPCLIENT-2301. Refactor release method to use local conn variable. This commit updates the release method to use the local conn variable from internalEndpoint.detach() for accurate state management and resource cleanup, addressing the issue HTTPCLIENT-2301. (#502) --- .../io/BasicHttpClientConnectionManager.java | 25 +++--- .../TestBasicHttpClientConnectionManager.java | 76 +++++++++++++++++++ 2 files changed, 88 insertions(+), 13 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 5d127fbe8..e6f4b911d 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 @@ -369,6 +369,7 @@ public class BasicHttpClientConnectionManager implements HttpClientConnectionMan } } + private InternalConnectionEndpoint cast(final ConnectionEndpoint endpoint) { if (endpoint instanceof InternalConnectionEndpoint) { return (InternalConnectionEndpoint) endpoint; @@ -390,22 +391,13 @@ public class BasicHttpClientConnectionManager implements HttpClientConnectionMan return; } try { - if (keepAlive == null) { - this.conn.close(CloseMode.GRACEFUL); + if (keepAlive == null && conn != null) { + conn.close(CloseMode.GRACEFUL); } this.updated = System.currentTimeMillis(); - if (!this.conn.isOpen() && !this.conn.isConsistent()) { - this.route = null; - this.conn = null; - this.expiry = Long.MAX_VALUE; - if (LOG.isDebugEnabled()) { - LOG.debug("{} Connection is not kept alive", id); - } - } else { + if (conn != null && conn.isOpen() && conn.isConsistent()) { this.state = state; - if (conn != null) { - conn.passivate(); - } + conn.passivate(); if (TimeValue.isPositive(keepAlive)) { if (LOG.isDebugEnabled()) { LOG.debug("{} Connection can be kept alive for {}", id, keepAlive); @@ -417,6 +409,13 @@ public class BasicHttpClientConnectionManager implements HttpClientConnectionMan } this.expiry = Long.MAX_VALUE; } + } else { + this.route = null; + this.conn = null; + this.expiry = Long.MAX_VALUE; + if (LOG.isDebugEnabled()) { + LOG.debug("{} Connection is not kept alive", id); + } } } finally { this.leased = false; diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestBasicHttpClientConnectionManager.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestBasicHttpClientConnectionManager.java index bb59b7678..9c1f3dd6b 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestBasicHttpClientConnectionManager.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestBasicHttpClientConnectionManager.java @@ -126,6 +126,8 @@ public class TestBasicHttpClientConnectionManager { Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE); + mgr.release(endpoint1, null, TimeValue.ofMilliseconds(100)); Assertions.assertEquals(route, mgr.getRoute()); @@ -153,6 +155,7 @@ public class TestBasicHttpClientConnectionManager { Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE); mgr.release(endpoint1, "some other state", TimeValue.ofMilliseconds(10000)); @@ -181,6 +184,7 @@ public class TestBasicHttpClientConnectionManager { Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE, Boolean.FALSE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE, Boolean.FALSE); mgr.release(endpoint1, null, TimeValue.NEG_ONE_MILLISECOND); @@ -212,6 +216,7 @@ public class TestBasicHttpClientConnectionManager { Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE, Boolean.FALSE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE, Boolean.FALSE); mgr.release(endpoint1, null, TimeValue.ofMilliseconds(10)); @@ -256,6 +261,7 @@ public class TestBasicHttpClientConnectionManager { Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE); mgr.release(endpoint1, null, TimeValue.NEG_ONE_MILLISECOND); @@ -288,6 +294,7 @@ public class TestBasicHttpClientConnectionManager { Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE, Boolean.FALSE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE, Boolean.FALSE); mgr.release(endpoint1, null, TimeValue.ofMilliseconds(10)); @@ -315,6 +322,7 @@ public class TestBasicHttpClientConnectionManager { Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE, Boolean.FALSE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE, Boolean.FALSE); mgr.release(endpoint1, null, TimeValue.NEG_ONE_MILLISECOND); @@ -480,4 +488,72 @@ public class TestBasicHttpClientConnectionManager { socket, "somehost", 8443, tlsConfig, context); } + + @Test + public void shouldCloseStaleConnectionAndCreateNewOne() throws Exception { + final HttpHost target = new HttpHost("somehost", 80); + final HttpRoute route = new HttpRoute(target); + + Mockito.when(connFactory.createConnection(Mockito.any())).thenReturn(conn); + + final LeaseRequest connRequest1 = mgr.lease("some-id", route, null); + final ConnectionEndpoint endpoint1 = connRequest1.get(Timeout.ZERO_MILLISECONDS); + Assertions.assertNotNull(endpoint1); + + Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); + + Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE); + + mgr.release(endpoint1, null, TimeValue.ofMilliseconds(100)); + + Assertions.assertEquals(route, mgr.getRoute()); + Assertions.assertNull(mgr.getState()); + + final LeaseRequest connRequest2 = mgr.lease("some-id", route, null); + Mockito.when(conn.isStale()).thenReturn(Boolean.TRUE); + final ConnectionEndpoint conn2 = connRequest2.get(Timeout.ZERO_MILLISECONDS); + Assertions.assertNotNull(conn2); + Assertions.assertTrue(conn2.isConnected()); + + Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); + } + + @Test + public void shouldCloseGRACEFULStaleConnection() throws Exception { + final HttpHost target = new HttpHost("somehost", 80); + final HttpRoute route = new HttpRoute(target); + + Mockito.when(connFactory.createConnection(Mockito.any())).thenReturn(conn); + + final LeaseRequest connRequest1 = mgr.lease("some-id", route, null); + final ConnectionEndpoint endpoint1 = connRequest1.get(Timeout.ZERO_MILLISECONDS); + Assertions.assertNotNull(endpoint1); + + Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); + + Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE); + Mockito.when(conn.isConsistent()).thenReturn(Boolean.TRUE); + + // Simulate the connection being released with no keep-alive (it should be closed) + mgr.release(endpoint1, null, null); + + // Ensure the connection was closed + Mockito.verify(conn, Mockito.times(1)).close(CloseMode.GRACEFUL); + + // Now, when a new lease request is made, the connection is stale + Mockito.when(conn.isStale()).thenReturn(Boolean.TRUE); + + // Attempt to lease a new connection + final LeaseRequest connRequest2 = mgr.lease("some-id", route, null); + final ConnectionEndpoint endpoint2 = connRequest2.get(Timeout.ZERO_MILLISECONDS); + Assertions.assertNotNull(endpoint2); + + // The connection should be closed and a new one created because the old one was stale + Mockito.verify(connFactory, Mockito.times(1)).createConnection(Mockito.any()); + + // The new connection should be connected + Assertions.assertTrue(endpoint2.isConnected()); + } + }