Regression: connection managers fail to take into account per route connection config when closing expired connections

ok2c · ok2c · commit fe1e095ef9e0 · 2022-11-06T11:11:53.000+01:00
diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestConnectionManagement.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestConnectionManagement.java
@@ -58,7 +58,6 @@
 import org.apache.hc.core5.util.TimeValue;
 import org.apache.hc.core5.util.Timeout;
 import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
@@ -276,7 +275,7 @@ public void testCloseExpiredIdleConnections() throws Exception {
         connManager.close();
     }
 
-    @Test @Disabled
+    @Test
     public void testCloseExpiredTTLConnections() throws Exception {
         final ClassicTestServer server = startServer();
         server.registerHandler("/random/*", new RandomHandler());
@@ -294,6 +293,7 @@ public void testCloseExpiredTTLConnections() throws Exception {
         );
 
         final PoolingHttpClientConnectionManager connManager = testResources.connManager();
+        connManager.setMaxTotal(1);
 
         final HttpRoute route = new HttpRoute(target, null, false);
         final HttpContext context = new BasicHttpContext();
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionConfig.java b/httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionConfig.java
@@ -107,7 +107,7 @@ protected ConnectionConfig clone() throws CloneNotSupportedException {
     public String toString() {
         final StringBuilder builder = new StringBuilder();
         builder.append("[");
-        builder.append(", connectTimeout=").append(connectTimeout);
+        builder.append("connectTimeout=").append(connectTimeout);
         builder.append(", socketTimeout=").append(socketTimeout);
         builder.append(", validateAfterInactivity=").append(validateAfterInactivity);
         builder.append(", timeToLive=").append(timeToLive);
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
@@ -188,19 +188,33 @@ protected PoolingHttpClientConnectionManager(
         this.connectionOperator = Args.notNull(httpClientConnectionOperator, "Connection operator");
         switch (poolConcurrencyPolicy != null ? poolConcurrencyPolicy : PoolConcurrencyPolicy.STRICT) {
             case STRICT:
-                this.pool = new StrictConnPool<>(
+                this.pool = new StrictConnPool<HttpRoute, ManagedHttpClientConnection>(
                         DEFAULT_MAX_CONNECTIONS_PER_ROUTE,
                         DEFAULT_MAX_TOTAL_CONNECTIONS,
                         timeToLive,
                         poolReusePolicy,
-                        null);
+                        null) {
+
+                    @Override
+                    public void closeExpired() {
+                        enumAvailable(e -> closeIfExpired(e));
+                    }
+
+                };
                 break;
             case LAX:
-                this.pool = new LaxConnPool<>(
+                this.pool = new LaxConnPool<HttpRoute, ManagedHttpClientConnection>(
                         DEFAULT_MAX_CONNECTIONS_PER_ROUTE,
                         timeToLive,
                         poolReusePolicy,
-                        null);
+                        null) {
+
+                    @Override
+                    public void closeExpired() {
+                        enumAvailable(e -> closeIfExpired(e));
+                    }
+
+                };
                 break;
             default:
                 throw new IllegalArgumentException("Unexpected PoolConcurrencyPolicy value: " + poolConcurrencyPolicy);
@@ -570,6 +584,19 @@ public void setTlsConfigResolver(final Resolver<HttpHost, TlsConfig> tlsConfigRe
         this.tlsConfigResolver = tlsConfigResolver;
     }
 
+    void closeIfExpired(final PoolEntry<HttpRoute, ManagedHttpClientConnection> entry) {
+        final long now = System.currentTimeMillis();
+        if (entry.getExpiryDeadline().isBefore(now)) {
+            entry.discardConnection(CloseMode.GRACEFUL);
+        } else {
+            final ConnectionConfig connectionConfig = resolveConnectionConfig(entry.getRoute());
+            final TimeValue timeToLive = connectionConfig.getTimeToLive();
+            if (timeToLive != null && Deadline.calculate(entry.getCreated(), timeToLive).isBefore(now)) {
+                entry.discardConnection(CloseMode.GRACEFUL);
+            }
+        }
+    }
+
     /**
      * @deprecated Use custom {@link #setConnectionConfigResolver(Resolver)}
      */
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
@@ -172,19 +172,33 @@ protected PoolingAsyncClientConnectionManager(
         this.connectionOperator = Args.notNull(connectionOperator, "Connection operator");
         switch (poolConcurrencyPolicy != null ? poolConcurrencyPolicy : PoolConcurrencyPolicy.STRICT) {
             case STRICT:
-                this.pool = new StrictConnPool<>(
+                this.pool = new StrictConnPool<HttpRoute, ManagedAsyncClientConnection>(
                         DEFAULT_MAX_CONNECTIONS_PER_ROUTE,
                         DEFAULT_MAX_TOTAL_CONNECTIONS,
                         timeToLive,
                         poolReusePolicy,
-                        null);
+                        null) {
+
+                    @Override
+                    public void closeExpired() {
+                        enumAvailable(e -> closeIfExpired(e));
+                    }
+
+                };
                 break;
             case LAX:
-                this.pool = new LaxConnPool<>(
+                this.pool = new LaxConnPool<HttpRoute, ManagedAsyncClientConnection>(
                         DEFAULT_MAX_CONNECTIONS_PER_ROUTE,
                         timeToLive,
                         poolReusePolicy,
-                        null);
+                        null) {
+
+                    @Override
+                    public void closeExpired() {
+                        enumAvailable(e -> closeIfExpired(e));
+                    }
+
+                };
                 break;
             default:
                 throw new IllegalArgumentException("Unexpected PoolConcurrencyPolicy value: " + poolConcurrencyPolicy);
@@ -619,6 +633,19 @@ public void setTlsConfigResolver(final Resolver<HttpHost, TlsConfig> tlsConfigRe
         this.tlsConfigResolver = tlsConfigResolver;
     }
 
+    void closeIfExpired(final PoolEntry<HttpRoute, ManagedAsyncClientConnection > entry) {
+        final long now = System.currentTimeMillis();
+        if (entry.getExpiryDeadline().isBefore(now)) {
+            entry.discardConnection(CloseMode.GRACEFUL);
+        } else {
+            final ConnectionConfig connectionConfig = resolveConnectionConfig(entry.getRoute());
+            final TimeValue timeToLive = connectionConfig.getTimeToLive();
+            if (timeToLive != null && Deadline.calculate(entry.getCreated(), timeToLive).isBefore(now)) {
+                entry.discardConnection(CloseMode.GRACEFUL);
+            }
+        }
+    }
+
     /**
      * @deprecated Use custom {@link #setConnectionConfigResolver(Resolver)}
      */
