Bug fix: corrected sleep time calculation in IdleConnectionEvictor; use 1 minute sleep time by default

Author: ok2c

httpclient5/src/main/java/org/apache/hc/client5/http/impl/IdleConnectionEvictor.java

@@ -28,6 +28,7 @@
 package org.apache.hc.client5.http.impl;
 
 import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
 
 import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
@@ -46,14 +47,17 @@
 @Contract(threading = ThreadingBehavior.SAFE_CONDITIONAL)
 public final class IdleConnectionEvictor {
 
+    private static final TimeValue ONE_SECOND = TimeValue.ofSeconds(1L);
+    private static final TimeValue ONE_MINUTE = TimeValue.ofMinutes(1L);
+
     private final ThreadFactory threadFactory;
     private final Thread thread;
 
     public IdleConnectionEvictor(final ConnPoolControl<?> connectionManager, final ThreadFactory threadFactory,
                                  final TimeValue sleepTime, final TimeValue maxIdleTime) {
         Args.notNull(connectionManager, "Connection manager");
         this.threadFactory = threadFactory != null ? threadFactory : new DefaultThreadFactory("idle-connection-evictor", true);
-        final TimeValue localSleepTime = sleepTime != null ? sleepTime : TimeValue.ofSeconds(5);
+        final TimeValue localSleepTime = sleepTime != null ? sleepTime : calculateSleepTime(maxIdleTime);
         this.thread = this.threadFactory.newThread(() -> {
             try {
                 while (!Thread.currentThread().isInterrupted()) {
@@ -76,7 +80,7 @@ public IdleConnectionEvictor(final ConnPoolControl<?> connectionManager, final T
     }
 
     public IdleConnectionEvictor(final ConnPoolControl<?> connectionManager, final TimeValue maxIdleTime) {
-        this(connectionManager, null, maxIdleTime, maxIdleTime);
+        this(connectionManager, null, null, maxIdleTime);
     }
 
     public void start() {
@@ -95,4 +99,13 @@ public void awaitTermination(final Timeout timeout) throws InterruptedException
         thread.join(timeout != null ? timeout.toMilliseconds() : Long.MAX_VALUE);
     }
 
+    static TimeValue calculateSleepTime(final TimeValue maxIdleTime) {
+        if (maxIdleTime == null) {
+            return ONE_MINUTE;
+        } else {
+            final TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+            return sleepTime.compareTo(ONE_SECOND) < 0 ? ONE_SECOND : sleepTime;
+        }
+    }
+
 }

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java

@@ -37,7 +37,6 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.concurrent.ThreadFactory;
-import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
 import java.util.function.UnaryOperator;
 
@@ -163,8 +162,6 @@
  */
 public class HttpAsyncClientBuilder {
 
-    private static final TimeValue ONE_SECOND = TimeValue.ofSeconds(1L);
-
     private static class RequestInterceptorEntry {
 
         enum Position { FIRST, LAST }
@@ -1146,11 +1143,8 @@ public CloseableHttpAsyncClient build() {
             }
             if (evictExpiredConnections || evictIdleConnections) {
                 if (connManagerCopy instanceof ConnPoolControl) {
-                    final TimeValue maxIdleTimeCopy = evictIdleConnections ? maxIdleTime : null;
-                    TimeValue sleepTime = maxIdleTimeCopy != null ? maxIdleTimeCopy.divide(10, TimeUnit.NANOSECONDS) : ONE_SECOND;
-                    sleepTime = sleepTime.compareTo(ONE_SECOND) < 0 ? ONE_SECOND : sleepTime;
                     final IdleConnectionEvictor connectionEvictor = new IdleConnectionEvictor((ConnPoolControl<?>) connManagerCopy,
-                            sleepTime, maxIdleTimeCopy);
+                            null, evictIdleConnections ? maxIdleTime : null);
                     closeablesCopy.add(connectionEvictor::shutdown);
                     connectionEvictor.start();
                 }

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java

@@ -37,7 +37,6 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
 import java.util.function.UnaryOperator;
 
@@ -147,8 +146,6 @@
  */
 public class HttpClientBuilder {
 
-    private static final TimeValue ONE_SECOND = TimeValue.ofSeconds(1L);
-
     private static class RequestInterceptorEntry {
 
         enum Position { FIRST, LAST }
@@ -1115,11 +1112,8 @@ public CloseableHttpClient build() {
             }
             if (evictExpiredConnections || evictIdleConnections) {
                 if (connManagerCopy instanceof ConnPoolControl) {
-                    final TimeValue maxIdleTimeCopy = evictIdleConnections ? maxIdleTime : null;
-                    TimeValue sleepTime = maxIdleTimeCopy != null ? maxIdleTimeCopy.divide(10, TimeUnit.NANOSECONDS) : ONE_SECOND;
-                    sleepTime = sleepTime.compareTo(ONE_SECOND) < 0 ? ONE_SECOND : sleepTime;
                     final IdleConnectionEvictor connectionEvictor = new IdleConnectionEvictor((ConnPoolControl<?>) connManagerCopy,
-                            sleepTime, maxIdleTimeCopy);
+                            null, evictIdleConnections ? maxIdleTime : null);
                     closeablesCopy.add(() -> {
                         connectionEvictor.shutdown();
                         try {

httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestIdleConnectionEvictor.java

@@ -78,4 +78,12 @@ void testEvictExpiredOnly() throws Exception {
         Assertions.assertFalse(connectionEvictor.isRunning());
     }
 
+    @Test
+    void testCalculateSleepTime() throws Exception {
+        Assertions.assertEquals(TimeValue.ofMinutes(1), IdleConnectionEvictor.calculateSleepTime(null));
+        Assertions.assertEquals(TimeValue.ofSeconds(3), IdleConnectionEvictor.calculateSleepTime(TimeValue.ofSeconds(30)));
+        Assertions.assertEquals(TimeValue.ofSeconds(1), IdleConnectionEvictor.calculateSleepTime(TimeValue.ofSeconds(10)));
+        Assertions.assertEquals(TimeValue.ofSeconds(1), IdleConnectionEvictor.calculateSleepTime(TimeValue.ofMilliseconds(125)));
+    }
+
 }