8216561: HttpClient: The logic of retry on connect exception is inverted

Allows retry on connect exception by default, ensuring that the second attempt takes into account the time spent in the first attempt in order to honor the connect timeout value (if present).

Reviewed-by: chegar

src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java

@@ -33,8 +33,10 @@ import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URLPermission;
 import java.security.AccessControlContext;
+import java.time.Duration;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Executor;
 import java.util.function.Function;
@@ -125,6 +127,10 @@ final class Exchange<T> {
         return request;
     }
 
+    public Optional<Duration> remainingConnectTimeout() {
+        return multi.remainingConnectTimeout();
+    }
+
     HttpClientImpl client() {
         return client;
     }

src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java

@@ -160,7 +160,7 @@ final class HttpClientImpl extends HttpClient implements Trackable {
     private final CookieHandler cookieHandler;
     private final Duration connectTimeout;
     private final Redirect followRedirects;
-    private final Optional<ProxySelector> userProxySelector;
+    private final ProxySelector userProxySelector;
     private final ProxySelector proxySelector;
     private final Authenticator authenticator;
     private final Version version;
@@ -286,12 +286,12 @@ final class HttpClientImpl extends HttpClient implements Trackable {
         connectTimeout = builder.connectTimeout;
         followRedirects = builder.followRedirects == null ?
                 Redirect.NEVER : builder.followRedirects;
-        this.userProxySelector = Optional.ofNullable(builder.proxy);
+        this.userProxySelector = builder.proxy;
-        this.proxySelector = userProxySelector
+        this.proxySelector = Optional.ofNullable(userProxySelector)
                 .orElseGet(HttpClientImpl::getDefaultProxySelector);
         if (debug.on())
             debug.log("proxySelector is %s (user-supplied=%s)",
-                      this.proxySelector, userProxySelector.isPresent());
+                      this.proxySelector, userProxySelector != null);
         authenticator = builder.authenticator;
         if (builder.version == null) {
             version = HttpClient.Version.HTTP_2;
@@ -1149,7 +1149,7 @@ final class HttpClientImpl extends HttpClient implements Trackable {
 
     @Override
     public Optional<ProxySelector> proxy() {
-        return this.userProxySelector;
+        return Optional.ofNullable(userProxySelector);
     }
 
     // Return the effective proxy that this client uses.

src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java

@@ -26,12 +26,14 @@
 package jdk.internal.net.http;
 
 import java.io.IOException;
-import java.io.UncheckedIOException;
 import java.net.ConnectException;
 import java.net.http.HttpConnectTimeoutException;
+import java.time.Duration;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.security.AccessControlContext;
+import java.util.Objects;
+import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionStage;
 import java.util.concurrent.CompletionException;
@@ -39,6 +41,7 @@ import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Flow;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Function;
 
 import java.net.http.HttpClient;
@@ -71,6 +74,7 @@ class MultiExchange<T> {
 
     private final HttpRequest userRequest; // the user request
     private final HttpRequestImpl request; // a copy of the user request
+    private final ConnectTimeoutTracker connectTimeout; // null if no timeout
     final AccessControlContext acc;
     final HttpClientImpl client;
     final HttpResponse.BodyHandler<T> responseHandler;
@@ -107,6 +111,38 @@ class MultiExchange<T> {
     // RedirectHandler
     volatile int numberOfRedirects = 0;
 
+    // This class is used to keep track of the connection timeout
+    // across retries, when a ConnectException causes a retry.
+    // In that case - we will retry the connect, but we don't
+    // want to double the timeout by starting a new timer with
+    // the full connectTimeout again.
+    // Instead we use the ConnectTimeoutTracker to return a new
+    // duration that takes into account the time spent in the
+    // first connect attempt.
+    // If however, the connection gets connected, but we later
+    // retry the whole operation, then we reset the timer before
+    // retrying (since the connection used for the second request
+    // will not necessarily be the same: it could be a new
+    // unconnected connection) - see getExceptionalCF().
+    private static final class ConnectTimeoutTracker {
+        final Duration max;
+        final AtomicLong startTime = new AtomicLong();
+        ConnectTimeoutTracker(Duration connectTimeout) {
+            this.max = Objects.requireNonNull(connectTimeout);
+        }
+
+        Duration getRemaining() {
+            long now = System.nanoTime();
+            long previous = startTime.compareAndExchange(0, now);
+            if (previous == 0 || max.isZero()) return max;
+            Duration remaining = max.minus(Duration.ofNanos(now - previous));
+            assert remaining.compareTo(max) <= 0;
+            return remaining.isNegative() ? Duration.ZERO : remaining;
+        }
+
+        void reset() { startTime.set(0); }
+    }
+
     /**
      * MultiExchange with one final response.
      */
@@ -135,7 +171,8 @@ class MultiExchange<T> {
         } else {
             pushGroup = null;
         }
-
+        this.connectTimeout = client.connectTimeout()
+                .map(ConnectTimeoutTracker::new).orElse(null);
         this.exchange = new Exchange<>(request, this);
     }
 
@@ -161,6 +198,11 @@ class MultiExchange<T> {
         this.exchange = exchange;
     }
 
+    public Optional<Duration> remainingConnectTimeout() {
+        return Optional.ofNullable(connectTimeout)
+                .map(ConnectTimeoutTracker::getRemaining);
+    }
+
     private void cancelTimer() {
         if (responseTimerEvent != null) {
             client.cancelTimer(responseTimerEvent);
@@ -355,7 +397,7 @@ class MultiExchange<T> {
         return s.isEmpty() ? true : Boolean.parseBoolean(s);
     }
 
-    private static boolean retryConnect() {
+    private static boolean disableRetryConnect() {
         String s = Utils.getNetProperty("jdk.httpclient.disableRetryConnect");
         if (s == null)
             return false;
@@ -365,7 +407,7 @@ class MultiExchange<T> {
     /** True if ALL ( even non-idempotent ) requests can be automatic retried. */
     private static final boolean RETRY_ALWAYS = retryPostValue();
     /** True if ConnectException should cause a retry. Enabled by default */
-    private static final boolean RETRY_CONNECT = retryConnect();
+    private static final boolean RETRY_CONNECT = !disableRetryConnect();
 
     /** Returns true is given request has an idempotent method. */
     private static boolean isIdempotentRequest(HttpRequest request) {
@@ -416,10 +458,13 @@ class MultiExchange<T> {
             Throwable cause = retryCause(t);
 
             if (!(t instanceof ConnectException)) {
+                // we may need to start a new connection, and if so
+                // we want to start with a fresh connect timeout again.
+                if (connectTimeout != null) connectTimeout.reset();
                 if (!canRetryRequest(currentreq)) {
                     return failedFuture(cause); // fails with original cause
                 }
-            }
+            } // ConnectException: retry, but don't reset the connectTimeout.
 
             // allow the retry mechanism to do its work
             retryCause = cause;

src/java.net.http/share/classes/jdk/internal/net/http/PlainHttpConnection.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -65,7 +65,7 @@ class PlainHttpConnection extends HttpConnection {
      */
     private ConnectTimerEvent newConnectTimer(Exchange<?> exchange,
                                               CompletableFuture<Void> cf) {
-        Duration duration = client().connectTimeout().orElse(null);
+        Duration duration = exchange.remainingConnectTimeout().orElse(null);
         if (duration != null) {
             ConnectTimerEvent cte = new ConnectTimerEvent(duration, exchange, cf);
             return cte;