Fix NPE in connection evictor setup (#774)

Builders computed evictor sleep time from maxIdleTime even when only evictExpiredConnections() was enabled, causing an NPE.

Author: arturobernalg

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

@@ -864,7 +864,7 @@ public final HttpAsyncClientBuilder evictExpiredConnections() {
      */
     public final HttpAsyncClientBuilder evictIdleConnections(final TimeValue maxIdleTime) {
         this.evictIdleConnections = true;
-        this.maxIdleTime = maxIdleTime;
+        this.maxIdleTime = Args.notNull(maxIdleTime, "Max idle time");
         return this;
     }
 
@@ -1144,10 +1144,11 @@ public CloseableHttpAsyncClient build() {
             }
             if (evictExpiredConnections || evictIdleConnections) {
                 if (connManagerCopy instanceof ConnPoolControl) {
-                    TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+                    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, maxIdleTime);
+                            sleepTime, maxIdleTimeCopy);
                     closeablesCopy.add(connectionEvictor::shutdown);
                     connectionEvictor.start();
                 }

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

@@ -778,7 +778,7 @@ public final HttpClientBuilder evictExpiredConnections() {
      */
     public final HttpClientBuilder evictIdleConnections(final TimeValue maxIdleTime) {
         this.evictIdleConnections = true;
-        this.maxIdleTime = maxIdleTime;
+        this.maxIdleTime = Args.notNull(maxIdleTime, "Max idle time");
         return this;
     }
 
@@ -1113,10 +1113,11 @@ public CloseableHttpClient build() {
             }
             if (evictExpiredConnections || evictIdleConnections) {
                 if (connManagerCopy instanceof ConnPoolControl) {
-                    TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+                    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, maxIdleTime);
+                            sleepTime, maxIdleTimeCopy);
                     closeablesCopy.add(() -> {
                         connectionEvictor.shutdown();
                         try {

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

@@ -31,10 +31,13 @@
 import org.apache.hc.client5.http.async.AsyncExecCallback;
 import org.apache.hc.client5.http.async.AsyncExecChain;
 import org.apache.hc.client5.http.async.AsyncExecChainHandler;
+import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient;
+import org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder;
 import org.apache.hc.client5.http.impl.async.HttpAsyncClients;
 import org.apache.hc.core5.http.HttpException;
 import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 class TestHttpAsyncClientBuilder {
@@ -82,4 +85,14 @@ public void execute(final HttpRequest request, final AsyncEntityProducer entityP
             chain.proceed(request, entityProducer, scope, asyncExecCallback);
         }
     }
+
+
+    @Test
+    void testEvictExpiredConnectionsDoesNotRequireMaxIdleTime() throws Exception {
+        try (final CloseableHttpAsyncClient client = HttpAsyncClientBuilder.create()
+                .evictExpiredConnections()
+                .build()) {
+            Assertions.assertNotNull(client);
+        }
+    }
 }

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

@@ -33,6 +33,7 @@
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.HttpException;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 class TestHttpClientBuilder {
@@ -66,4 +67,13 @@ public ClassicHttpResponse execute(
             return chain.proceed(request, scope);
         }
     }
+
+    @Test
+    void testEvictExpiredConnectionsDoesNotRequireMaxIdleTime() throws Exception {
+        try (final CloseableHttpClient client = HttpClientBuilder.create()
+                .evictExpiredConnections()
+                .build()) {
+            Assertions.assertNotNull(client);
+        }
+    }
 }