fix(gax): do not treat a SocketTimeoutException as a thread interrupt - #14578
markaddleman wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates DirectRetryingExecutor to catch SocketTimeoutException separately from other InterruptedIOExceptions, ensuring that socket timeouts are passed to the retry algorithm without interrupting the thread. It also adds unit tests to verify this behavior. The review feedback suggests improving testInterruptedIOExceptionStillFailsAsInterrupted by explicitly asserting that the current thread's interrupted status is correctly preserved, rather than relying on future.get() throwing an ExecutionException wrapping an InterruptedException.
| void testInterruptedIOExceptionStillFailsAsInterrupted() { | ||
| try { | ||
| RetryingFuture<String> future = | ||
| runRetryingTimeouts( | ||
| () -> { | ||
| throw new InterruptedIOException("interrupted"); | ||
| }); | ||
| ExecutionException e = assertThrows(ExecutionException.class, future::get); | ||
| assertInstanceOf(InterruptedException.class, e.getCause()); | ||
| } finally { | ||
| Thread.interrupted(); | ||
| } | ||
| } |
There was a problem hiding this comment.
In testInterruptedIOExceptionStillFailsAsInterrupted, the test asserts that future.get() throws an ExecutionException with an InterruptedException as its cause. However, because DirectRetryingExecutor.submit calls Thread.currentThread().interrupt(), the current thread's interrupt flag is set.
If the interrupt flag is correctly preserved, any subsequent blocking call to future.get() (which delegates to Guava's AbstractFuture.get()) should immediately throw a standalone InterruptedException rather than an ExecutionException wrapping it, unless the interrupt flag was somehow cleared and lost during the future's completion callback execution.
To ensure that the thread's interrupted status is correctly preserved and not lost, we should explicitly assert that the current thread remains interrupted after runRetryingTimeouts completes, and then clear the flag. If the flag is currently being lost, we should investigate why and fix the underlying issue.
References
- In Java, do not swallow InterruptedException. When catching it, restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately.
|
I have signed the Google CLA. Please re-check. |
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. Related to googleapis#12860. Claude-Session: https://claude.ai/code/session_01A4Mm1HgYkqqj25ao2sPSKX
406976f to
02f740e
Compare
This branch has not been deployed
1 participant
Summary
DirectRetryingExecutor.submitcatchesInterruptedIOExceptionand sets the current thread's interrupt flag.SocketTimeoutExceptionextendsInterruptedIOException, so an HTTP connect or read timeout also sets the flag, although no thread was interrupted.setAttemptFuturethen callsFuture.get, which sees the flag and throws a newInterruptedException. TheSocketTimeoutExceptionis lost, and the retry algorithm judges anInterruptedExceptionin its place. A caller sees anInterruptedExceptionfrom an unknown sender.This change catches
SocketTimeoutExceptionfirst and passes it to the retry algorithm unchanged. A real interrupt (InterruptedException, any otherInterruptedIOException,ClosedByInterruptException) keeps the current path.Related to #12860, which reports the interrupt handling in the same catch block. This PR covers the case where no interrupt happened at all.
How we found it
In production,
google-cloud-bigquery2.54.1 with gax 2.84.0. AgetQueryResultspoll did not answer within java-bigquery's default 60 s HTTP read timeout, and the query failed withBigQueryException: java.lang.InterruptedExceptionfromDirectRetryingExecutor.submit. A repro with a 2 s read timeout gives the same stack frame by frame. A Java agent onThread.interruptshowed the read timeout, then a self-interrupt from this catch block, then theInterruptedExceptionin Guava'sAbstractFutureState.blockingGet. Details are on #12860.Tests
DirectRetryingExecutorTesthas two new tests:testSocketTimeoutReachesTheRetryAlgorithm: the retry algorithm sees theSocketTimeoutException, and the call succeeds on the next attempt. On unfixed gax it fails withExecutionException: java.lang.InterruptedException.testInterruptedIOExceptionStillFailsAsInterrupted: a realInterruptedIOExceptionkeeps the current path and still fails as anInterruptedException.All 20 tests in
DirectRetryingExecutorTestpass with the change.