From 14651cd86b6fc1d48f56a208a9b5278b3e2dcf75 Mon Sep 17 00:00:00 2001 From: Artem Zinnatullin Date: Mon, 5 Aug 2019 09:32:19 -0700 Subject: [PATCH] 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 --- .../repository/downloader/HttpDownloader.java | 71 +++- .../lib/bazel/repository/downloader/BUILD | 1 + .../downloader/DownloaderTestSuite.java | 1 + ...tpConnectorMultiplexerIntegrationTest.java | 52 ++- .../downloader/HttpDownloaderTest.java | 337 ++++++++++++++++++ 5 files changed, 446 insertions(+), 16 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java 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 5efe7b52d4975b..90a591873bf0af 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 @@ public Path download( 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 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 e5873d4a0dc788..9d2f046d806c3c 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 @@ java_test( "//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 6a479c2c39d134..60b07e56f52d51 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 f5dccf879e797b..3b4c5af98a8d14 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 @@ public class HttpConnectorMultiplexerIntegrationTest { 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 @@ public void normalRequest() throws Exception { 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() { @@ -241,4 +241,52 @@ public Object call() throws Exception { 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 00000000000000..e6ce8fb284dc73 --- /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 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 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 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; + } +}