Make HttpConnector retries configurable and add jitter

HttpConnector has some hard coded values for maximum number of attempts and maximum timeout between retries. Make these configurable by command line flags. Also add jitter to those retries.

Closes #17228.

PiperOrigin-RevId: 508588893
Change-Id: I345d6000d2131e9a182433fc420440e127e0650e

src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java

@@ -385,6 +385,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
             .handle(Event.warn("Ignoring request to scale http timeouts by a non-positive factor"));
         httpDownloader.setTimeoutScaling(1.0f);
       }
+      httpDownloader.setMaxAttempts(repoOptions.httpConnectorAttempts);
+      httpDownloader.setMaxRetryTimeout(repoOptions.httpConnectorRetryMaxTimeout);
 
       if (repoOptions.repositoryOverrides != null) {
         // To get the usual latest-wins semantics, we need a mutable map, as the builder

src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java

@@ -27,6 +27,7 @@
 import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParsingException;
+import java.time.Duration;
 import java.util.List;
 
 /** Command-line options for repositories. */
@@ -124,6 +125,24 @@ public class RepositoryOptions extends OptionsBase {
       help = "Scale all timeouts related to http downloads by the given factor")
   public double httpTimeoutScaling;
 
+  @Option(
+      name = "http_connector_attempts",
+      defaultValue = "8",
+      documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
+      effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+      help = "The maximum number of attempts for http downloads.")
+  public int httpConnectorAttempts;
+
+  @Option(
+      name = "http_connector_retry_max_timeout",
+      defaultValue = "0s",
+      documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
+      effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+      help =
+          "The maximum timeout for http download retries. With a value of 0, no timeout maximum is"
+              + " defined.")
+  public Duration httpConnectorRetryMaxTimeout;
+
   @Option(
       name = "override_repository",
       defaultValue = "null",

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java

@@ -20,7 +20,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.io.ByteStreams;
-import com.google.common.math.IntMath;
 import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
@@ -35,6 +34,7 @@
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.UnknownHostException;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
@@ -52,7 +52,7 @@
 @ThreadSafe
 class HttpConnector {
 
-  private static final int MAX_RETRIES = 8;
+  private static final int MAX_ATTEMPTS = 8;
   private static final int MAX_REDIRECTS = 40;
   private static final int MIN_RETRY_DELAY_MS = 100;
   private static final int MIN_CONNECT_TIMEOUT_MS = 1000;
@@ -68,18 +68,33 @@ class HttpConnector {
   private final ProxyHelper proxyHelper;
   private final Sleeper sleeper;
   private final float timeoutScaling;
+  private final int maxAttempts;
+  private final Duration maxRetryTimeout;
 
   HttpConnector(
       Locale locale,
       EventHandler eventHandler,
       ProxyHelper proxyHelper,
       Sleeper sleeper,
-      float timeoutScaling) {
+      float timeoutScaling,
+      int maxAttempts,
+      Duration maxRetryTimeout) {
     this.locale = locale;
     this.eventHandler = eventHandler;
     this.proxyHelper = proxyHelper;
     this.sleeper = sleeper;
     this.timeoutScaling = timeoutScaling;
+    this.maxAttempts = maxAttempts > 0 ? maxAttempts : MAX_ATTEMPTS;
+    this.maxRetryTimeout = maxRetryTimeout;
+  }
+
+  HttpConnector(
+      Locale locale,
+      EventHandler eventHandler,
+      ProxyHelper proxyHelper,
+      Sleeper sleeper,
+      float timeoutScaling) {
+    this(locale, eventHandler, proxyHelper, sleeper, timeoutScaling, 0, Duration.ZERO);
   }
 
   HttpConnector(
@@ -217,8 +232,12 @@ URLConnection connect(
         }
         // We don't respect the Retry-After header (RFC7231 § 7.1.3) because it's rarely used and
         // tends to be too conservative when it is. We're already being good citizens by using
-        // exponential backoff. Furthermore RFC law didn't use the magic word "MUST".
-        int timeout = IntMath.pow(2, retries) * MIN_RETRY_DELAY_MS;
+        // exponential backoff with jitter. Furthermore RFC law didn't use the magic word "MUST".
+        double rawTimeout = Math.scalb(MIN_RETRY_DELAY_MS, retries);
+        if (!maxRetryTimeout.isZero()) {
+          rawTimeout = Math.min(rawTimeout, (double) maxRetryTimeout.toMillis());
+        }
+        int timeout = (int) ((0.75 + Math.random() / 2) * rawTimeout);
         if (e instanceof SocketTimeoutException) {
           eventHandler.handle(Event.progress("Timeout connecting to " + url));
           connectTimeout = Math.min(connectTimeout * 2, scale(MAX_CONNECT_TIMEOUT_MS));
@@ -229,7 +248,7 @@ URLConnection connect(
           // Please note that SocketTimeoutException is a subtype of InterruptedIOException.
           throw e;
         }
-        if (++retries == MAX_RETRIES) {
+        if (++retries == maxAttempts) {
           if (e instanceof SocketTimeoutException) {
             // SocketTimeoutExceptions are InterruptedIOExceptions; however they do not signify
             // an external interruption, but simply a failed download due to some server timing

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java

@@ -33,6 +33,7 @@
 import java.io.OutputStream;
 import java.net.SocketTimeoutException;
 import java.net.URL;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
@@ -53,13 +54,23 @@ public class HttpDownloader implements Downloader {
   private static final Locale LOCALE = Locale.getDefault();
 
   private float timeoutScaling = 1.0f;
+  private int maxAttempts = 0;
+  private Duration maxRetryTimeout = Duration.ZERO;
 
   public HttpDownloader() {}
 
   public void setTimeoutScaling(float timeoutScaling) {
     this.timeoutScaling = timeoutScaling;
   }
 
+  public void setMaxAttempts(int maxAttempts) {
+    this.maxAttempts = maxAttempts;
+  }
+
+  public void setMaxRetryTimeout(Duration maxRetryTimeout) {
+    this.maxRetryTimeout = maxRetryTimeout;
+  }
+
   @Override
   public void download(
       List<URL> urls,
@@ -161,7 +172,14 @@ private HttpConnectorMultiplexer setUpConnectorMultiplexer(
       ExtendedEventHandler eventHandler, Map<String, String> clientEnv) {
     ProxyHelper proxyHelper = new ProxyHelper(clientEnv);
     HttpConnector connector =
-        new HttpConnector(LOCALE, eventHandler, proxyHelper, SLEEPER, timeoutScaling);
+        new HttpConnector(
+            LOCALE,
+            eventHandler,
+            proxyHelper,
+            SLEEPER,
+            timeoutScaling,
+            maxAttempts,
+            maxRetryTimeout);
     ProgressInputStream.Factory progressInputStreamFactory =
         new ProgressInputStream.Factory(LOCALE, CLOCK, eventHandler);
     HttpStream.Factory httpStreamFactory = new HttpStream.Factory(progressInputStreamFactory);

src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java

@@ -126,7 +126,7 @@ public void badHost_throwsIOException() throws Exception {
   public void normalRequest() throws Exception {
     final Map<String, List<String>> headers = new ConcurrentHashMap<>();
     try (ServerSocket server = new ServerSocket(0, 1, InetAddress.getByName(null))) {
-      @SuppressWarnings("unused") 
+      @SuppressWarnings("unused")
       Future<?> possiblyIgnoredError =
           executor.submit(
               new Callable<Object>() {
@@ -208,7 +208,8 @@ public Object call() throws Exception {
                   .getInputStream(),
               ISO_8859_1)) {
         assertThat(CharStreams.toString(payload)).isEqualTo("hello");
-        assertThat(clock.currentTimeMillis()).isEqualTo(100L);
+        assertThat(clock.currentTimeMillis()).isGreaterThan(50L);
+        assertThat(clock.currentTimeMillis()).isLessThan(150L);
       }
     }
   }