fix: rollback transactions that are waiting for tx-id to be returned#4342
fix: rollback transactions that are waiting for tx-id to be returned#4342
Conversation
If a transaction has been started by an async query, and the transaction is closed before the async query has returned the first results and the transaction ID, the transaction would not be rolled back by the client. This would cause locks to be held for longer than they should. This fix adds a check whether the transaction has already sent a request that will start the transaction, and if so, it will add a callback that will rollback the transaction when the transaction ID is returned.
product-auto-label
bot
added
size: m
Summary of ChangesHello @olavloite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where asynchronous transactions might not be properly rolled back if they are closed before the transaction ID is returned by the server. The change introduces a mechanism to defer the rollback operation until the transaction ID is resolved, ensuring that all initiated transactions are eventually cleaned up. This prevents potential resource leaks and improves the robustness of the transaction management system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a fix to ensure that transactions waiting for a transaction ID are properly rolled back when closed. The changes in TransactionRunnerImpl.java correctly add logic to handle this scenario by waiting for the transaction ID future to complete before issuing a rollback. The new test file, OrphanedTransactionTest.java, provides good coverage for both the success and failure paths of this new logic. I've identified one area for improvement in the implementation.
| transactionId -> | ||
| transactionId.isEmpty() | ||
| ? ApiFutures.immediateFuture(Empty.getDefaultInstance()) | ||
| : rpc.rollbackAsync( | ||
| RollbackRequest.newBuilder() | ||
| .setSession(session.getName()) | ||
| .setTransactionId(transactionId) | ||
| .build(), | ||
| getTransactionChannelHint()), |
There was a problem hiding this comment.
The session should be marked as used when a rollback is initiated. This is consistent with other RPC calls in this class and is important for the session pool to correctly manage session liveness, preventing premature eviction of active sessions.
transactionId -> {
if (transactionId.isEmpty()) {
return ApiFutures.immediateFuture(Empty.getDefaultInstance());
}
session.markUsed(clock.instant());
return rpc.rollbackAsync(
RollbackRequest.newBuilder()
.setSession(session.getName())
.setTransactionId(transactionId)
.build(),
getTransactionChannelHint());
},There was a problem hiding this comment.
The session.markUsed call is redundant, now that all operations use multiplexed sessions. I will open a follow-up PR to remove all calls to this, as it does not mean anything in the current setup.
| session.markUsed(clock.instant()); | ||
| return apiFuture; | ||
| } else if (transactionIdFuture != null) { | ||
| ApiFuture<ByteString> transactionIdOrEmptyFuture = |
There was a problem hiding this comment.
1. Do we have to update the session last use time?
2. Can we move this entire into a new function? Previously we used to have 2 condition.
1. transaction id is present
2. transaction id is null
the fix which we are implementing, we are waiting for beginTransaction RPC, once we get it the response back, based on the success / failure, we are trying to do what we were doing before(if transaction id is there then rollback, else do nothing). Moving this a function and reusing the code might be better.
There was a problem hiding this comment.
Do we have to update the session last use time?
No, see also my response to Gemini above :-)
Regarding the second point: I don't think there is much code to be shared, as the implementations are relatively different. There are two possible scenarios:
- There is a transaction ID: In that case
this.transactionIdis not null. How it has been set (e.g. returned by an explicitBeginTransaction, returned by a statement with an inlined-begin, or set in the constructor) is not important. The only important thing is that it is there. - There is no transaction ID, but
this.transactionIdFutureis not null: This means that we are waiting for a transaction ID to be returned, either by an inlined-begin, or by aBeginTransactionRPC. We don't wait for it to be returned, but we add a callback to the future. If the future returns a result, then we use that to execute an async Rollback.
I think that the only way that we could share the logic for the two above scenarios, would be by wrapping this.transactionId in something like an ApiFutures.immediateFuture(..) and then pass that Future in to the method that handles scenario 2. But I'm not sure that simplifies the overall code (and it does add a bit of extra overhead).
There was a problem hiding this comment.
I was thinking of the following approach.
public void waitForTransactionId() {
if (transactionId != null || transactionIdFuture == null) {
return;
}
try {
transactionId = SpannerApiFutures.get(transactionIdFuture);
} catch (SpannerException ignored) {
}
}
and just call waitForTransactionId before close() in rollbackAsync().
I thought of this way. Maybe if you see any issues, we can proceed with the current approach itself. I am approving the PR
There was a problem hiding this comment.
That would make rollbackAsync() potentially blocking, which is not something that we want.
2 participants
If a transaction has been started by an async query, and the transaction is closed before the async query has returned the first results and the transaction ID, the transaction would not be rolled back by the client. This would cause locks to be held for longer than they should.
This fix adds a check whether the transaction has already sent a request that will start the transaction, and if so, it will add a callback that will rollback the transaction when the transaction ID is returned.