Merge pull request #300 from digipost/exception-on-not-closing-stream

Exception on not closing ResponseInputStream

Author: runeflobakk

NOTICE

@@ -8,17 +8,14 @@ Licensed under Apache 2 - http://www.apache.org/licenses/LICENSE-2.0.html
 
 This software includes third party software subject to the following licenses:
 
-  Apache Commons Codec under Apache License, Version 2.0
   Apache HttpClient under Apache License, Version 2.0
   Apache HttpComponents Core HTTP/1.1 under Apache License, Version 2.0
   Apache HttpComponents Core HTTP/2 under Apache License, Version 2.0
-  Apache HttpCore under Apache License, Version 2.0
   Digipost Certificate Validator under The Apache Software License, Version 2.0
   Digipost JAXB Resolver - com.sun.xml.bind under The Apache Software License, Version 2.0
   JavaBeans Activation Framework API jar under CDDL/GPLv2+CE
   jaxb-api under CDDL 1.1 or GPL2 w/ CPE
   JAXB2 Basics - Runtime under BSD-Style License
-  JCL 1.2 implemented over SLF4J under Apache License, Version 2.0
   Old JAXB Core under CDDL+GPL License
   Old JAXB Runtime under Eclipse Distribution License - v 1.0
   Posten signering - API JAXB Classes under The Apache Software License, Version 2.0

lib/NOTICE

@@ -8,17 +8,14 @@ Licensed under Apache 2 - http://www.apache.org/licenses/LICENSE-2.0.html
 
 This software includes third party software subject to the following licenses:
 
-  Apache Commons Codec under Apache License, Version 2.0
   Apache HttpClient under Apache License, Version 2.0
   Apache HttpComponents Core HTTP/1.1 under Apache License, Version 2.0
   Apache HttpComponents Core HTTP/2 under Apache License, Version 2.0
-  Apache HttpCore under Apache License, Version 2.0
   Digipost Certificate Validator under The Apache Software License, Version 2.0
   Digipost JAXB Resolver - com.sun.xml.bind under The Apache Software License, Version 2.0
   JavaBeans Activation Framework API jar under CDDL/GPLv2+CE
   jaxb-api under CDDL 1.1 or GPL2 w/ CPE
   JAXB2 Basics - Runtime under BSD-Style License
-  JCL 1.2 implemented over SLF4J under Apache License, Version 2.0
   Old JAXB Core under CDDL+GPL License
   Old JAXB Runtime under Eclipse Distribution License - v 1.0
   Posten signering - API JAXB Classes under The Apache Software License, Version 2.0

lib/pom.xml

@@ -28,7 +28,7 @@
             <dependency>
                 <groupId>org.junit</groupId>
                 <artifactId>junit-bom</artifactId>
-                <version>5.9.2</version>
+                <version>5.10.0</version>
                 <type>pom</type>
                 <scope>import</scope>
             </dependency>
@@ -55,7 +55,7 @@
         <dependency>
             <groupId>no.digipost</groupId>
             <artifactId>certificate-validator</artifactId>
-            <version>2.6</version>
+            <version>3.0.2</version>
             <exclusions>
                 <exclusion>
                     <groupId>org.bouncycastle</groupId>
@@ -72,26 +72,26 @@
         <dependency>
             <groupId>org.apache.httpcomponents.core5</groupId>
             <artifactId>httpcore5</artifactId>
-            <version>5.2.1</version>
+            <version>5.2.2</version>
         </dependency>
 
         <dependency>
             <groupId>commons-io</groupId>
             <artifactId>commons-io</artifactId>
-            <version>2.11.0</version>
+            <version>2.13.0</version>
             <scope>test</scope>
         </dependency>
 
         <dependency>
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-api</artifactId>
-            <version>2.0.6</version>
+            <version>2.0.9</version>
             <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-simple</artifactId>
-            <version>2.0.6</version>
+            <version>2.0.9</version>
             <scope>test</scope>
         </dependency>
 
@@ -124,7 +124,7 @@
         <dependency>
             <groupId>nl.jqno.equalsverifier</groupId>
             <artifactId>equalsverifier</artifactId>
-            <version>3.14</version>
+            <version>3.15.1</version>
             <scope>test</scope>
         </dependency>
         <dependency>
@@ -155,7 +155,7 @@
         <dependency>
             <groupId>com.github.tomakehurst</groupId>
             <artifactId>wiremock-jre8</artifactId>
-            <version>2.35.0</version>
+            <version>2.35.1</version>
             <scope>test</scope>
             <exclusions>
                 <exclusion>
@@ -188,7 +188,7 @@
                 <plugin>
                     <groupId>com.github.siom79.japicmp</groupId>
                     <artifactId>japicmp-maven-plugin</artifactId>
-                    <version>0.15.3</version>
+                    <version>0.17.3</version>
                     <configuration>
                         <parameter>
                             <includes>
@@ -202,7 +202,7 @@
                 </plugin>
                 <plugin>
                     <artifactId>maven-shade-plugin</artifactId>
-                    <version>3.4.1</version>
+                    <version>3.5.0</version>
                     <configuration>
                         <minimizeJar>true</minimizeJar>
                         <artifactSet>
@@ -228,11 +228,11 @@
                 </plugin>
                 <plugin>
                     <artifactId>maven-surefire-plugin</artifactId>
-                    <version>3.0.0</version>
+                    <version>3.1.2</version>
                 </plugin>
                 <plugin>
                     <artifactId>maven-resources-plugin</artifactId>
-                    <version>3.3.0</version>
+                    <version>3.3.1</version>
                 </plugin>
                 <plugin>
                     <artifactId>maven-javadoc-plugin</artifactId>
@@ -244,7 +244,7 @@
                 </plugin>
                 <plugin>
                     <artifactId>maven-enforcer-plugin</artifactId>
-                    <version>3.2.1</version>
+                    <version>3.4.1</version>
                     <configuration>
                         <rules>
                             <bannedDependencies>

lib/src/main/java/no/digipost/signature/client/ClientConfiguration.java

@@ -238,6 +238,29 @@ public Builder timeoutsForDocumentDownloads(Consumer<? super TimeoutsConfigurer>
         }
 
 
+        /**
+         * Configure the pool used by the client to manage connections
+         * used for integrating with the API.
+         *
+         * @param connectionPool the {@link ConnectionPoolConfigurer connection pool configuration API}
+         */
+        public Builder connectionPool(Consumer<? super ConnectionPoolConfigurer> connectionPool) {
+            connectionPool.accept(defaultHttpClientConfigurer);
+            return this;
+        }
+
+
+        /**
+         * Configure the pool used by the client to manage connections
+         * used specifically for downloading documents.
+         *
+         * @param connectionPool the {@link ConnectionPoolConfigurer connection pool configuration API}
+         */
+        public Builder connectionPoolForDocumentDownloads(Consumer<? super ConnectionPoolConfigurer> connectionPool) {
+            connectionPool.accept(httpClientForDocumentDownloadsConfigurer);
+            return this;
+        }
+
         /**
          * This method allows for custom configuration of the created {@link HttpClient} if anything is
          * needed that is not already supported by other methods in {@link ClientConfiguration}.
@@ -333,7 +356,8 @@ public Builder serverCertificateTrustStrategy(CertificateChainValidation certifi
          * Allows for overriding which {@link Clock} is used to convert between Java and XML,
          * may be useful for e.g. automated tests.
          * <p>
-         * Uses {@link Clock#systemDefaultZone() the best available system clock} if not specified.
+         * Uses the {@link Clock#systemDefaultZone() system clock with default time zone}
+         * if not specified.
          */
         public Builder clock(Clock clock) {
             this.clock = clock;

lib/src/main/java/no/digipost/signature/client/ConnectionPoolConfigurer.java

@@ -0,0 +1,26 @@
+package no.digipost.signature.client;
+
+/**
+ * Allows configuring aspects of the connection pool
+ * used by the internal HTTP client.
+ */
+public interface ConnectionPoolConfigurer {
+
+    /**
+     * The default amount of connections in the connection pool is
+     * {@value #DEFAULT_TOTAL_CONNECTIONS_IN_POOL}.
+     * This may be overridden using {@link #maxTotalConnectionsInPool(int)}
+     */
+    int DEFAULT_TOTAL_CONNECTIONS_IN_POOL = 10;
+
+
+    /**
+     * Set the amount of connections in the connection pool, if the default
+     * of {@value #DEFAULT_TOTAL_CONNECTIONS_IN_POOL} is not applicable for
+     * your integration.
+     *
+     * @param count the amount of connections in the connection pool
+     */
+    ConnectionPoolConfigurer maxTotalConnectionsInPool(int count);
+
+}

lib/src/main/java/no/digipost/signature/client/core/exceptions/HttpIOException.java

@@ -16,9 +16,14 @@ public HttpIOException(HttpResponse response, IOException cause) {
     }
 
     public HttpIOException(HttpRequest request, IOException cause) {
+        this(request, null, cause);
+    }
+
+    public HttpIOException(HttpRequest request, String message, IOException cause) {
         this(
                 "Error executing " + request + ", because " +
-                cause.getClass().getSimpleName() + " '" + cause.getMessage() + "'",
+                cause.getClass().getSimpleName() + " '" + cause.getMessage() + "'" +
+                (message != null ? ". " + message : ""),
                 cause);
     }
 

lib/src/main/java/no/digipost/signature/client/core/internal/DownloadHelper.java

@@ -7,6 +7,7 @@
 import org.apache.hc.client5.http.classic.HttpClient;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
+import org.apache.hc.core5.http.ConnectionRequestTimeoutException;
 import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
 import org.apache.hc.core5.http.message.BasicHeader;
@@ -53,6 +54,16 @@ public ResponseInputStream getDataStream(URI absoluteUri, ContentType ... accept
             try {
                 try {
                     response = httpClient.executeOpen(null, request, null);
+                } catch (ConnectionRequestTimeoutException connectionRequestTimeoutException) {
+                    throw new HttpIOException(request,
+                            "This happens when an HTTP connection could not be obtained from the connection pool, and " +
+                            "is likely because of missing resource management of responses when downloading data, " +
+                            "e.g. signed documents (PAdESes). Please verify your implemention guarantees that InputStreams " +
+                            "obtained from the client library are properly handled and closed, e.g. using try-with-resources " +
+                            "in Java or .use { .. } in Kotlin. This attention is only necessary for data streams, and not " +
+                            "for methods yielding regular Java objects, where parsing and closing the API responses is " +
+                            "the internal responsibility of the client library.",
+                            connectionRequestTimeoutException);
                 } catch (IOException e) {
                     throw new HttpIOException(request, e);
                 }

lib/src/main/java/no/digipost/signature/client/core/internal/configuration/ApacheHttpClientBuilderConfigurer.java

@@ -1,6 +1,7 @@
 package no.digipost.signature.client.core.internal.configuration;
 
 import no.digipost.signature.client.ApacheHttpClientConfigurer;
+import no.digipost.signature.client.ConnectionPoolConfigurer;
 import no.digipost.signature.client.TimeoutsConfigurer;
 import org.apache.hc.client5.http.config.ConnectionConfig;
 import org.apache.hc.client5.http.config.RequestConfig;
@@ -11,9 +12,10 @@
 import java.time.Duration;
 import java.util.function.Consumer;
 
-public final class ApacheHttpClientBuilderConfigurer implements TimeoutsConfigurer, ApacheHttpClientConfigurer, Configurer<HttpClientBuilder> {
+public final class ApacheHttpClientBuilderConfigurer implements TimeoutsConfigurer, ConnectionPoolConfigurer, ApacheHttpClientConfigurer, Configurer<HttpClientBuilder> {
 
     private final PoolingHttpClientConnectionManagerBuilder connectionManagerConfig = PoolingHttpClientConnectionManagerBuilder.create();
+
     private final SocketConfig.Builder socketConfig = SocketConfig.custom();
     private final ConnectionConfig.Builder connectionConfig = ConnectionConfig.custom();
     private final RequestConfig.Builder requestConfig = RequestConfig.custom();
@@ -26,12 +28,16 @@ public final class ApacheHttpClientBuilderConfigurer implements TimeoutsConfigur
             .setDefaultRequestConfig(requestConfig.build())
             .setConnectionManager(connectionManagerConfig.build());
 
-    private Configurer<? super PoolingHttpClientConnectionManagerBuilder> additionalConnectionManagerConfigurer = Configurer.notConfigured();
-    private Configurer<? super HttpClientBuilder> additionalHttpClientConfigurer = Configurer.notConfigured();
+    private Configurer<PoolingHttpClientConnectionManagerBuilder> additionalConnectionManagerConfigurer = Configurer.notConfigured();
+    private Configurer<HttpClientBuilder> additionalHttpClientConfigurer = Configurer.notConfigured();
+
 
+    public ApacheHttpClientBuilderConfigurer() {
+        maxTotalConnectionsInPool(DEFAULT_TOTAL_CONNECTIONS_IN_POOL);
+    }
 
     public ApacheHttpClientBuilderConfigurer connectionManager(Configurer<PoolingHttpClientConnectionManagerBuilder> connectionManagerConfigurer) {
-        this.additionalConnectionManagerConfigurer = connectionManagerConfigurer;
+        this.additionalConnectionManagerConfigurer = additionalConnectionManagerConfigurer.andThen(connectionManagerConfigurer);
         return this;
     }
 
@@ -59,6 +65,16 @@ public ApacheHttpClientBuilderConfigurer responseArrivalTimeout(Duration duratio
         return this;
     }
 
+    @Override
+    public ApacheHttpClientBuilderConfigurer maxTotalConnectionsInPool(int count) {
+        return connectionManager(connectionMgr -> connectionMgr
+
+                // a "route" is essentially the target host (optionally with proxy hops). The HTTP client
+                // is specifically for one API, and separating connections per route is not applicable,
+                // therefore the per route count is the same as total count of connections
+                .setMaxConnPerRoute(count).setMaxConnTotal(count));
+    }
+
     @Override
     public ApacheHttpClientBuilderConfigurer configure(
             Consumer<? super SocketConfig.Builder> socketConfig,
@@ -73,7 +89,7 @@ public ApacheHttpClientBuilderConfigurer configure(
 
     @Override
     public ApacheHttpClientBuilderConfigurer configure(Consumer<? super HttpClientBuilder> httpClientCustomizer) {
-        this.additionalHttpClientConfigurer = Configurer.of(httpClientCustomizer);
+        this.additionalHttpClientConfigurer = additionalHttpClientConfigurer.andThen(Configurer.of(httpClientCustomizer));
         return this;
     }
 

lib/src/test/java/no/digipost/signature/client/portal/PortalClientConnectionPoolTest.java

@@ -0,0 +1,73 @@
+package no.digipost.signature.client.portal;
+
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.junit5.WireMockTest;
+import no.digipost.signature.client.ClientConfiguration;
+import no.digipost.signature.client.ServiceEnvironment;
+import no.digipost.signature.client.core.PAdESReference;
+import no.digipost.signature.client.core.Sender;
+import no.digipost.signature.client.core.exceptions.HttpIOException;
+import org.apache.hc.core5.http.ConnectionRequestTimeoutException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+
+import java.net.URI;
+import java.util.stream.Stream;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.get;
+import static com.github.tomakehurst.wiremock.client.WireMock.givenThat;
+import static com.github.tomakehurst.wiremock.client.WireMock.ok;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching;
+import static com.github.tomakehurst.wiremock.client.WireMock.verify;
+import static com.github.tomakehurst.wiremock.http.RequestMethod.GET;
+import static com.github.tomakehurst.wiremock.matching.RequestPatternBuilder.newRequestPattern;
+import static com.github.tomakehurst.wiremock.matching.UrlPattern.ANY;
+import static java.time.Duration.ofMillis;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static no.digipost.signature.client.ServiceEnvironment.STAGING;
+import static no.digipost.signature.client.TestKonfigurasjon.CLIENT_KEYSTORE;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsStringIgnoringCase;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static uk.co.probablyfine.matchers.Java8Matchers.where;
+
+@WireMockTest
+class PortalClientConnectionPoolTest {
+
+    private final ServiceEnvironment unitTestEnv;
+    private final ClientConfiguration.Builder configBuilder;
+
+    PortalClientConnectionPoolTest(WireMockRuntimeInfo wireMockInfo) {
+        this.unitTestEnv = STAGING.withServiceUrl(URI.create(wireMockInfo.getHttpBaseUrl()));
+        this.configBuilder = ClientConfiguration.builder(CLIENT_KEYSTORE)
+                .serviceEnvironment(unitTestEnv)
+                .defaultSender(new Sender("123456789"));
+    }
+
+    @Test
+    @Timeout(value = 3, unit = SECONDS)
+    void exhaustingConnectionPoolWhenMissingResourceManagement() {
+
+        int connectionPoolSize = 15;
+        PortalClient client = new PortalClient(configBuilder
+                .timeoutsForDocumentDownloads(t -> t.connectionRequestTimeout(ofMillis(100)))
+                .connectionPoolForDocumentDownloads(pool -> pool.maxTotalConnectionsInPool(connectionPoolSize))
+                .build());
+
+        givenThat(get(ANY).willReturn(ok()));
+
+        PAdESReference padesReference = PAdESReference.of(URI.create(unitTestEnv.signatureServiceRootUrl() + "/pades"));
+        HttpIOException thrown = assertThrows(HttpIOException.class, () -> Stream.generate(() -> padesReference).forEach(client::getPAdES));
+
+        assertAll(
+                () -> assertThat(thrown, where(Throwable::getCause, instanceOf(ConnectionRequestTimeoutException.class))),
+                () -> assertThat(thrown, where(Throwable::getMessage, containsStringIgnoringCase("could not be obtained from the connection pool"))),
+                () -> assertThat(thrown, where(Throwable::getMessage, containsStringIgnoringCase("missing resource management of responses")))
+            );
+
+        verify(connectionPoolSize, newRequestPattern(GET, urlPathMatching(".*/pades$")));
+    }
+
+}