googleapis / googleapis/google-cloud-java
[sdk-platform-java] DirectRetryingExecutor re-interrupts the thread before recording interrupt-related attempt failure
- Lingua principale
- Java
- Stelle
- 2.1k
- Fork
- 1.2k
- Merge medio
- 1g 23h
- PR unite (30g)
- 157
Descrizione
### Description
I believe there is an interrupt-ordering issue in `DirectRetryingExecutor` that can cause interrupt behaviour
to surface before the original interrupt-related attempt failure is recorded on the `RetryingFuture`.
In `gax-java/gax/src/main/java/com/google/api/gax/retrying/DirectRetryingExecutor.java`, the catch block for:
- `InterruptedException`
- `InterruptedIOException`
- `ClosedByInterruptException`
currently restores the thread interrupt status before calling `retryingFuture.setAttemptFuture(...)`.
Current code:
```java
} catch (InterruptedException | InterruptedIOException | ClosedByInterruptException e) {
Thread.currentThread().interrupt();
retryingFuture.setAttemptFuture(ApiFutures.immediateFailedFuture(e));
}
```
That ordering appears to allow downstream interrupt handling to observe the thread's interrupt flag too early.
In a local repro against `com.google.api:gax:2.71.0`, the original interrupt-related failure is not preserved
cleanly enough for retry evaluation to proceed as expected.
#### Environment details
- Artifact: `com.google.api:gax`
- Affected version: `2.71.0`
- Consuming library where we observed this in production: `com.google.cloud:google-cloud-bigquery:2.55.3`
- OS: Darwin 25.4.0 arm64
- Java: Temurin / OpenJDK 21.0.4
### Steps to reproduce
1. Configure a `RetryAlgorithm` that retries when `prevThrowable instanceof InterruptedIOException`.
2. Create a `DirectRetryingExecutor`.
3. Use a callable that:
- throws `InterruptedIOException("transient")` on the first call
- returns `"SUCCESS"` on the second call
4. Submit the callable through the `RetryingFuture` and wait for completion.
### Code example
A minimal regression test can be expressed with this shape:
- retry logic is configured to retry ONLY `InterruptedIOException`
- first attempt throws `InterruptedIOException("transient")`
- second attempt succeeds
For example:
```java
AtomicInteger callCount = new AtomicInteger();
AtomicReference result = new AtomicReference<>();
AtomicReference failure = new AtomicReference<>();
RetryAlgorithm retryAlgorithm =
new RetryAlgorithm<>(
new BasicResultRetryAlgorithm() {
@Override
public boolean shouldRetry(Throwable prevThrowable, String prevResponse) {
return prevThrowable instanceof InterruptedIOException;
}
},
new ExponentialRetryAlgorithm(
FailingCallable.FAST_RETRY_SETTINGS,
CurrentMillisClock.getDefaultClock()));
DirectRetryingExecutor executor =
new DirectRetryingExecutor(retryAlgorithm) {
@Override
protected void sleep(java.time.Duration delay) {
// no-op to keep the repro focused on submit() ordering
}
};
Thread worker =
new Thread(
() -> {
RetryingFuture future =
executor.createFuture(
() -> {
if (callCount.getAndIncrement() == 0) {
throw new InterruptedIOException("transient");
}
return "SUCCESS";
});
future.setAttemptFuture(executor.submit(future));
// Clear the caller-thread interrupt flag before reading the final
// future outcome. The fix still restores the interrupt flag, just later.
Thread.interrupted();
try {
result.set(future.get());
} catch (ExecutionException e) {
failure.set(e.getCause());
} catch (Throwable t) {
failure.set(t);
}
});
worker.start();
worker.join();
assertNull(failure.get());
assertEquals("SUCCESS", result.get());
assertEquals(2, callCount.get());
```
### Expected behaviour
- the original interrupt-related failure is recorded first
- retry evaluation sees the original `InterruptedIOException`
- the retry executor performs the second attempt
- the future completes successfully with `"SUCCESS"`
### Actual behaviour
Before reordering the catch block:
- `submit()` catches `InterruptedIOException`
- it restores the interrupt flag before `setAttemptFuture(...)`
- downstream future code can observe the interrupt flag too early
- the future terminates with `InterruptedException` instead of cleanly preserving the original
`InterruptedIOException` for retry evaluation
- the regression test fails with:
```text
expected: but was:
```
### Additional context
In a consuming application using `google-cloud-bigquery:2.55.3`, we observed a production stack trace with:
```text
java.lang.InterruptedException: null
at com.google.common.util.concurrent.AbstractFutureState.blockingGet(AbstractFutureState.java:231)
at com.google.common.util.concurrent.Platform.get(Platform.java:54)
at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:253)
at com.google.common.util.concurrent.ForwardingFuture.get(ForwardingFuture.java:66)
at com.google.api.gax.retrying.BasicRetryingFuture.setAttemptFuture(BasicRetryingFuture.java:98)
at com.google.api.gax.retrying.DirectRetryingExecutor.submit(DirectRetryingExecutor.java:106)
at com.google.cloud.bigquery.BigQueryRetryHelper.run(BigQueryRetryHelper.java:108)
...
```
From the library source, this appears consistent with the following sequence:
1. `DirectRetryingExecutor` catches an interrupt-related exception
2. it calls `Thread.currentThread().interrupt()`
3. it then calls `setAttemptFuture(...)`
4. downstream future code observes the interrupt flag and throws `InterruptedException`
### Proposed fix
Reorder the catch block so the failed attempt is recorded before the interrupt flag is restored:
```java
} catch (InterruptedException | InterruptedIOException | ClosedByInterruptException e) {
retryingFuture.setAttemptFuture(ApiFutures.immediateFailedFuture(e));
Thread.currentThread().interrupt();
}
```
Using the test case above:
- before the reorder: the regression test fails with `InterruptedException`
- after the reorder: the test passes
- a full local `gax` module test run also passes
Guida per i contributori
Apri la guida per i contributori
Direzione di ricerca
Inizia in gax-java/gax/src/main/java/com/google/api/gax/retrying/DirectRetryingExecutor.java e ispeziona il blocco catch che gestisce le eccezioni relative alle interruzioni. Riproduci il problema di ordinamento con un retry configurato per InterruptedIOException, usando la struttura del test di regressione descritta nell’issue. È completato quando l’errore originale raggiunge la valutazione del retry, il secondo tentativo ha successo e i test del modulo gax passano.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- java
- Ambito
- backend
- Tipo di issue
- Bug
- Difficoltà
- 3/5
- Tempo stimato
- 1-2 giorni
- Stato di attività
- Tranquilla
- Chiarezza
- Specificata chiaramente
- Idoneità per principianti
- 76/100