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

Transaction._free drops more references, and Transaction.abort() is a bit safer #84

Merged
merged 1 commit into from
Dec 9, 2019

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Sep 5, 2019

Specifically, free the manager and synchronizers, plus a few other dictionaries that could be arbitrary size and some of which could contain arbitrary data.

And abort() always invokes _free() even in the case of exceptions. commit() still doesn't so that the synchronizers are available for the expected abort().

But take care about when and how abort frees its manager: not only does this preserve backwards compatibility, but it lets it be safe to abort a transaction object more than once. A happy side-effect of only freeing the manager there is that synchronizers can access
data they set in afterCompletion().

Previously, this code would raise ValueError; it now does nothing on the second call to abort:

tx = transaction.get()
tx.abort()
transaction.get()
tx.abort()

If a synchronizer raised an exception in beforeCompletion, you'd be left with a transaction manager in a bad state and a difficult to clean up situation. Previously, the transaction would never be freed, so this final assertion held and resources joined to the transaction would not actually be aborted:

import transaction

class Sync(object):
    def beforeCompletion(self, txn):
        raise Exception("Bad sync")

sync = Sync()

transaction.manager.registerSynch(sync)
tx = transaction.get()
try:
    transaction.abort()
except Exception:
    pass
assert tx is transaction.get()

Now the resources are properly aborted and the transaction manager can start a new transaction (assuming the bad synchronizer can be fixed, of course).

From discussion in #82

… little safer.

Specifically, free the manager and synchronizers, plus a few other
dictionaries that could be arbitrary size and contain arbitrary data.

And abort() always invokes _free() even in the case of exceptions.
commit() still doesn't so that the synchronizers are available for the
expected abort().

But take care about when and how abort frees its manager: not only
does this preserve backwards compatibility, but it lets it be
safe to abort a transaction object more than once. A happy side-effect
of only freeing the manager there is that synchronizers can access
data they set in afterCompletion().

From discussion in #82
@jamadden jamadden merged commit ec1ceec into master Dec 9, 2019
@jamadden jamadden deleted the issue82 branch December 9, 2019 18:58
@jamadden
Copy link
Member Author

jamadden commented Dec 9, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants