Skip to content

correctly handle signals in nested transactions#646

Closed
michamos wants to merge 1 commit intopallets-eco:mainfrom
michamos:fix-nested-transaction-signals
Closed

correctly handle signals in nested transactions#646
michamos wants to merge 1 commit intopallets-eco:mainfrom
michamos:fix-nested-transaction-signals

Conversation

@michamos
Copy link

@michamos michamos commented Oct 15, 2018

This PR fixes the issue reported in #645 by checking whether the current transaction is nested in the before_commit and after_commit handlers, and not doing anything if that's the case. Handling rollbacks of nested transaction is a bit more tricky: it requires maintaining a stack of the changes in every subtransaction, and only discarding the changes in the subtransactions that are rolled back.

I have added a test (and a fixture to work around issues in the sqlite driver) to check that the behavior (actual changes sent at the moment the full transaction is committed) is correct in two cases: when a subtransaction rollback preserves an outer transaction, and when it causes the whole transaction to be rolled back.

@michamos michamos force-pushed the fix-nested-transaction-signals branch from 61f6c09 to 57233f4 Compare October 16, 2018 07:21
@pallets-eco pallets-eco deleted a comment from michamos Oct 16, 2018
@michamos
Copy link
Author

michamos commented Nov 9, 2018

@davidism sorry to bother you again about this, but this is really an important issue for us, and we currently need to use ugly workarounds because of it (retrying tasks periodically until the real commit has happened and they get the new version when they query the DB). I am willing to work further on it and do whatever is needed to get it fixed. Could you maybe provide some insight into whether something like this (with potential modifications) could eventually be merged?

@rsyring rsyring added this to the Unknown milestone Mar 9, 2019
@michamos
Copy link
Author

michamos commented Jun 6, 2019

What's the plan for this after #727 ? as there is no plan to deprecate the modification tracking any more but just disable it by default, I think this fix (or something similar) should be added, as it's currently broken when using begin_nested. Note we have been using this in production for a few months now with now adverse effects.

@davidism
Copy link
Member

Duplicate of #358, an alternate implementation. Closing for the same reasons:

#1087 changes modification tracking, so this will no longer be mergeable. I also have the impression that how SQLAlchemy tracks nested transactions has changed, but I'm not sure. That said, after considering it more, I don't want to add more complexity to the track modifications feature. If you need other behavior, you're better off using SQLAlchemy's events directly.

@davidism davidism closed this Sep 18, 2022
@davidism davidism removed this from the Unsure milestone Sep 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments