From 650a9bc115591ae601f7b848315a14dac3d8af5a Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Mon, 26 Oct 2026 18:45:46 +0000 Subject: [PATCH] fix(gax): do not treat a SocketTimeoutException as a thread interrupt DirectRetryingExecutor.submit catches InterruptedIOException and sets the current thread's interrupt flag. SocketTimeoutException extends InterruptedIOException, so an HTTP connect or read timeout also sets the flag, although no thread was interrupted. setAttemptFuture then calls Future.get, which sees the flag and throws a new InterruptedException. The SocketTimeoutException is lost, and the retry algorithm judges an InterruptedException in its place. A caller sees an InterruptedException from an unknown sender. Catch SocketTimeoutException first and give it to the retry algorithm unchanged. A real interrupt (InterruptedException, other InterruptedIOException, ClosedByInterruptException) keeps the old path. Supersedes #14578 Related to #12860 Co-authored-by: Mark Addleman --- .../gax/retrying/DirectRetryingExecutor.java | 6 ++ .../retrying/DirectRetryingExecutorTest.java | 68 +++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/DirectRetryingExecutor.java b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/DirectRetryingExecutor.java index aeff549e7542..bdcc4b03a2fd 100644 --- a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/DirectRetryingExecutor.java +++ b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/DirectRetryingExecutor.java @@ -34,6 +34,7 @@ import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; import java.io.InterruptedIOException; +import java.net.SocketTimeoutException; import java.nio.channels.ClosedByInterruptException; import java.util.concurrent.Callable; import org.jspecify.annotations.NullMarked; @@ -103,6 +104,11 @@ public ApiFuture submit(RetryingFuture retryingFuture) { sleep(retryingFuture.getAttemptSettings().getRandomizedRetryDelayDuration()); ResponseT response = retryingFuture.getCallable().call(); retryingFuture.setAttemptFuture(ApiFutures.immediateFuture(response)); + } catch (SocketTimeoutException e) { + // A connect or read timeout is an InterruptedIOException, but no thread was interrupted. + // Setting the interrupt flag here makes setAttemptFuture throw a new InterruptedException, + // which drops this exception and stops the retry. Let the retry algorithm judge it. + retryingFuture.setAttemptFuture(ApiFutures.immediateFailedFuture(e)); } catch (InterruptedException | InterruptedIOException | ClosedByInterruptException e) { Thread.currentThread().interrupt(); retryingFuture.setAttemptFuture(ApiFutures.immediateFailedFuture(e)); diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/DirectRetryingExecutorTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/DirectRetryingExecutorTest.java index f42c3fb20d77..e7eecd2fa7e8 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/DirectRetryingExecutorTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/DirectRetryingExecutorTest.java @@ -29,9 +29,21 @@ */ package com.google.api.gax.retrying; +import static com.google.api.gax.retrying.FailingCallable.FAST_RETRY_SETTINGS; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; + import com.google.api.core.ApiClock; import com.google.api.gax.core.FakeApiClock; +import java.io.InterruptedIOException; +import java.net.SocketTimeoutException; import java.time.Duration; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; class DirectRetryingExecutorTest extends AbstractRetryingExecutorTest { @@ -66,4 +78,60 @@ protected RetryAlgorithm getAlgorithm( new TestResultRetryAlgorithm(apocalypseCountDown, apocalypseException), new ExponentialRetryAlgorithm(retrySettings, clock)); } + + /** + * Runs {@code callable} under an algorithm that retries a {@link SocketTimeoutException} and + * nothing else, so the test sees which exception the algorithm was given. + */ + private RetryingFuture runRetryingTimeouts(Callable callable) { + setUp(false); + RetryAlgorithm algorithm = + new RetryAlgorithm<>( + new BasicResultRetryAlgorithm() { + @Override + public boolean shouldRetry(Throwable prevThrowable, String prevResponse) { + return prevThrowable instanceof SocketTimeoutException; + } + }, + new ExponentialRetryAlgorithm(FAST_RETRY_SETTINGS, clock)); + RetryingExecutorWithContext executor = getExecutor(algorithm); + RetryingFuture future = executor.createFuture(callable, retryingContext); + future.setAttemptFuture(executor.submit(future)); + return future; + } + + @Test + void testSocketTimeoutReachesTheRetryAlgorithm() throws Exception { + AtomicInteger calls = new AtomicInteger(); + try { + RetryingFuture future = + runRetryingTimeouts( + () -> { + if (calls.getAndIncrement() == 0) { + throw new SocketTimeoutException("Read timed out"); + } + return "SUCCESS"; + }); + assertFalse(Thread.currentThread().isInterrupted()); + assertEquals("SUCCESS", future.get()); + assertEquals(2, calls.get()); + } finally { + Thread.interrupted(); + } + } + + @Test + void testInterruptedIOExceptionStillFailsAsInterrupted() { + try { + RetryingFuture future = + runRetryingTimeouts( + () -> { + throw new InterruptedIOException("interrupted"); + }); + ExecutionException e = assertThrows(ExecutionException.class, future::get); + assertInstanceOf(InterruptedException.class, e.getCause()); + } finally { + Thread.interrupted(); + } + } }