fixup! STF-322: Add bounded transport-failure retry to WebServiceClient

Author: oschwald

src/main/java/com/maxmind/minfraud/WebServiceClient.java

@@ -17,9 +17,11 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InterruptedIOException;
+import java.net.ConnectException;
 import java.net.ProxySelector;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.net.UnknownHostException;
 import java.net.http.HttpClient;
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
@@ -31,6 +33,8 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import javax.net.ssl.SSLHandshakeException;
+import javax.net.ssl.SSLPeerUnverifiedException;
 
 /**
  * Client for MaxMind minFraud Score, Insights, and Factors
@@ -124,13 +128,8 @@ public Builder(int accountId, String licenseKey) {
         /**
          * @param val Timeout duration to establish a connection to the web service. There is no
          *            timeout by default.
-         *            <p>
-         *            When {@code maxRetries > 0}, one API call may incur up to
-         *            {@code (maxRetries + 1)} connection attempts, each subject to
-         *            {@code connectTimeout} and {@code requestTimeout}. Worst-case wall-clock
-         *            duration is roughly
-         *            {@code (maxRetries + 1) x (connectTimeout + requestTimeout)}.
          * @return Builder object
+         * @apiNote See {@link #maxRetries(int)} for how this timeout interacts with retries.
          */
         public WebServiceClient.Builder connectTimeout(Duration val) {
             connectTimeout = val;
@@ -184,14 +183,9 @@ public WebServiceClient.Builder locales(List<String> val) {
 
 
         /**
-         * @param val Request timeout duration. here is no timeout by default.
-         *            <p>
-         *            When {@code maxRetries > 0}, one API call may incur up to
-         *            {@code (maxRetries + 1)} connection attempts, each subject to
-         *            {@code connectTimeout} and {@code requestTimeout}. Worst-case wall-clock
-         *            duration is roughly
-         *            {@code (maxRetries + 1) x (connectTimeout + requestTimeout)}.
+         * @param val Request timeout duration. There is no timeout by default.
          * @return Builder object
+         * @apiNote See {@link #maxRetries(int)} for how this timeout interacts with retries.
          */
         public Builder requestTimeout(Duration val) {
             requestTimeout = val;
@@ -225,11 +219,20 @@ public Builder httpClient(HttpClient val) {
 
         /**
          * @param val Maximum number of retries on transport-level failures
-         *            (connection reset, broken pipe, connect timeout, ...).
+         *            (connection reset, broken pipe, EOF, ...).
          *            Applies uniformly to all endpoints. Defaults to 1.
          *            Set to 0 to disable.
          * @return Builder.
          * @throws IllegalArgumentException if {@code val} is negative.
+         * @apiNote Timeouts are not retried ({@link java.net.http.HttpTimeoutException},
+         *     including the connect-phase subclass
+         *     {@link java.net.http.HttpConnectTimeoutException}). When
+         *     {@code maxRetries > 0}, retries are triggered only by fast transport
+         *     failures, so each attempt is independently bounded by
+         *     {@link #connectTimeout(Duration)} and {@link #requestTimeout(Duration)}.
+         *     The multiplied worst-case wall clock a naive reading suggests is
+         *     unreachable in practice, since hitting the timeout aborts the call
+         *     rather than triggering a retry.
          */
         public Builder maxRetries(int val) {
             if (val < 0) {
@@ -386,13 +389,18 @@ private <T> T responseFor(String service, AbstractModel transaction, Class<T> cl
     private HttpResponse<InputStream> sendWithRetry(HttpRequest request)
         throws IOException, InterruptedException {
         int attempts = 0;
+        IOException prior = null;
         while (true) {
             try {
                 return httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream());
             } catch (IOException e) {
+                if (prior != null) {
+                    e.addSuppressed(prior);
+                }
                 if (!isRetriableTransportFailure(e) || attempts >= maxRetries) {
                     throw e;
                 }
+                prior = e;
                 attempts++;
             }
         }
@@ -413,11 +421,24 @@ private static boolean isRetriableTransportFailure(IOException e) {
         if (e instanceof InterruptedIOException) {
             return false;
         }
+        // Typically deterministic failures: retrying just delays surfacing the
+        // config bug without recovering the request.
+        if (e instanceof UnknownHostException) {
+            return false;
+        }
+        if (e instanceof ConnectException) {
+            return false;
+        }
+        if (e instanceof SSLHandshakeException) {
+            return false;
+        }
+        if (e instanceof SSLPeerUnverifiedException) {
+            return false;
+        }
         // Everything else from httpClient.send() is a transport failure
-        // (connection reset, broken pipe, EOF, closed channel, connect refused,
-        // DNS, TLS, protocol). HTTP 4xx and 5xx responses cannot reach this
-        // predicate -- they come back as HttpResponse objects rather than
-        // IOExceptions.
+        // (connection reset, broken pipe, EOF, closed channel, ...).
+        // HTTP 4xx and 5xx responses do not reach this predicate -- they come
+        // back as HttpResponse objects rather than IOExceptions.
         return true;
     }