Deferred rollback #123
Open
Deferred rollback #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are several ways that a nested rollback can cause un-intended behavior, but this is one I feel is pretty realistic. (In fact, it probably already happens in the wild somewhere.) In this case, if a subroutine wraps some code in a transaction, then catches errors from the code expecting the transaction to be rolled back, then performs some of its own database work without carefully inspecting the exception it caught, the whole thing can end up geting committed if it occurs within an inner transaction.
Currently, there is a messy case when savepoints aren't enabled and a user creates nested transactions and then calls 'rollback'. DBIC throws an exception in hopes that it propogates up the stack to the top-most transaction where it can be rolled back properly, but there is no guarantee that the user doesn't just catch the exception and then accidentally commit unwanted changes by exiting a txn_do block cleanly. This change is a proposed fix that sets a "deferred rollback" condition on the Storage object such that attempting any other transaction-related operation will fail until the top-level transaction has been rolled back. It also makes the BlockRunner aware of deferred rollbacks so that it performs a rollback instead of commit when a block exits cleanly. It also takes the somewhat bold step of stopping BlockRunner from starting new blocks at all until the condition is resolved... I stopped short of reaching into the individual Storage engines to stop them from executing further statements, though that is theoretically what we want to do.
|
Spoke briefly with @nrdvana on IRC about this at time of opening. The issue is legit, but there are likely outstanding issues with the implementation. On top of that CI is not fully functional yet, and the backlog in general doesn't look too friendly. Likely will pick this up end-of-spring-ish. Thanks for the kick @nrdvana ! |
|
@wolfsage your IRC-example of nexting-out-of a txn is a known problem similar but not the same as 2bbdb85a47. The PR on which I am commenting set out to stop the problem from being silent, but more pressing work is in the way of looking at this properly. Just highlighting you as a "Yes, I am aware, work is pending..." |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is my attempt at fixing an edge case where an inner transaction performs a rollback but an outer transaction performs a commit, which can happen in various ways by catching exceptions in txn_do blocks and etc.