You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hc.apache.org by ab...@apache.org on 2023/11/09 16:17:26 UTC
(httpcomponents-client) branch 5.4.x updated: 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)
This is an automated email from the ASF dual-hosted git repository.
abernal pushed a commit to branch 5.4.x
in repository https://gitbox.apache.org/repos/asf/httpcomponents-client.git
The following commit(s) were added to refs/heads/5.4.x by this push:
new 6ac894615 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)
6ac894615 is described below
commit 6ac8946155c01cfa41fda5ac71840d96f0cf41c5
Author: Arturo Bernal <ab...@apache.org>
AuthorDate: Thu Nov 9 17:17:20 2023 +0100
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)
---
.../impl/io/BasicHttpClientConnectionManager.java | 25 ++++---
.../io/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 941f18b2a..f4ee58a3e 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());
+ }
+
}