googleapis / googleapis/google-cloud-java

[sdk-platform-java] DirectRetryingExecutor re-interrupts the thread before recording interrupt-related attempt failure

Đang mở
#12,860 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
priority: p2 type: bug
Ngôn ngữ chính
Java
Star
2.1k
Fork
1.2k
Merge trung bình
1 ngày 23 giờ
Pull request đã merge (30 ngày)
154

Mô tả

### 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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.