Bug fix: classic connection managers fail to take #isConsistent() flag into account when re-using persistent connections

ok2c · ok2c · commit 9cfdd54c94ab · 2020-05-27T16:37:37.000+02:00
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
@@ -265,7 +265,7 @@ public synchronized void release(final ConnectionEndpoint endpoint, final Object
                 this.conn.close(CloseMode.GRACEFUL);
             }
             this.updated = System.currentTimeMillis();
-            if (!this.conn.isOpen()) {
+            if (!this.conn.isOpen() && !this.conn.isConsistent()) {
                 this.conn = null;
                 this.route = null;
                 this.conn = null;
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
@@ -355,7 +355,7 @@ public void release(final ConnectionEndpoint endpoint, final Object state, final
         if (conn != null && keepAlive == null) {
             conn.close(CloseMode.GRACEFUL);
         }
-        boolean reusable = conn != null && conn.isOpen();
+        boolean reusable = conn != null && conn.isOpen() && conn.isConsistent();
         try {
             if (reusable) {
                 entry.updateState(state);
@@ -371,6 +371,10 @@ public void release(final ConnectionEndpoint endpoint, final Object state, final
                     log.debug(ConnPoolSupport.getId(endpoint) + ": connection " + ConnPoolSupport.getId(conn) +
                             " can be kept alive " + s);
                 }
+            } else {
+                if (this.log.isDebugEnabled()) {
+                    this.log.debug(ConnPoolSupport.getId(endpoint) + ": connection is not kept alive");
+                }
             }
         } catch (final RuntimeException ex) {
             reusable = false;
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
@@ -107,6 +107,7 @@ public void testLeaseRelease() throws Exception {
         Mockito.when(plainSocketFactory.createSocket(Mockito.<HttpContext>any())).thenReturn(socket);
 
         Mockito.when(conn.isOpen()).thenReturn(true);
+        Mockito.when(conn.isConsistent()).thenReturn(true);
         Mockito.when(future.isCancelled()).thenReturn(false);
         Mockito.when(future.get(1, TimeUnit.SECONDS)).thenReturn(entry);
         Mockito.when(pool.lease(
@@ -213,7 +214,8 @@ public void testReleaseReusable() throws Exception {
                 Mockito.<Timeout>any(),
                 Mockito.<FutureCallback<PoolEntry<HttpRoute, ManagedHttpClientConnection>>>eq(null)))
                 .thenReturn(future);
-        Mockito.when(conn.isOpen()).thenReturn(Boolean.TRUE);
+        Mockito.when(conn.isOpen()).thenReturn(true);
+        Mockito.when(conn.isConsistent()).thenReturn(true);
 
         final LeaseRequest connRequest1 = mgr.lease("some-id", route, null);
         final ConnectionEndpoint endpoint1 = connRequest1.get(Timeout.ofSeconds(1));

Original file line number	Diff line number	Diff line change
`@@ -265,7 +265,7 @@ public synchronized void release(final ConnectionEndpoint endpoint, final Object`
`265`	`265`	`this.conn.close(CloseMode.GRACEFUL);`
`266`	`266`	`}`
`267`	`267`	`this.updated = System.currentTimeMillis();`
`268`		`- if (!this.conn.isOpen()) {`
	`268`	`+ if (!this.conn.isOpen() && !this.conn.isConsistent()) {`
`269`	`269`	`this.conn = null;`
`270`	`270`	`this.route = null;`
`271`	`271`	`this.conn = null;`
Original file line number	Diff line number	Diff line change
`@@ -355,7 +355,7 @@ public void release(final ConnectionEndpoint endpoint, final Object state, final`
`355`	`355`	`if (conn != null && keepAlive == null) {`
`356`	`356`	`conn.close(CloseMode.GRACEFUL);`
`357`	`357`	`}`
`358`		`- boolean reusable = conn != null && conn.isOpen();`
	`358`	`+ boolean reusable = conn != null && conn.isOpen() && conn.isConsistent();`
`359`	`359`	`try {`
`360`	`360`	`if (reusable) {`
`361`	`361`	`entry.updateState(state);`
`@@ -371,6 +371,10 @@ public void release(final ConnectionEndpoint endpoint, final Object state, final`
`371`	`371`	`log.debug(ConnPoolSupport.getId(endpoint) + ": connection " + ConnPoolSupport.getId(conn) +`
`372`	`372`	`" can be kept alive " + s);`
`373`	`373`	`}`
	`374`	`+ } else {`
	`375`	`+ if (this.log.isDebugEnabled()) {`
	`376`	`+ this.log.debug(ConnPoolSupport.getId(endpoint) + ": connection is not kept alive");`
	`377`	`+ }`
`374`	`378`	`}`
`375`	`379`	`} catch (final RuntimeException ex) {`
`376`	`380`	`reusable = false;`