Support abort hooks (symmetrically to commit hooks) #77

[d-maurer]

Fixes #77

[jamadden]

Thanks for putting this together!

I left some questions that I'd like to gain some insight on. It might be good to get other's opinions too (/cc @tseaver @jimfulton @mgedmin)

[d-maurer]

Jason Madden wrote at 2019-5-14 06:12 -0700:

jamadden commented on this pull request. I left some questions that I'd like to gain some insight on. It might be good to get other's opinions too (/cc @tseaver @jimfulton @mgedmin) > @@ -120,6 +120,13 @@ def __init__(self, synchronizers=None, manager=None): # List of (hook, args, kws) tuples added by addAfterCommitHook(). self._after_commit = [] + Looks like an extra line of whitespace.

Possible. The repository `zopefoundation/transaction` has not yet a "flake8" test (like other `zopefoundation` repositories).

... + Hooks are called only for a top-level abort. If the I wonder if those are the best semantics. If someone wants to catch all possible aborts, they need to register two hooks: one for top-level aborts, and one for after commit, checking for `False`. It seems like it would be easier to also call the abort hooks from `_cleanup` (error handling during commit) so that all cases where a transaction is aborted can be handled with a single hook. Of course, that might raise the possibility of wanting to know whether the abort was explicit or implicit...

The code in the PR essentially comes from `dm.transaction.aborthook`. There, I was surprised to find out that `transaction` makes the distinction between an implicit abort (called internally during commit) and an explicit (aka "top level") abort (I had not expected such a distinction - and I wrote special documation to make this clear). The implicit abort during commit already had its hook (the commit hook with first parameter `False`) and I did not want to add an additional kind of hooks for this case.

... It's not clear to me why we're calling `rm.abort` again in this loop (note: the loop after the after abort hooks run)?

I have copied the code from the "after commit hook call" handling. The motivation behind seems to be: the hooks might have interacted with the resource managers (sometimes also called data managers) and left them in an unclean state. The "rm.abort()" calls should ensure, that they become clean again.

... I wonder if calling the before abort hooks should be wrapped in `try/except:` handling. ... So it's OK if a before-commit hook raises. But if a before abort hook raises, then we're left with no way to actually abort the transaction.

You are right. I will change this together with potential other changes we decide upon.

... I feel like there's enough duplication across these four different hooks that some meta-programming to reduce duplication might be called for. That can be a separate PR though.

This is a matter of preferences. I dislike duplication - potentially even more than you. However, Jim disliked my implementation of `TransactionManager.run` in terms of `TransactionManager.attempts` (a beautiful very elegant implementation without duplication) in favor of one in elementary terms (with quite some non-trivial logic duplicated from `attempts`). I concluded that the `transaction` "master"s prefer explicity over conciseness and elegance.

[d-maurer]

The discussion in #82 reminded my that all hooks must be cleared after commit/abort (which currently is not yet ensured).

[d-maurer]

commit likely calls _free and _callAfterCommitHooks in the wrong order: the _free informs the transaction manager that the current transaction has terminated; if the manager is in "implicit" mode, then it will (effectively) start a new transaction. The _callAfterCommitHooks has special code to clean up the resource managers in the case that a hook call has changed one of them. However, it looks at its own resource managers (cleared by _free) while the hook has potentially affected those of a new transaction.

[d-maurer]

@jamadden I have followed most of your suggestions. Would you like to take another look?

[jamadden]

LGTM!