Skip to content

Commit d9a8eef

Browse files
fix(spanner): prevent statement cancellation race in AbstractBaseUnitOfWork (#14283)
In `AbstractBaseUnitOfWork.executeStatementAsync`, statements were submitted asynchronously to `statementExecutor` before storing the resulting future in `currentlyRunningStatementFuture`. Under thread contention, the query RPC can reach the mock server and trigger `cancel()` before the future is assigned, causing `cancel()` to silently drop the cancellation request and resulting in statement timeouts in CI tests such as `StatementTimeoutTest#testCancelReadWriteAutocommitMultipleStatements`. Furthermore, submitting statements while holding the unit-of-work monitor lock causes lock contention when `StatementExecutorType.DIRECT_EXECUTOR` is used (such as in Spanner JDBC or PGAdapter), because direct executors execute synchronously on the calling thread. This change: - Pre-registers a `SettableApiFuture` as `currentlyRunningStatementFuture` under synchronization before submitting to the executor, ensuring any immediate `cancel()` invocation is captured without racing. - Submits the statement task to `statementExecutor` outside the synchronized block to avoid holding the unit-of-work lock during direct synchronous execution. - Attaches callbacks to forward results/exceptions to the `SettableApiFuture`, and registers a listener on the `SettableApiFuture` to propagate cancellation to the underlying future and clear `currentlyRunningStatementFuture`.
1 parent d086ba7 commit d9a8eef

1 file changed

Lines changed: 46 additions & 15 deletions

File tree

‎java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractBaseUnitOfWork.java‎

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@
1717
package com.google.cloud.spanner.connection;
1818

1919
import com.google.api.core.ApiFuture;
20+
import com.google.api.core.ApiFutureCallback;
2021
import com.google.api.core.ApiFutures;
22+
import com.google.api.core.SettableApiFuture;
2123
import com.google.api.gax.grpc.GrpcCallContext;
2224
import com.google.api.gax.longrunning.OperationFuture;
2325
import com.google.api.gax.rpc.ApiCallContext;
@@ -401,11 +403,29 @@ public <ReqT, RespT> ApiCallContext configure(
401403
io.opentelemetry.context.Context.current()
402404
.with(OpenTelemetryContextKeys.THREAD_NAME_KEY, Thread.currentThread().getName())
403405
.makeCurrent()) {
404-
ApiFuture<T> f = statementExecutor.submit(context.wrap(callable));
405406
final SpannerAsyncExecutionException caller =
406407
callType == CallType.ASYNC
407408
? new SpannerAsyncExecutionException(statement.getStatement())
408409
: null;
410+
final SettableApiFuture<T> statementFuture = SettableApiFuture.create();
411+
synchronized (this) {
412+
this.currentlyRunningStatementFuture = statementFuture;
413+
}
414+
final ApiFuture<T> f;
415+
try {
416+
f = statementExecutor.submit(context.wrap(callable));
417+
} catch (Throwable t) {
418+
synchronized (this) {
419+
if (this.currentlyRunningStatementFuture == statementFuture) {
420+
this.currentlyRunningStatementFuture = null;
421+
}
422+
}
423+
if (isSingleUse()) {
424+
endUnitOfWorkSpan();
425+
}
426+
statementFuture.setException(t);
427+
throw t;
428+
}
409429
final ApiFuture<T> future =
410430
ApiFutures.catching(
411431
f,
@@ -417,25 +437,36 @@ public <ReqT, RespT> ApiCallContext configure(
417437
throw SpannerExceptionFactory.asSpannerException(input);
418438
},
419439
MoreExecutors.directExecutor());
420-
synchronized (this) {
421-
this.currentlyRunningStatementFuture = future;
422-
}
423-
future.addListener(
424-
new Runnable() {
440+
ApiFutures.addCallback(
441+
future,
442+
new ApiFutureCallback<T>() {
425443
@Override
426-
public void run() {
427-
synchronized (this) {
428-
if (currentlyRunningStatementFuture == future) {
429-
currentlyRunningStatementFuture = null;
430-
}
431-
}
432-
if (isSingleUse()) {
433-
endUnitOfWorkSpan();
444+
public void onFailure(Throwable t) {
445+
statementFuture.setException(t);
446+
}
447+
448+
@Override
449+
public void onSuccess(T result) {
450+
statementFuture.set(result);
451+
}
452+
},
453+
MoreExecutors.directExecutor());
454+
statementFuture.addListener(
455+
() -> {
456+
if (statementFuture.isCancelled()) {
457+
future.cancel(true);
458+
}
459+
synchronized (AbstractBaseUnitOfWork.this) {
460+
if (currentlyRunningStatementFuture == statementFuture) {
461+
currentlyRunningStatementFuture = null;
434462
}
435463
}
464+
if (isSingleUse()) {
465+
endUnitOfWorkSpan();
466+
}
436467
},
437468
MoreExecutors.directExecutor());
438-
return future;
469+
return statementFuture;
439470
}
440471
}
441472

0 commit comments

Comments
 (0)