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

CHANGES.rst

@@ -21,9 +21,17 @@
 - 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.
+- Make Transaction drop references to its hooks, manager,
+  synchronizers and data after a successful ``commit()`` and after
+  *any* ``abort()``. This helps avoid potential cyclic references. See
+  `issue 82 <https://github.com/zopefoundation/transaction/issues/82>`_.
 
+- Allow synchronizers to access ``Transaction.data()`` when their
+  ``afterCompletion`` method is called while aborting a transaction.
+
+- Make it safe to call ``Transaction.abort()`` more than once. The
+  second and subsequent calls are no-ops. Previously a
+  ``ValueError(Foreign transaction)`` would be raised.
 
 2.4.0 (2018-10-23)
 ==================

docs/hooks.rst

@@ -377,11 +377,10 @@ after commit hooks are registered
 
 .. doctest::
 
-    >>> mgr = TransactionManager()
-    >>> do = DataObject(mgr)
-
     >>> t = begin()
-    >>> t._manager._txn is not None
+    >>> t._manager is not None
+    True
+    >>> t._manager._txn is t
     True
 
     >>> t.addAfterCommitHook(hook, ('-', 1))
@@ -390,7 +389,9 @@ after commit hooks are registered
     >>> log
     ["True arg '-' kw1 1 kw2 'no_kw2'"]
 
-    >>> t._manager._txn is not None
-    False
+    >>> t._manager is None
+    True
+    >>> mgr._txn is None
+    True
 
     >>> reset_log()

transaction/_transaction.py

@@ -50,7 +50,7 @@ def _makeLogger(): #pragma NO COVER
 def myhasattr(obj, attr):
     return getattr(obj, attr, _marker) is not _marker
 
-class Status:
+class Status(object):
     # ACTIVE is the initial state.
     ACTIVE       = "Active"
 
@@ -63,6 +63,12 @@ class Status:
     # to commit or join this transaction will raise TransactionFailedError.
     COMMITFAILED = "Commit failed"
 
+class _NoSynchronizers(object):
+
+    @staticmethod
+    def map(_f):
+        "Does nothing"
+
 @implementer(interfaces.ITransaction,
              interfaces.ITransactionDeprecated)
 class Transaction(object):
@@ -516,15 +522,25 @@ def _cleanup(self, L):
             except Exception:
                 self.log.error("Error in tpc_abort() on manager %s",
                                rm, exc_info=sys.exc_info())
+    def _free_manager(self):
+        try:
+            if self._manager:
+                self._manager.free(self)
+        finally:
+            # If we try to abort a transaction and fail, the manager
+            # may have begun a new transaction, and will raise a
+            # ValueError from free(); we don't want that to happen
+            # again in _free(), which abort() always calls, so be sure
+            # to clear out the manager.
+            self._manager = None
 
     def _free(self):
         # Called when the transaction has been committed or aborted
         # 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)
+        # All hooks and data are forgotten.
+        self._free_manager()
 
         if hasattr(self, '_data'):
             delattr(self, '_data')
@@ -536,6 +552,12 @@ def _free(self):
         del self._before_abort[:]
         del self._after_abort[:]
 
+        # self._synchronizers might be shared, we can't mutate it
+        self._synchronizers = _NoSynchronizers
+        self._adapters = None
+        self._voted = None
+        self.extension = None
+
     def data(self, ob):
         try:
             data = self._data
@@ -558,18 +580,22 @@ def set_data(self, ob, ob_data):
     def abort(self):
         """ See ITransaction.
         """
-        self._callBeforeAbortHooks()
-        if self._savepoint2index:
-            self._invalidate_all_savepoints()
-
-        self._synchronizers.map(lambda s: s.beforeCompletion(self))
-
         try:
-
             t = None
             v = None
             tb = None
 
+            self._callBeforeAbortHooks()
+            if self._savepoint2index:
+                self._invalidate_all_savepoints()
+
+            try:
+                self._synchronizers.map(lambda s: s.beforeCompletion(self))
+            except:
+                t, v, tb = sys.exc_info()
+                self.log.error("Failed to call synchronizers", exc_info=sys.exc_info())
+
+
             for rm in self._resources:
                 try:
                     rm.abort(self)
@@ -579,8 +605,12 @@ def abort(self):
                     self.log.error("Failed to abort resource manager: %s",
                                    rm, exc_info=sys.exc_info())
 
+
             self._callAfterAbortHooks()
-            self._free()
+            # Unlike in commit(), we are no longer the current transaction
+            # when we call afterCompletion(). But we can't be completely _free():
+            # the synchroninizer might want to access some data it set before.
+            self._free_manager()
 
             self._synchronizers.map(lambda s: s.afterCompletion(self))
 
@@ -589,6 +619,7 @@ def abort(self):
             if tb is not None:
                 reraise(t, v, tb)
         finally:
+            self._free()
             del t, v, tb
 
     def note(self, text):

transaction/tests/test__transaction.py

@@ -857,30 +857,63 @@ def _hook2(*args, **kw):
         self.assertEqual(_hooked2, [])
         # Hooks are not called but cleared on abort
         self.assertEqual(list(txn.getBeforeCommitHooks()), [])
+        self.assertIsNone(txn._manager)
 
     def test_abort_w_synchronizers(self):
-        from transaction.weakset import WeakSet
         from transaction.tests.common import DummyLogger
         from transaction.tests.common import Monkey
         from transaction import _transaction
+        test = self
         class _Synch(object):
-            _before = _after = False
+            _before = _after = None
             def beforeCompletion(self, txn):
                 self._before = txn
+                txn.set_data(self, 42)
+                test.assertIsNotNone(txn._manager)
             def afterCompletion(self, txn):
                 self._after = txn
-        synchs = [_Synch(), _Synch(), _Synch()]
-        ws = WeakSet()
-        for synch in synchs:
-            ws.add(synch)
+                # data is accessible afterCompletion,
+                # but the transaction is not current anymore.
+                test.assertEqual(42, txn.data(self))
+                test.assertIsNone(txn._manager)
+        class _BadSynch(_Synch):
+            def afterCompletion(self, txn):
+                _Synch.afterCompletion(self, txn)
+                raise SystemExit
+
+        # Ensure iteration order
+        class Synchs(object):
+            synchs = [_Synch(), _Synch(), _Synch(), _BadSynch()]
+            def map(self, func):
+                for s in self.synchs:
+                    func(s)
+
         logger = DummyLogger()
+        class Manager(object):
+            txn = None
+            def free(self, txn):
+                test.assertIs(txn, self.txn)
+                self.txn = None
+
+        manager = Manager()
+        synchs = Synchs()
+
         with Monkey(_transaction, _LOGGER=logger):
-            txn = self._makeOne(synchronizers=ws)
+            txn = self._makeOne(synchronizers=synchs, manager=manager)
+            manager.txn = txn
             logger._clear()
-            txn.abort()
-        for synch in synchs:
-            self.assertTrue(synch._before is txn)
-            self.assertTrue(synch._after is txn)
+            with self.assertRaises(SystemExit):
+                txn.abort()
+
+        for synch in synchs.synchs:
+            self.assertIs(synch._before, txn)
+            self.assertIs(synch._after, txn)
+
+        # And everything was cleaned up despite raising the bad
+        # exception
+        self.assertIsNone(txn._manager)
+        self.assertIsNot(txn._synchronizers, synchs)
+        self.assertIsNone(manager.txn)
 
     def test_abort_w_afterCommitHooks(self):
         from transaction.tests.common import DummyLogger
@@ -903,6 +936,7 @@ def _hook2(*args, **kw):
         self.assertEqual(_hooked2, [])
         self.assertEqual(list(txn.getAfterCommitHooks()), [])
         self.assertEqual(txn._resources, [])
+        self.assertIsNone(txn._manager)
 
     def test_abort_error_w_afterCommitHooks(self):
         from transaction import _transaction
@@ -937,6 +971,7 @@ def _hook2(*args, **kw):
         self.assertEqual(list(txn.getAfterCommitHooks()), [])
         self.assertTrue(aaa._a)
         self.assertFalse(aaa._x)
+        self.assertIsNone(txn._manager)
 
     def test_abort_error_w_synchronizers(self):
         from transaction.weakset import WeakSet
@@ -968,6 +1003,47 @@ def abort(self, txn):
         for synch in synchs:
             self.assertTrue(synch._before is t)
             self.assertTrue(synch._after is t) #called in _cleanup
+        self.assertIsNot(t._synchronizers, ws)
+
+    def test_abort_synchronizer_error_w_resources(self):
+        from transaction.weakset import WeakSet
+        from transaction.tests.common import DummyLogger
+        from transaction.tests.common import Monkey
+        from transaction import _transaction
+        class _Synch(object):
+            _before = _after = False
+            def beforeCompletion(self, txn):
+                self._before = txn
+            def afterCompletion(self, txn):
+                self._after = txn
+
+        class _BadSynch(_Synch):
+            def beforeCompletion(self, txn):
+                _Synch.beforeCompletion(self, txn)
+                raise SystemExit
+
+        # Ensure iteration order
+        class Synchs(object):
+            synchs = [_Synch(), _Synch(), _Synch(), _BadSynch()]
+            def map(self, func):
+                for s in self.synchs:
+                    func(s)
+
+        resource = Resource('a')
+        logger = DummyLogger()
+        synchs = Synchs()
+        with Monkey(_transaction, _LOGGER=logger):
+            t = self._makeOne(synchronizers=synchs)
+            logger._clear()
+            t._resources.append(resource)
+            with self.assertRaises(SystemExit):
+                t.abort()
+
+        for synch in synchs.synchs:
+            self.assertTrue(synch._before is t)
+            self.assertTrue(synch._after is t) # called in _cleanup
+        self.assertIsNot(t._synchronizers, synchs)
+        self.assertTrue(resource._a)
 
     def test_abort_clears_resources(self):
         class DM(object):
@@ -1118,7 +1194,7 @@ def aah():
         self.assertEqual(comm, ["before", "after"])
         self.assertEqual(list(txn.getBeforeAbortHooks()), [])
         self.assertEqual(list(txn.getAfterAbortHooks()), [])
-        
+
     def test_commit_w_abortHooks(self):
         comm = []
         txn = self._makeOne()
@@ -1133,7 +1209,7 @@ def aah():
         # but cleared
         self.assertEqual(list(txn.getBeforeAbortHooks()), [])
         self.assertEqual(list(txn.getAfterAbortHooks()), [])
-        
+
     def test_commit_w_error_w_abortHooks(self):
         comm = []
         txn = self._makeOne()
@@ -1151,7 +1227,7 @@ def aah():
         # not cleared
         self.assertEqual(list(txn.getBeforeAbortHooks()), [(bah, (), {})])
         self.assertEqual(list(txn.getAfterAbortHooks()), [(aah, (), {})])
-        
+
     def test_note(self):
         txn = self._makeOne()
         try: