From e4891ebbd1cd66696b4e08bcfe2bc1b7d07037d9 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Thu, 5 Sep 2019 07:03:08 -0500 Subject: [PATCH] Transaction._free drops more references, and Transaction.abort() is a 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 --- CHANGES.rst | 12 ++- docs/hooks.rst | 13 ++-- transaction/_transaction.py | 55 ++++++++++--- transaction/tests/test__transaction.py | 104 +++++++++++++++++++++---- 4 files changed, 150 insertions(+), 34 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index e9d4cf3..8bebba7 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -21,9 +21,17 @@ - Support abort hooks (symmetrically to commit hooks) (`#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 `_. +- 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) ================== diff --git a/docs/hooks.rst b/docs/hooks.rst index e3b2331..2a2e989 100644 --- a/docs/hooks.rst +++ b/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() diff --git a/transaction/_transaction.py b/transaction/_transaction.py index 9f1cca2..c2f9cb2 100644 --- a/transaction/_transaction.py +++ b/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): diff --git a/transaction/tests/test__transaction.py b/transaction/tests/test__transaction.py index fb8e5d4..bab6d26 100644 --- a/transaction/tests/test__transaction.py +++ b/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: