From 459bb5736c5af6fc7a70e1cd9dceb40abb0ad71d Mon Sep 17 00:00:00 2001 From: Pareesh Madan Date: Tue, 10 Dec 2024 03:18:25 -0800 Subject: [PATCH] Also apply --experimental_repository_downloader_retries to a SocketException Fix for #24530 --experimental_repository_downloader_retries will now retry on `SocketException` in addition to `ContentLengthMismatchException` Closes #24608. PiperOrigin-RevId: 704633572 Change-Id: Idd1fcbb768c9dabed596fe15d8ae9260ef3e895d --- .../downloader/DownloadManager.java | 9 +- .../downloader/HttpDownloaderTest.java | 94 +++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java index b23a92e4642bc4..c0f42b8735b55c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.io.InterruptedIOException; +import java.net.SocketException; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; @@ -357,12 +358,12 @@ private boolean shouldRetryDownload(IOException e, int attempt) { return false; } - if (e instanceof ContentLengthMismatchException) { + if (isRetryableException(e)) { return true; } for (var suppressed : e.getSuppressed()) { - if (suppressed instanceof ContentLengthMismatchException) { + if (isRetryableException(suppressed)) { return true; } } @@ -370,6 +371,10 @@ private boolean shouldRetryDownload(IOException e, int attempt) { return false; } + private boolean isRetryableException(Throwable e) { + return e instanceof ContentLengthMismatchException || e instanceof SocketException; + } + /** * Downloads the contents of one URL and reads it into a byte array. * 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 index cb50c11e559226..91c7da2c1db9d6 100644 --- 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 @@ -41,6 +41,7 @@ import java.net.InetAddress; import java.net.ServerSocket; import java.net.Socket; +import java.net.SocketException; import java.net.SocketTimeoutException; import java.net.URI; import java.net.URL; @@ -773,6 +774,99 @@ public void download_contentLengthMismatchWithOtherErrors_retries() throws Excep assertThat(content).isEqualTo("content"); } + @Test + public void download_socketException_retries() throws Exception { + Downloader downloader = mock(Downloader.class); + HttpDownloader httpDownloader = mock(HttpDownloader.class); + int retries = 5; + DownloadManager downloadManager = + new DownloadManager(repositoryCache, downloader, httpDownloader); + downloadManager.setRetries(retries); + AtomicInteger times = new AtomicInteger(0); + byte[] data = "content".getBytes(UTF_8); + doAnswer( + (Answer) + invocationOnMock -> { + if (times.getAndIncrement() < 3) { + throw new SocketException("Connection reset"); + } + Path output = invocationOnMock.getArgument(5, Path.class); + try (OutputStream outputStream = output.getOutputStream()) { + ByteStreams.copy(new ByteArrayInputStream(data), outputStream); + } + + return null; + }) + .when(downloader) + .download(any(), any(), any(), any(), any(), any(), any(), any(), any()); + + Path result = + download( + downloadManager, + ImmutableList.of(new URL("http://localhost")), + ImmutableMap.of(), + ImmutableMap.of(), + Optional.empty(), + "testCanonicalId", + Optional.empty(), + fs.getPath(workingDir.newFile().getAbsolutePath()), + eventHandler, + ImmutableMap.of(), + "testRepo"); + + assertThat(times.get()).isEqualTo(4); + String content = new String(ByteStreams.toByteArray(result.getInputStream()), UTF_8); + assertThat(content).isEqualTo("content"); + } + + @Test + public void download_socketExceptionWithOtherErrors_retries() throws Exception { + Downloader downloader = mock(Downloader.class); + HttpDownloader httpDownloader = mock(HttpDownloader.class); + int retries = 5; + DownloadManager downloadManager = + new DownloadManager(repositoryCache, downloader, httpDownloader); + downloadManager.setRetries(retries); + AtomicInteger times = new AtomicInteger(0); + byte[] data = "content".getBytes(UTF_8); + doAnswer( + (Answer) + invocationOnMock -> { + if (times.getAndIncrement() < 3) { + IOException e = new IOException(); + e.addSuppressed(new SocketException("Connection reset")); + e.addSuppressed(new IOException()); + throw e; + } + Path output = invocationOnMock.getArgument(5, Path.class); + try (OutputStream outputStream = output.getOutputStream()) { + ByteStreams.copy(new ByteArrayInputStream(data), outputStream); + } + + return null; + }) + .when(downloader) + .download(any(), any(), any(), any(), any(), any(), any(), any(), any()); + + Path result = + download( + downloadManager, + ImmutableList.of(new URL("http://localhost")), + ImmutableMap.of(), + ImmutableMap.of(), + Optional.empty(), + "testCanonicalId", + Optional.empty(), + fs.getPath(workingDir.newFile().getAbsolutePath()), + eventHandler, + ImmutableMap.of(), + "testRepo"); + + assertThat(times.get()).isEqualTo(4); + String content = new String(ByteStreams.toByteArray(result.getInputStream()), UTF_8); + assertThat(content).isEqualTo("content"); + } + public Path download( DownloadManager downloadManager, List originalUrls,