diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index 219cb430b92706..ca3f462a32dbab 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/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 diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java index 2e515fc7e46164..54e29c8264fca3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java index b65f2b72ad325e..9ea337ac78b2ad 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java +++ b/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 diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java index 78b321600b503e..b9194bd73f6567 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java +++ b/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,6 +54,8 @@ 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() {} @@ -60,6 +63,14 @@ 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 urls, @@ -161,7 +172,7 @@ private HttpConnectorMultiplexer setUpConnectorMultiplexer( ExtendedEventHandler eventHandler, Map 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); diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java index 7a6904371c3d74..11611587afa0f0 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java +++ b/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> headers = new ConcurrentHashMap<>(); try (ServerSocket server = new ServerSocket(0, 1, InetAddress.getByName(null))) { - @SuppressWarnings("unused") + @SuppressWarnings("unused") Future possiblyIgnoredError = executor.submit( new Callable() { @@ -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); } } }