-
-
Notifications
You must be signed in to change notification settings - Fork 95
Fix rollback being attempted on no transaction because commit already rolled it back #253
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
base: main
Are you sure you want to change the base?
Conversation
src/transaction_manager.rs
Outdated
@@ -358,7 +361,7 @@ where | |||
let is_broken = conn.transaction_state().is_broken.clone(); | |||
|
|||
match Self::critical_transaction_block(&is_broken, conn.batch_execute(&commit_sql)).await { | |||
Ok(()) => { | |||
a @ (Ok(()) | Err(Error::DatabaseError(SerializationFailure, _))) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that logic applies to other backends as well. Given that AnsiTransaction manager is also used for the Mysql backend I would rather prefer a solution that updates this function to only set requires_rollback_maybe_up_to_top_level
for cases where we do not commit? That could be done by having a flag in the transaction manager indicating that we actually execute commit now or something like that. So possibly:
- Add a bool
is_commit
flag toAnsiTransactionManager
- Set that flag to true before the
batch_execute
in the line above, set it to false in the end ofcritical_transaction_block
- Change
update_transaction_manager_status
to check for that flag setrequires_rollback_myabe_up_to_top_level
for it. If that flag is set to false, we won't perform a rollback below.
For reference: The diesel version defers to libpq for that information, but for diesel-async we need to track that on our own.
At some point we should at least try to unify as much of the transaction manager logic as possible, but that's something for future improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense. I think it can be argued that behavior in other backends could be ignored since SerializationError can only be detected and raised on postgres.
That said I'll try this different approach and report back!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiznich Did you have something like this in mind? a175eb2
Having the boolean used to tell we're in a commit and avoiding setting the other boolean flag makes a lot of sense to me, but the fact that we still have to change the commit_transaction
function in order to make sure the transaction state is correct (we need to decrement it without rolling back) felt a bit off to me.
I don't know maybe it's fine, you'll be the better judge.
What do you think?
4c14945
to
a175eb2
Compare
Problem
The issue is better described in the linked issue.
Fixes #252.
Solution
I think the commit succeeding and failing specifically for SerializationError means the same for the transaction manager: that it should adjust it's internal state and nothing else.
How I found out about the issue
I'm running diesel_async on a highly concurrent web-server. On 0.5.x I saw that some transactions were left open on the transaction manager (in memory state) so that the next transaction starting would cause a failure (AlreadyInTransaction).
0.6.x fixed that guaranteeing that serialization failures on commit would generate a rollback. No AlreadyInTransaction was ever seen again, but then we started getting "there is no transaction in progress" from PostgreSQL.
My reasoning is that on 0.5.x, the connection behavior was correct but the in-memory state was not.
Then in 0.6.x, the connection behavior is correct, the in-memory was also correct, but an extra ROLLBACK command was issued, which is incorrect behavior.
Then I think this PR leaves things in a correct state, as the test shows.
Shortcomings
I'm not sure about the effect of this PR on nested transaction, especially since I don't use them and don't know much about them. Maybe someone can shed some light if this change is enough or more work needs to be done to avoid breakage in these scenarios.
cc @lochetti