Fallback to next urls if download fails in HttpDownloader
Fixes #8974.
`HttpDownloader` never retried `IOException` that could have occurred during `ByteStreams.copy(payload, out)`, HttpConnector would have retried connection phase errors but not payload ones.
This chageset adds fallback to next url(s) if present for both payload read errors and connection errors and adds (completely) missing tests for `HttpDownloader`.
Note, that this PR technically disables questionable `HttpConnectorMultiplexer` optimization that attempts to connect to multiple urls at the same time (by starting threads that race with each other) and picking the one that connected faster. There is a way to keep that optimization while falling back to next urls, but it would require all exceptions to contain url that caused it so that `HttpDownloader` could retry download on other urls. I think making `HttpDownloader` more reliable and actually using provided `urls` as fallback is much more important than mentioned optimization.
Closes #9015.
PiperOrigin-RevId: 261702678
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 5efe7b5..90a5918 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
@@ -18,6 +18,7 @@
import com.google.common.base.Optional;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
import com.google.common.io.ByteStreams;
import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache;
import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType;
@@ -35,8 +36,11 @@
import java.io.IOException;
import java.io.InterruptedIOException;
import java.io.OutputStream;
+import java.net.SocketTimeoutException;
import java.net.URI;
import java.net.URL;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -199,21 +203,60 @@
HttpConnectorMultiplexer multiplexer =
new HttpConnectorMultiplexer(eventHandler, connector, httpStreamFactory, clock, sleeper);
- // Connect to the best mirror and download the file, while reporting progress to the CLI.
- semaphore.acquire();
+ // Iterate over urls and download the file falling back to the next url if previous failed,
+ // while reporting progress to the CLI.
boolean success = false;
- try (HttpStream payload = multiplexer.connect(urls, checksum, authHeaders);
- OutputStream out = destination.getOutputStream()) {
- ByteStreams.copy(payload, out);
- success = true;
- } catch (InterruptedIOException e) {
- throw new InterruptedException();
- } catch (IOException e) {
- throw new IOException(
- "Error downloading " + urls + " to " + destination + ": " + e.getMessage());
- } finally {
- semaphore.release();
- eventHandler.post(new FetchEvent(urls.get(0).toString(), success));
+
+ List<IOException> ioExceptions = ImmutableList.of();
+
+ for (URL url : urls) {
+ semaphore.acquire();
+
+ try (HttpStream payload =
+ multiplexer.connect(Collections.singletonList(url), checksum, authHeaders);
+ OutputStream out = destination.getOutputStream()) {
+ try {
+ ByteStreams.copy(payload, out);
+ } catch (SocketTimeoutException e) {
+ // SocketTimeoutExceptions are InterruptedIOExceptions; however they do not signify
+ // an external interruption, but simply a failed download due to some server timing
+ // out. So rethrow them as ordinary IOExceptions.
+ throw new IOException(e);
+ }
+ success = true;
+ break;
+ } catch (InterruptedIOException e) {
+ throw new InterruptedException(e.getMessage());
+ } catch (IOException e) {
+ if (ioExceptions.isEmpty()) {
+ ioExceptions = new ArrayList<>(1);
+ }
+ ioExceptions.add(e);
+ eventHandler.handle(
+ Event.warn("Download from " + url + " failed: " + e.getClass() + " " + e.getMessage()));
+ continue;
+ } finally {
+ semaphore.release();
+ eventHandler.post(new FetchEvent(url.toString(), success));
+ }
+ }
+
+ if (!success) {
+ final IOException exception =
+ new IOException(
+ "Error downloading "
+ + urls
+ + " to "
+ + destination
+ + (ioExceptions.isEmpty()
+ ? ""
+ : ": " + Iterables.getLast(ioExceptions).getMessage()));
+
+ for (IOException cause : ioExceptions) {
+ exception.addSuppressed(cause);
+ }
+
+ throw exception;
}
if (isCachingByProvidedChecksum) {
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/BUILD
index e5873d4..9d2f046 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/BUILD
@@ -21,6 +21,7 @@
"//src/main/java/com/google/devtools/build/lib:util",
"//src/main/java/com/google/devtools/build/lib/bazel/repository/cache",
"//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
+ "//src/main/java/com/google/devtools/build/lib/vfs",
"//src/test/java/com/google/devtools/build/lib:foundations_testutil",
"//src/test/java/com/google/devtools/build/lib:test_runner",
"//src/test/java/com/google/devtools/build/lib:testutil",
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloaderTestSuite.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloaderTestSuite.java
index 6a479c2..60b07e5 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloaderTestSuite.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloaderTestSuite.java
@@ -25,6 +25,7 @@
HttpConnectorMultiplexerIntegrationTest.class,
HttpConnectorMultiplexerTest.class,
HttpConnectorTest.class,
+ HttpDownloaderTest.class,
HttpStreamTest.class,
HttpUtilsTest.class,
ProgressInputStreamTest.class,
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerIntegrationTest.java
index f5dccf87..3b4c5af 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerIntegrationTest.java
@@ -73,7 +73,7 @@
private final Sleeper sleeper = mock(Sleeper.class);
private final Locale locale = Locale.US;
private final HttpConnector connector =
- new HttpConnector(locale, eventHandler, proxyHelper, sleeper);
+ new HttpConnector(locale, eventHandler, proxyHelper, sleeper, 0.1f);
private final ProgressInputStream.Factory progressInputStreamFactory =
new ProgressInputStream.Factory(locale, clock, eventHandler);
private final HttpStream.Factory httpStreamFactory =
@@ -102,7 +102,7 @@
try (ServerSocket server1 = new ServerSocket(0, 1, InetAddress.getByName(null));
ServerSocket server2 = new ServerSocket(0, 1, InetAddress.getByName(null))) {
for (final ServerSocket server : asList(server1, server2)) {
- @SuppressWarnings("unused")
+ @SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
executor.submit(
new Callable<Object>() {
@@ -241,4 +241,52 @@
HELLO_SHA256);
}
}
+
+ @Test
+ public void firstUrlSocketTimeout_secondOk() throws Exception {
+ final Phaser phaser = new Phaser(3);
+ try (ServerSocket server1 = new ServerSocket(0, 1, InetAddress.getByName(null));
+ ServerSocket server2 = new ServerSocket(0, 1, InetAddress.getByName(null))) {
+
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ executor.submit(
+ () -> {
+ phaser.arriveAndAwaitAdvance();
+ try (Socket socket = server1.accept()) {
+ // Do nothing to cause SocketTimeoutException on client side.
+ }
+ return null;
+ });
+
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError2 =
+ executor.submit(
+ () -> {
+ phaser.arriveAndAwaitAdvance();
+ try (Socket socket = server2.accept()) {
+ readHttpRequest(socket.getInputStream());
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "",
+ "hello");
+ }
+ return null;
+ });
+
+ phaser.arriveAndAwaitAdvance();
+ phaser.arriveAndDeregister();
+ try (HttpStream stream =
+ multiplexer.connect(
+ ImmutableList.of(
+ new URL(String.format("http://localhost:%d", server1.getLocalPort())),
+ new URL(String.format("http://localhost:%d", server2.getLocalPort()))),
+ HELLO_SHA256)) {
+ assertThat(toByteArray(stream)).isEqualTo("hello".getBytes(US_ASCII));
+ }
+ }
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java
new file mode 100644
index 0000000..e6ce8fb
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java
@@ -0,0 +1,337 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.bazel.repository.downloader;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.bazel.repository.downloader.DownloaderTestUtils.sendLines;
+import static com.google.devtools.build.lib.bazel.repository.downloader.HttpParser.readHttpRequest;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import com.google.common.base.Optional;
+import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.devtools.build.lib.vfs.DigestHashFunction.DefaultHashFunctionNotSetException;
+import com.google.devtools.build.lib.vfs.JavaIoFileSystem;
+import com.google.devtools.build.lib.vfs.Path;
+import java.io.DataInputStream;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketTimeoutException;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import org.junit.After;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.Timeout;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link HttpDownloader} */
+@RunWith(JUnit4.class)
+public class HttpDownloaderTest {
+
+ @Rule public final TemporaryFolder workingDir = new TemporaryFolder();
+
+ @Rule public final Timeout timeout = new Timeout(30, SECONDS);
+
+ private final RepositoryCache repositoryCache = mock(RepositoryCache.class);
+ private final HttpDownloader httpDownloader = new HttpDownloader(repositoryCache);
+
+ private final ExecutorService executor = Executors.newFixedThreadPool(2);
+ private final ExtendedEventHandler eventHandler = mock(ExtendedEventHandler.class);
+ private final JavaIoFileSystem fs;
+
+ public HttpDownloaderTest() throws DefaultHashFunctionNotSetException {
+ try {
+ DigestHashFunction.setDefault(DigestHashFunction.SHA256);
+ } catch (DigestHashFunction.DefaultAlreadySetException e) {
+ // Do nothing.
+ }
+ fs = new JavaIoFileSystem();
+
+ // Scale timeouts down to make tests fast.
+ httpDownloader.setTimeoutScaling(0.1f);
+ }
+
+ @After
+ public void after() {
+ executor.shutdown();
+ }
+
+ @Test
+ public void downloadFrom1UrlOk() throws IOException, InterruptedException {
+ try (ServerSocket server = new ServerSocket(0, 1, InetAddress.getByName(null))) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ executor.submit(
+ () -> {
+ try (Socket socket = server.accept()) {
+ readHttpRequest(socket.getInputStream());
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "Content-Type: text/plain",
+ "Content-Length: 5",
+ "",
+ "hello");
+ }
+ return null;
+ });
+
+ Path resultingFile =
+ httpDownloader.download(
+ Collections.singletonList(
+ new URL(String.format("http://localhost:%d/foo", server.getLocalPort()))),
+ Collections.emptyMap(),
+ Optional.absent(),
+ "testCanonicalId",
+ Optional.absent(),
+ fs.getPath(workingDir.newFile().getAbsolutePath()),
+ eventHandler,
+ Collections.emptyMap(),
+ "testRepo");
+
+ assertThat(new String(readFile(resultingFile), UTF_8)).isEqualTo("hello");
+ }
+ }
+
+ @Test
+ public void downloadFrom2UrlsFirstOk() throws IOException, InterruptedException {
+ try (ServerSocket server1 = new ServerSocket(0, 1, InetAddress.getByName(null));
+ ServerSocket server2 = new ServerSocket(0, 1, InetAddress.getByName(null))) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ executor.submit(
+ () -> {
+ while (!executor.isShutdown()) {
+ try (Socket socket = server1.accept()) {
+ readHttpRequest(socket.getInputStream());
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "Content-Type: text/plain",
+ "",
+ "content1");
+ }
+ }
+ return null;
+ });
+
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError2 =
+ executor.submit(
+ () -> {
+ while (!executor.isShutdown()) {
+ try (Socket socket = server2.accept()) {
+ readHttpRequest(socket.getInputStream());
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "Content-Type: text/plain",
+ "",
+ "content2");
+ }
+ }
+ return null;
+ });
+
+ final List<URL> urls = new ArrayList<>(2);
+ urls.add(new URL(String.format("http://localhost:%d/foo", server1.getLocalPort())));
+ urls.add(new URL(String.format("http://localhost:%d/foo", server2.getLocalPort())));
+
+ Path resultingFile =
+ httpDownloader.download(
+ urls,
+ Collections.emptyMap(),
+ Optional.absent(),
+ "testCanonicalId",
+ Optional.absent(),
+ fs.getPath(workingDir.newFile().getAbsolutePath()),
+ eventHandler,
+ Collections.emptyMap(),
+ "testRepo");
+
+ assertThat(new String(readFile(resultingFile), UTF_8)).isEqualTo("content1");
+ }
+ }
+
+ @Test
+ public void downloadFrom2UrlsFirstSocketTimeoutOnBodyReadSecondOk()
+ throws IOException, InterruptedException {
+ try (ServerSocket server1 = new ServerSocket(0, 1, InetAddress.getByName(null));
+ ServerSocket server2 = new ServerSocket(0, 1, InetAddress.getByName(null))) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ executor.submit(
+ () -> {
+ Socket socket = server1.accept();
+ readHttpRequest(socket.getInputStream());
+
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "Content-Type: text/plain",
+ "",
+ "content1");
+
+ // Never close the socket to cause SocketTimeoutException during body read on client
+ // side.
+ return null;
+ });
+
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError2 =
+ executor.submit(
+ () -> {
+ while (!executor.isShutdown()) {
+ try (Socket socket = server2.accept()) {
+ readHttpRequest(socket.getInputStream());
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "Content-Type: text/plain",
+ "",
+ "content2");
+ }
+ }
+ return null;
+ });
+
+ final List<URL> urls = new ArrayList<>(2);
+ urls.add(new URL(String.format("http://localhost:%d/foo", server1.getLocalPort())));
+ urls.add(new URL(String.format("http://localhost:%d/foo", server2.getLocalPort())));
+
+ Path resultingFile =
+ httpDownloader.download(
+ urls,
+ Collections.emptyMap(),
+ Optional.absent(),
+ "testCanonicalId",
+ Optional.absent(),
+ fs.getPath(workingDir.newFile().getAbsolutePath()),
+ eventHandler,
+ Collections.emptyMap(),
+ "testRepo");
+
+ assertThat(new String(readFile(resultingFile), UTF_8)).isEqualTo("content2");
+ }
+ }
+
+ @Test
+ public void downloadFrom2UrlsBothSocketTimeoutDuringBodyRead()
+ throws IOException, InterruptedException {
+ try (ServerSocket server1 = new ServerSocket(0, 1, InetAddress.getByName(null));
+ ServerSocket server2 = new ServerSocket(0, 1, InetAddress.getByName(null))) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ executor.submit(
+ () -> {
+ Socket socket = server1.accept();
+ readHttpRequest(socket.getInputStream());
+
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "Content-Type: text/plain",
+ "",
+ "content1");
+
+ // Never close the socket to cause SocketTimeoutException during body read on client
+ // side.
+ return null;
+ });
+
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError2 =
+ executor.submit(
+ () -> {
+ Socket socket = server1.accept();
+ readHttpRequest(socket.getInputStream());
+
+ sendLines(
+ socket,
+ "HTTP/1.1 200 OK",
+ "Date: Fri, 31 Dec 1999 23:59:59 GMT",
+ "Connection: close",
+ "Content-Type: text/plain",
+ "",
+ "content2");
+
+ // Never close the socket to cause SocketTimeoutException during body read on client
+ // side.
+ return null;
+ });
+
+ final List<URL> urls = new ArrayList<>(2);
+ urls.add(new URL(String.format("http://localhost:%d/foo", server1.getLocalPort())));
+ urls.add(new URL(String.format("http://localhost:%d/foo", server2.getLocalPort())));
+
+ Path outputFile = fs.getPath(workingDir.newFile().getAbsolutePath());
+ try {
+ httpDownloader.download(
+ urls,
+ Collections.emptyMap(),
+ Optional.absent(),
+ "testCanonicalId",
+ Optional.absent(),
+ outputFile,
+ eventHandler,
+ Collections.emptyMap(),
+ "testRepo");
+ fail("Should have thrown");
+ } catch (IOException expected) {
+ assertThat(expected.getSuppressed()).hasLength(2);
+
+ for (Throwable suppressed : expected.getSuppressed()) {
+ assertThat(suppressed).isInstanceOf(IOException.class);
+ assertThat(suppressed).hasCauseThat().isInstanceOf(SocketTimeoutException.class);
+ }
+ }
+ }
+ }
+
+ private static byte[] readFile(Path path) throws IOException {
+ final byte[] data = new byte[(int) path.getFileSize()];
+
+ try (DataInputStream stream = new DataInputStream(path.getInputStream())) {
+ stream.readFully(data);
+ }
+
+ return data;
+ }
+}
