From 2eb00f903002cd170ea66c89856c95b6df4e8066 Mon Sep 17 00:00:00 2001
From: Jason Madden <jamadden@gmail.com>
Date: Fri, 20 May 2016 12:33:32 -0500
Subject: [PATCH] Free DataManagers when a transaction becomes non-current.

---
 CHANGES.rst                            |  6 ++++++
 docs/hooks.rst                         |  2 +-
 transaction/_transaction.py            | 21 ++++++++++++++------
 transaction/tests/test__transaction.py | 27 ++++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/CHANGES.rst b/CHANGES.rst
index 3d5d5cb..9eaa5e3 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -1,6 +1,12 @@
 Changes
 =======
 
+1.6.0 (TBD)
+-----------
+
+- Drop references to data managers joined to a transaction when it is
+  committed or aborted.
+
 1.5.0 (2016-05-05)
 ------------------
 
diff --git a/docs/hooks.rst b/docs/hooks.rst
index 5b03fac..b7297af 100644
--- a/docs/hooks.rst
+++ b/docs/hooks.rst
@@ -360,7 +360,7 @@ can be executed. We don't support execution dependencies at this level.
 
     >>> reset_log()
 
-Test that the associated transaction manager has been cleanup when
+Test that the associated transaction manager has been cleaned up when
 after commit hooks are registered
 
 .. doctest::
diff --git a/transaction/_transaction.py b/transaction/_transaction.py
index 8aab03f..6f31a2d 100644
--- a/transaction/_transaction.py
+++ b/transaction/_transaction.py
@@ -41,7 +41,7 @@ def _makeLogger(): #pragma NO COVER
     if _LOGGER is not None:
         return _LOGGER
     return logging.getLogger("txn.%d" % get_thread_ident())
-    
+
 
 # The point of this is to avoid hiding exceptions (which the builtin
 # hasattr() does).
@@ -281,8 +281,7 @@ def commit(self):
             finally:
                 del t, v, tb
         else:
-            if self._manager:
-                self._manager.free(self)
+            self._free()
             self._synchronizers.map(lambda s: s.afterCompletion(self))
             self._callAfterCommitHooks(status=True)
         self.log.debug("commit")
@@ -434,6 +433,17 @@ def _cleanup(self, L):
                 self.log.error("Error in tpc_abort() on manager %s",
                                rm, exc_info=sys.exc_info())
 
+    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
+        if self._manager:
+            self._manager.free(self)
+
+        del self._resources[:]
+
+
     def abort(self):
         """ See ITransaction.
         """
@@ -447,7 +457,7 @@ def abort(self):
             t = None
             v = None
             tb = None
-            
+
             for rm in self._resources:
                 try:
                     rm.abort(self)
@@ -457,8 +467,7 @@ def abort(self):
                     self.log.error("Failed to abort resource manager: %s",
                                    rm, exc_info=sys.exc_info())
 
-            if self._manager:
-                self._manager.free(self)
+            self._free()
 
             self._synchronizers.map(lambda s: s.afterCompletion(self))
 
diff --git a/transaction/tests/test__transaction.py b/transaction/tests/test__transaction.py
index 49e747f..fbc8952 100644
--- a/transaction/tests/test__transaction.py
+++ b/transaction/tests/test__transaction.py
@@ -461,6 +461,7 @@ def _hook2(*args, **kw):
         self.assertEqual(_hooked1, [((True, 'one',), {'uno': 1})])
         self.assertEqual(_hooked2, [((True,), {})])
         self.assertEqual(txn._after_commit, [])
+        self.assertEqual(txn._resources, [])
 
     def test_commit_error_w_afterCompleteHooks(self):
         from transaction import _transaction
@@ -528,6 +529,17 @@ def tpc_begin(self, txn):
             self.assertTrue(synch._before is txn)
             self.assertTrue(synch._after is txn) #called in _cleanup
 
+    def test_commit_clears_resources(self):
+        class DM(object):
+            tpc_begin = commit = tpc_finish = tpc_vote = lambda s, txn: True
+
+        dm = DM()
+        txn = self._makeOne()
+        txn.join(dm)
+        self.assertEqual(txn._resources, [dm])
+        txn.commit()
+        self.assertEqual(txn._resources, [])
+
     def test_getBeforeCommitHooks_empty(self):
         txn = self._makeOne()
         self.assertEqual(list(txn.getBeforeCommitHooks()), [])
@@ -878,6 +890,7 @@ def _hook2(*args, **kw):
         self.assertEqual(_hooked2, [])
         self.assertEqual(list(txn.getAfterCommitHooks()),
                          [(_hook1, ('one',), {'uno': 1}), (_hook2, (), {})])
+        self.assertEqual(txn._resources, [])
 
     def test_abort_error_w_afterCompleteHooks(self):
         from transaction import _transaction
@@ -945,6 +958,17 @@ def abort(self, txn):
             self.assertTrue(synch._before is t)
             self.assertTrue(synch._after is t) #called in _cleanup
 
+    def test_abort_clears_resources(self):
+        class DM(object):
+            abort = lambda s, txn: True
+
+        dm = DM()
+        txn = self._makeOne()
+        txn.join(dm)
+        self.assertEqual(txn._resources, [dm])
+        txn.abort()
+        self.assertEqual(txn._resources, [])
+
     def test_note(self):
         txn = self._makeOne()
         try:
@@ -1486,3 +1510,6 @@ def test_suite():
         unittest.makeSuite(NoRollbackSavepointTests),
         unittest.makeSuite(MiscellaneousTests),
         ))
+
+if __name__ == '__main__':
+    unittest.main()