Make HttpConnector retries configurable and add jitter

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,22 @@ 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,29 @@ 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 +228,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.pow(2, retries) * MIN_RETRY_DELAY_MS;
+        if (!maxRetryTimeout.isZero()) {
+            rawTimeout = Math.min(rawTimeout, 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 +244,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,7 @@ 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);
       }
     }
   }