centralize hook call logic; clarify the case "abort hook with failing…

… commit"

CHANGES.rst

@@ -21,6 +21,9 @@
 - Support abort hooks (symmetrically to commit hooks)
   (`#77 <https://github.com/zopefoundation/transaction/issues/77>`_).
 
+- Hooks are now cleared after successfull ``commit`` and ``abort`` to avoid
+  potential cyclic references.
+
 
 2.4.0 (2018-10-23)
 ==================

transaction/_transaction.py

@@ -319,9 +319,9 @@ def commit(self):
             finally:
                 del t, v, tb
         else:
-            self._free()
             self._synchronizers.map(lambda s: s.afterCompletion(self))
             self._callAfterCommitHooks(status=True)
+            self._free()
         self.log.debug("commit")
 
     def _saveAndGetCommitishError(self):
@@ -367,12 +367,8 @@ def addBeforeCommitHook(self, hook, args=(), kws=None):
 
     def _callBeforeCommitHooks(self):
         # Call all hooks registered, allowing further registrations
-        # during processing.  Note that calls to addBeforeCommitHook() may
-        # add additional hooks while hooks are running, and iterating over a
-        # growing list is well-defined in Python.
-        for hook, args, kws in self._before_commit:
-            hook(*args, **kws)
-        self._before_commit = []
+        # during processing.
+        self._call_hooks(self._before_commit)
 
     def getAfterCommitHooks(self):
         """ See ITransaction.
@@ -387,34 +383,53 @@ def addAfterCommitHook(self, hook, args=(), kws=None):
         self._after_commit.append((hook, tuple(args), kws))
 
     def _callAfterCommitHooks(self, status=True):
+        self._call_hooks(self._after_commit,
+                         exc=False, clean=True, prefix_args=(status,))
+
+    def _call_hooks(self, hooks, exc=True, clean=False, prefix_args=()):
+        """call *hooks*.
+
+        If *exc* is true, fail on the first exception; otherwise
+        log the exception and continue.
+
+        If *clean* is true, abort all resources. This is to ensure
+        a clean state should a (after) hook has affected one
+        of the resources.
+
+        *prefix_args* defines additioan arguments prefixed
+        to the arguments provided by the hook definition.
+
+        ``_call_hooks`` supports that a hook adds new hooks.
+        """
         # Avoid to abort anything at the end if no hooks are registred.
-        if not self._after_commit:
+        if not hooks:
             return
-        # Call all hooks registered, allowing further registrations
-        # during processing.  Note that calls to addAterCommitHook() may
-        # add additional hooks while hooks are running, and iterating over a
-        # growing list is well-defined in Python.
-        for hook, args, kws in self._after_commit:
-            # The first argument passed to the hook is a Boolean value,
-            # true if the commit succeeded, or false if the commit aborted.
-            try:
-                hook(status, *args, **kws)
-            except:
-                # We need to catch the exceptions if we want all hooks
-                # to be called
-                self.log.error("Error in after commit hook exec in %s ",
-                               hook, exc_info=sys.exc_info())
-        # The transaction is already committed. It must not have
-        # further effects after the commit.
-        for rm in self._resources:
-            try:
-                rm.abort(self)
-            except:
-                # XXX should we take further actions here ?
-                self.log.error("Error in abort() on manager %s",
-                               rm, exc_info=sys.exc_info())
-        self._after_commit = []
-        self._before_commit = []
+        try:
+            # Call all hooks registered, allowing further registrations
+            # during processing
+            for hook, args, kws in hooks:
+                try:
+                    hook(*(prefix_args + args), **kws)
+                except:
+                    if exc:
+                        raise
+                    # We should not fail
+                    self.log.error("Error in hook exec in %s ",
+                                   hook, exc_info=sys.exc_info())
+        finally:
+            del hooks[:]  # clear hooks
+            if not clean:
+                return
+            # The primary operation has already been performed.
+            # But the hooks execution might have left the resources
+            # in an unclean state. Clean up
+            for rm in self._resources:
+                try:
+                    rm.abort(self)
+                except:
+                    # XXX should we take further actions here ?
+                    self.log.error("Error in abort() on manager %s",
+                                   rm, exc_info=sys.exc_info())
 
     def getBeforeAbortHooks(self):
         """ See ITransaction.
@@ -430,12 +445,8 @@ def addBeforeAbortHook(self, hook, args=(), kws=None):
 
     def _callBeforeAbortHooks(self):
         # Call all hooks registered, allowing further registrations
-        # during processing.  Note that calls to addBeforeAbortHook() may
-        # add additional hooks while hooks are running, and iterating over a
-        # growing list is well-defined in Python.
-        for hook, args, kws in self._before_abort:
-            hook(*args, **kws)
-        self._before_abort = []
+        # during processing.
+        self._call_hooks(self._before_abort, exc=False)
 
     def getAfterAbortHooks(self):
         """ See ITransaction.
@@ -450,32 +461,7 @@ def addAfterAbortHook(self, hook, args=(), kws=None):
         self._after_abort.append((hook, tuple(args), kws))
 
     def _callAfterAbortHooks(self):
-        # Avoid to abort anything at the end if no hooks are registred.
-        if not self._after_abort:
-            return
-        # Call all hooks registered, allowing further registrations
-        # during processing.  Note that calls to addAterAbortHook() may
-        # add additional hooks while hooks are running, and iterating over a
-        # growing list is well-defined in Python.
-        for hook, args, kws in self._after_abort:
-            try:
-                hook(*args, **kws)
-            except:
-                # We need to catch the exceptions if we want all hooks
-                # to be called
-                self.log.error("Error in after abort hook exec in %s ",
-                               hook, exc_info=sys.exc_info())
-        # The transaction is already abortted. It must not have
-        # further effects after the abort.
-        for rm in self._resources:
-            try:
-                rm.abort(self)
-            except:
-                # XXX should we take further actions here ?
-                self.log.error("Error in abort() on manager %s",
-                               rm, exc_info=sys.exc_info())
-        self._after_abort = []
-        self._before_abort = []
+        self._call_hooks(self._after_abort, clean=True)
 
     def _commitResources(self):
         # Execute the two-phase commit protocol.
@@ -537,6 +523,7 @@ def _free(self):
         # to break references---this transaction object will not be returned
         # as the current transaction from its manager after this, and all
         # IDatamanager objects joined to it will forgotten
+        # All hooks are forgotten.
         if self._manager:
             self._manager.free(self)
 
@@ -545,6 +532,11 @@ def _free(self):
 
         del self._resources[:]
 
+        del self._before_commit[:]
+        del self._after_commit[:]
+        del self._before_abort[:]
+        del self._after_abort[:]
+
     def data(self, ob):
         try:
             data = self._data

transaction/interfaces.py

@@ -374,13 +374,15 @@ def addBeforeAbortHook(hook, args=(), kws=None):
         hooks.  Applications should take care to avoid creating infinite loops
         by recursively registering hooks.
 
-        Hooks are called only for a top-level abort. If the
+        Abort hooks are called only for a top-level abort. If the
         transaction is committed, abort hooks are not called.
-        This is true even if the commit causes
-        internally an abort; in this case, the after commit hooks
-        are called with first argument `False`.
-        Calling a hook "consumes" its registration too:  hook registrations
-        do not persist across transactions.
+        This is true even if the commit fails. In this case, however,
+        the transaction is in the ``COMMITFAILED`` state and is
+        virtually unusable; therefore, a top-level abort will typically
+        follow.
+
+        Calling a hook "consumes" its registration.
+        Hook registrations do not persist across transactions.
         """
 
     def getBeforeAbortHooks():
@@ -407,13 +409,15 @@ def addAfterAbortHook(hook, args=(), kws=None):
         hooks.  Applications should take care to avoid creating infinite loops
         by recursively registering hooks.
 
-        Hooks are called only for a top-level abort. If the
+        Abort hooks are called only for a top-level abort. If the
         transaction is committed, abort hooks are not called.
-        This is true even if the commit causes
-        internally an abort; in this case, the after commit hooks
-        are called with first argument `False`.
-        Calling a hook "consumes" its registration too:  hook registrations
-        do not persist across transactions.
+        This is true even if the commit fails. In this case, however,
+        the transaction is in the ``COMMITFAILED`` state and is
+        virtually unusable; therefore, a top-level abort will typically
+        follow.
+
+        Calling a hook "consumes" its registration.
+        Hook registrations do not persist across transactions.
         """
 
     def getAfterAbortHooks():

transaction/tests/test__transaction.py

@@ -615,7 +615,7 @@ def _hook2(*args, **kw):
         self.assertEqual(len(logger._log), 1)
         self.assertEqual(logger._log[0][0], 'error')
         self.assertTrue(logger._log[0][1].startswith(
-                            "Error in after commit hook"))
+                            "Error in hook"))
 
     def test_callAfterCommitHook_w_abort(self):
         from transaction.tests.common import DummyLogger
@@ -635,7 +635,7 @@ def _hook2(*args, **kw):
         txn._callAfterCommitHooks()
         self.assertEqual(logger._log[0][0], 'error')
         self.assertTrue(logger._log[0][1].startswith(
-                            "Error in after commit hook"))
+                            "Error in hook"))
 
     def test__commitResources_normal(self):
         from transaction.tests.common import DummyLogger
@@ -855,9 +855,8 @@ def _hook2(*args, **kw):
             txn.abort()
         self.assertEqual(_hooked1, [])
         self.assertEqual(_hooked2, [])
-        # Hooks are neither called nor cleared on abort
-        self.assertEqual(list(txn.getBeforeCommitHooks()),
-                         [(_hook1, ('one',), {'uno': 1}), (_hook2, (), {})])
+        # Hooks are not called but cleared on abort
+        self.assertEqual(list(txn.getBeforeCommitHooks()), [])
 
     def test_abort_w_synchronizers(self):
         from transaction.weakset import WeakSet
@@ -899,14 +898,13 @@ def _hook2(*args, **kw):
             txn._after_commit.append((_hook2, (), {}))
             logger._clear()
             txn.abort()
-        # Hooks are neither called nor cleared on abort
+        # Hooks are not called but cleared on abort
         self.assertEqual(_hooked1, [])
         self.assertEqual(_hooked2, [])
-        self.assertEqual(list(txn.getAfterCommitHooks()),
-                         [(_hook1, ('one',), {'uno': 1}), (_hook2, (), {})])
+        self.assertEqual(list(txn.getAfterCommitHooks()), [])
         self.assertEqual(txn._resources, [])
 
-    def test_abort_error_w_afterCompleteHooks(self):
+    def test_abort_error_w_afterCommitHooks(self):
         from transaction import _transaction
         from transaction.tests.common import DummyLogger
         from transaction.tests.common import Monkey
@@ -933,11 +931,10 @@ def _hook2(*args, **kw):
             txn._resources.append(broken2)
             logger._clear()
             self.assertRaises(ValueError, txn.abort)
-        # Hooks are neither called nor cleared on abort
+        # Hooks are not called but cleared on abort
         self.assertEqual(_hooked1, [])
         self.assertEqual(_hooked2, [])
-        self.assertEqual(list(txn.getAfterCommitHooks()),
-                         [(_hook1, ('one',), {'uno': 1}), (_hook2, (), {})])
+        self.assertEqual(list(txn.getAfterCommitHooks()), [])
         self.assertTrue(aaa._a)
         self.assertFalse(aaa._x)
 
@@ -1023,7 +1020,7 @@ def _hook(*args, **kw):
         self.assertEqual(list(txn.getAfterAbortHooks()),
                          [(_hook, ('one',), {})])
 
-    def test_callAfterAbortHook_w_error(self):
+    def test_callBeforeAbortHook_w_error(self):
         from transaction.tests.common import DummyLogger
         from transaction.tests.common import Monkey
         from transaction import _transaction
@@ -1036,17 +1033,17 @@ def _hook2(*args, **kw):
         with Monkey(_transaction, _LOGGER=logger):
             txn = self._makeOne()
         logger._clear()
-        txn.addAfterAbortHook(_hook1, ('one',))
-        txn.addAfterAbortHook(_hook2, ('two',), dict(dos=2))
-        txn._callAfterAbortHooks()
+        txn.addBeforeAbortHook(_hook1, ('one',))
+        txn.addBeforeAbortHook(_hook2, ('two',), dict(dos=2))
+        txn._callBeforeAbortHooks()
         # second hook gets called even if first raises
         self.assertEqual(_hooked2, [(('two',), {'dos': 2})])
         self.assertEqual(len(logger._log), 1)
         self.assertEqual(logger._log[0][0], 'error')
         self.assertTrue(logger._log[0][1].startswith(
-                            "Error in after abort hook"))
+                            "Error in hook"))
 
-    def test_callAfterAbortHook_w_abort(self):
+    def test_callBeforeAbortHook_w_abort(self):
         from transaction.tests.common import DummyLogger
         from transaction.tests.common import Monkey
         from transaction import _transaction
@@ -1059,12 +1056,12 @@ def _hook2(*args, **kw):
         with Monkey(_transaction, _LOGGER=logger):
             txn = self._makeOne()
         logger._clear()
-        txn.addAfterAbortHook(_hook1, ('one',))
-        txn.addAfterAbortHook(_hook2, ('two',), dict(dos=2))
-        txn._callAfterAbortHooks()
+        txn.addBeforeAbortHook(_hook1, ('one',))
+        txn.addBeforeAbortHook(_hook2, ('two',), dict(dos=2))
+        txn._callBeforeAbortHooks()
         self.assertEqual(logger._log[0][0], 'error')
         self.assertTrue(logger._log[0][1].startswith(
-                            "Error in after abort hook"))
+                            "Error in hook"))
 
     def test_callAfterAbortHook_w_abort_error(self):
         from transaction.tests.common import DummyLogger
@@ -1110,8 +1107,9 @@ def aah():
         txn.addBeforeAbortHook(bah)
         txn.commit()
         self.assertEqual(comm, [])  # not called
-        self.assertEqual(list(txn.getBeforeAbortHooks()), [(bah, (), {})])
-        self.assertEqual(list(txn.getAfterAbortHooks()), [(aah, (), {})])
+        # but cleared
+        self.assertEqual(list(txn.getBeforeAbortHooks()), [])
+        self.assertEqual(list(txn.getAfterAbortHooks()), [])
 
     def test_commit_w_error_w_abortHooks(self):
         comm = []
@@ -1127,6 +1125,7 @@ def aah():
         with self.assertRaises(ValueError):
             txn.commit()
         self.assertEqual(comm, [])  # not called
+        # not cleared
         self.assertEqual(list(txn.getBeforeAbortHooks()), [(bah, (), {})])
         self.assertEqual(list(txn.getAfterAbortHooks()), [(aah, (), {})])