fix: allowing settable connection request timeout (#44592)

also defaulting to 5000

closes: #44500

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

docs/documentation/upgrading/topics/changes/changes-26_5_0.adoc

@@ -125,6 +125,15 @@ public org.keycloak.models.UserCredentialManager credentialManager() {
 }
 ----
 
+=== HTTP client connection request timeout has a default and is configurable
+
+In previous versions, the HTTP client (SPI `connections-http-client`, provider `default`) had no default for the connection request timeout, which meant unlimited time.
+In situations with long-running out-going HTTP requests this would cause pool contention.
+
+In this release, the default is 5000 milliseconds. You may adjust the default with a new provider option:
+
+- **Connection request timeout property**: `connection-request-timeout-millis` (default: `5000`)
+
 === OCSP request timeouts follow HTTP client configuration
 
 In previous versions, OCSP requests used hardcoded connection and socket timeouts of 10 seconds.

docs/guides/server/outgoinghttp.adoc

@@ -24,6 +24,9 @@ The following are the command options:
 *establish-connection-timeout-millis*::
 Maximum time in milliseconds until establishing a connection times out. Default: Not set.
 
+*connection-request-timeout-millis*::
+Maximum time in milliseconds to obtain a connection for a request. Default: 5000ms
+
 *socket-timeout-millis*::
 Maximum time of inactivity between two data packets until a socket connection times out, in milliseconds. Default: 5000ms
 

services/src/main/java/org/keycloak/connections/httpclient/DefaultHttpClientFactory.java

@@ -77,10 +77,12 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
 
     private static class InputStreamResponseHandler extends AbstractResponseHandler<InputStream> {
 
+        @Override
         public InputStream handleEntity(HttpEntity entity) throws IOException {
             return entity.getContent();
         }
 
+        @Override
         public InputStream handleResponse(HttpResponse response) throws IOException {
             return super.handleResponse(response);
         }
@@ -173,6 +175,7 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
                 if (httpClient == null) {
                     long socketTimeout = config.getLong("socket-timeout-millis", 5000L);
                     long establishConnectionTimeout = config.getLong("establish-connection-timeout-millis", -1L);
+                    long connectionRequestTimeout = config.getLong("connection-request-timeout-millis", HttpClientBuilder.DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS);
                     int maxPooledPerRoute = config.getInt("max-pooled-per-route", 64);
                     int connectionPoolSize = config.getInt("connection-pool-size", 128);
                     long connectionTTL = config.getLong("connection-ttl-millis", -1L);
@@ -206,6 +209,7 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
 
                     builder.socketTimeout(socketTimeout, TimeUnit.MILLISECONDS)
                             .establishConnectionTimeout(establishConnectionTimeout, TimeUnit.MILLISECONDS)
+                            .connectionRequestTimeout(connectionRequestTimeout, TimeUnit.MILLISECONDS)
                             .maxPooledPerRoute(maxPooledPerRoute)
                             .connectionPoolSize(connectionPoolSize)
                             .connectionTTL(connectionTTL, TimeUnit.MILLISECONDS)
@@ -320,10 +324,16 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
                 .property()
                 .name("establish-connection-timeout-millis")
                 .type("long")
-                .helpText("When trying to make an initial socket connection, what is the timeout?")
+                .helpText("Timeout when making an initial socket connection. Only effective if less than the connection-request-timeout-millis.")
                 .defaultValue(-1L)
                 .add()
                 .property()
+                .name("connection-request-timeout-millis")
+                .type("long")
+                .helpText("Timeout when trying to obtain any connection, new or pooled.")
+                .defaultValue(HttpClientBuilder.DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS)
+                .add()
+                .property()
                 .name("max-pooled-per-route")
                 .type("int")
                 .helpText("Assigns maximum connection per route value.")

services/src/main/java/org/keycloak/connections/httpclient/HttpClientBuilder.java

@@ -74,6 +74,8 @@ public class HttpClientBuilder {
         }
     }
 
+    public static long DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS = 5000L;
+
     protected KeyStore truststore;
     protected KeyStore clientKeyStore;
     protected String clientPrivateKeyPassword;
@@ -91,6 +93,8 @@ public class HttpClientBuilder {
     protected TimeUnit socketTimeoutUnits = TimeUnit.MILLISECONDS;
     protected long establishConnectionTimeout = -1;
     protected TimeUnit establishConnectionTimeoutUnits = TimeUnit.MILLISECONDS;
+    protected long connectionRequestTimeout = DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS;
+    protected TimeUnit connectionRequestTimeoutUnits = TimeUnit.MILLISECONDS;
     protected boolean disableCookies = false;
     protected ProxyMappings proxyMappings;
     protected boolean expectContinueEnabled = false;
@@ -123,6 +127,20 @@ public class HttpClientBuilder {
         return this;
     }
 
+    /**
+     * When trying to obtain a connection, what is the timeout?
+     *
+     * @param timeout
+     * @param unit
+     * @return
+     */
+    public HttpClientBuilder connectionRequestTimeout(long timeout, TimeUnit unit)
+    {
+        this.connectionRequestTimeout = timeout;
+        this.connectionRequestTimeoutUnits = unit;
+        return this;
+    }
+
     public HttpClientBuilder connectionTTL(long ttl, TimeUnit unit) {
         this.connectionTTL = ttl;
         this.connectionTTLUnit = unit;
@@ -251,6 +269,7 @@ public class HttpClientBuilder {
             RequestConfig requestConfig = RequestConfig.custom()
                     .setConnectTimeout((int) TimeUnit.MILLISECONDS.convert(establishConnectionTimeout, establishConnectionTimeoutUnits))
                     .setSocketTimeout((int) TimeUnit.MILLISECONDS.convert(socketTimeout, socketTimeoutUnits))
+                    .setConnectionRequestTimeout((int) TimeUnit.MILLISECONDS.convert(connectionRequestTimeout, connectionRequestTimeoutUnits))
                     .setExpectContinueEnabled(expectContinueEnabled).build();
 
             org.apache.http.impl.client.HttpClientBuilder builder = getApacheHttpClientBuilder()

services/src/test/java/org/keycloak/connections/httpclient/HttpClientBuilderTest.java

@@ -18,6 +18,7 @@ public class HttpClientBuilderTest {
 
         Assert.assertEquals("Default socket timeout is -1 and can be converted by TimeUnit", -1, requestConfig.getSocketTimeout());
         Assert.assertEquals("Default connect timeout is -1 and can be converted by TimeUnit", -1, requestConfig.getConnectTimeout());
+        Assert.assertEquals("Default connection request timeout is " + HttpClientBuilder.DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS, HttpClientBuilder.DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS, requestConfig.getConnectionRequestTimeout());
     }
 
     @Test
@@ -25,13 +26,15 @@ public class HttpClientBuilderTest {
         HttpClientBuilder httpClientBuilder = new HttpClientBuilder();
         httpClientBuilder
                 .socketTimeout(2, TimeUnit.SECONDS)
-                .establishConnectionTimeout(1, TimeUnit.SECONDS);
+                .establishConnectionTimeout(1, TimeUnit.SECONDS)
+                .connectionRequestTimeout(3, TimeUnit.SECONDS);
         CloseableHttpClient httpClient = httpClientBuilder.build();
 
         RequestConfig requestConfig = getRequestConfig(httpClient);
 
         Assert.assertEquals("Socket timeout is converted to milliseconds", 2000, requestConfig.getSocketTimeout());
         Assert.assertEquals("Connect timeout is converted to milliseconds", 1000, requestConfig.getConnectTimeout());
+        Assert.assertEquals("Connection request timeout is converted to milliseconds", 3000, requestConfig.getConnectionRequestTimeout());
     }
 
     @Test