From c06cc18ed770df19a7a8dff55becb11b3107f9d9 Mon Sep 17 00:00:00 2001 From: Emil Kattainen Date: Tue, 24 May 2022 13:28:29 +0300 Subject: [PATCH] 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. --- .../lib/bazel/BazelRepositoryModule.java | 2 ++ .../bazel/repository/RepositoryOptions.java | 17 ++++++++++++ .../repository/downloader/HttpConnector.java | 27 ++++++++++++++----- .../repository/downloader/HttpDownloader.java | 13 ++++++++- .../downloader/HttpConnectorTest.java | 5 ++-- 5 files changed, 55 insertions(+), 9 deletions(-) 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); } } }