From f29db1bf54ffb5c49b3027d3ebe762fcfd10ee1b Mon Sep 17 00:00:00 2001 From: Emil Kattainen Date: Fri, 10 Feb 2023 00:45:39 -0800 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. Closes #17228. PiperOrigin-RevId: 508588893 Change-Id: I345d6000d2131e9a182433fc420440e127e0650e --- .../lib/bazel/BazelRepositoryModule.java | 2 ++ .../bazel/repository/RepositoryOptions.java | 19 ++++++++++++ .../repository/downloader/HttpConnector.java | 31 +++++++++++++++---- .../repository/downloader/HttpDownloader.java | 20 +++++++++++- .../downloader/HttpConnectorTest.java | 5 +-- 5 files changed, 68 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 02f013fa014eb9..f8fcd1e02c6c02 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,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", 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..2a89239e80258f 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,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 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..b3d15c5ed4804d 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,14 @@ 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); } } }