Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: session retry could cause infinite wait #616

Merged
merged 1 commit into from Nov 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -98,6 +98,7 @@ private ApiFuture<TransactionContext> internalBeginAsync(boolean firstAttempt) {
new ApiFutureCallback<Void>() {
@Override
public void onFailure(Throwable t) {
onError(t);
res.setException(SpannerExceptionFactory.newSpannerException(t));
}

Expand Down Expand Up @@ -130,6 +131,7 @@ public ApiFuture<Timestamp> commitAsync() {
}
ApiFuture<Timestamp> res = txn.commitAsync();
txnState = TransactionState.COMMITTED;

ApiFutures.addCallback(
res,
new ApiFutureCallback<Timestamp>() {
Expand Down Expand Up @@ -174,10 +176,6 @@ public ApiFuture<Void> apply(Empty input) throws Exception {

@Override
public TransactionContextFuture resetForRetryAsync() {
if (txn == null || (!txn.isAborted() && txnState != TransactionState.ABORTED)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is no longer needed here as it is checked in the SessionPoolAsyncTransactionManager.

throw new IllegalStateException(
"resetForRetry can only be called if the previous attempt aborted");
}
return new TransactionContextFutureImpl(this, internalBeginAsync(false));
}

Expand Down
Expand Up @@ -38,6 +38,9 @@ class SessionPoolAsyncTransactionManager
@GuardedBy("lock")
private TransactionState txnState;

@GuardedBy("lock")
private AbortedException abortedException;

private final SessionPool pool;
private volatile PooledSessionFuture session;
private volatile SettableApiFuture<AsyncTransactionManagerImpl> delegate;
Expand Down Expand Up @@ -159,6 +162,7 @@ public void onError(Throwable t) {
if (t instanceof AbortedException) {
synchronized (lock) {
txnState = TransactionState.ABORTED;
abortedException = (AbortedException) t;
}
}
}
Expand All @@ -167,9 +171,12 @@ public void onError(Throwable t) {
public ApiFuture<Timestamp> commitAsync() {
synchronized (lock) {
Preconditions.checkState(
txnState == TransactionState.STARTED,
txnState == TransactionState.STARTED || txnState == TransactionState.ABORTED,
"commit can only be invoked if the transaction is in progress. Current state: "
+ txnState);
if (txnState == TransactionState.ABORTED) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure that the AsyncTransactionManager will return an ApiFuture with exception Aborted if the transaction has already been aborted by an asynchronous statement that has not yet been checked by the application, e.g. (simplified):

txn.updateAsync(...); // This aborts the transaction, but the status of the returned ApiFuture is not checked by the application.
...
txn.commitAsync();

return ApiFutures.immediateFailedFuture(abortedException);
}
txnState = TransactionState.COMMITTED;
}
return ApiFutures.transformAsync(
Expand All @@ -186,6 +193,7 @@ public void onFailure(Throwable t) {
synchronized (lock) {
if (t instanceof AbortedException) {
txnState = TransactionState.ABORTED;
abortedException = (AbortedException) t;
} else {
txnState = TransactionState.COMMIT_FAILED;
}
Expand Down
Expand Up @@ -109,6 +109,7 @@ class AsyncTransactionStatementImpl<I, O> extends ForwardingApiFuture<O>
@Override
public void onFailure(Throwable t) {
mgr.onError(t);
statementResult.setException(t);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This missing statement was what could cause the infinite wait. The result of this specific statement would never be done if the previous statement (input) would fail.

txnResult.setException(t);
}

Expand Down