[CalendarServer-changes] [11734] CalendarServer/trunk/twext/enterprise

[source_changes at macosforge.org]

Revision: 11734
          http://trac.calendarserver.org//changeset/11734
Author:   glyph at apple.com
Date:     2013-09-17 16:41:33 -0700 (Tue, 17 Sep 2013)
Log Message:
-----------
Address a memory leak with circular references in commit hooks.

Modified Paths:
--------------
    CalendarServer/trunk/twext/enterprise/adbapi2.py
    CalendarServer/trunk/twext/enterprise/test/test_adbapi2.py

Modified: CalendarServer/trunk/twext/enterprise/adbapi2.py
===================================================================
--- CalendarServer/trunk/twext/enterprise/adbapi2.py	2013-09-17 23:41:32 UTC (rev 11733)
+++ CalendarServer/trunk/twext/enterprise/adbapi2.py	2013-09-17 23:41:33 UTC (rev 11734)
@@ -84,6 +84,15 @@
 
 
 
+def _destructively(aList):
+    """
+    Destructively iterate a list, popping elements from the beginning.
+    """
+    while aList:
+        yield aList.pop(0)
+
+
+
 def _deriveParameters(cursor, args):
     """
     Some DB-API extensions need to call special extension methods on
@@ -439,8 +448,7 @@
         a Deferred to not interfere with the originally submitted order of
         commands.
         """
-        while self._spool:
-            yield self._spool.pop(0)
+        return _destructively(self._spool)
 
 
     def _unspool(self, other):
@@ -489,7 +497,7 @@
         """
         Callback for C{commit} and C{abort} Deferreds.
         """
-        for operation in self._hooks:
+        for operation in _destructively(self._hooks):
             yield operation()
         returnValue(ignored)
 
@@ -501,7 +509,14 @@
         self._hooks.append(operation)
 
 
+    def clear(self):
+        """
+        Remove all hooks from this operation.
+        """
+        del self._hooks[:]
 
+
+
 class _CommitAndAbortHooks(object):
     """
     Shared implementation of post-commit and post-abort hooks.
@@ -668,6 +683,8 @@
 
     def abort(self):
         self._markComplete()
+        self._commit.clear()
+        self._preCommit.clear()
         result = super(_SingleTxn, self).abort()
         if self in self._pool._waiting:
             self._stopWaiting()
@@ -1595,6 +1612,8 @@
 
 
     def abort(self):
+        self._commit.clear()
+        self._preCommit.clear()
         return self._complete(Abort).addCallback(self._abort.runHooks)
 
 

Modified: CalendarServer/trunk/twext/enterprise/test/test_adbapi2.py
===================================================================
--- CalendarServer/trunk/twext/enterprise/test/test_adbapi2.py	2013-09-17 23:41:32 UTC (rev 11733)
+++ CalendarServer/trunk/twext/enterprise/test/test_adbapi2.py	2013-09-17 23:41:33 UTC (rev 11734)
@@ -18,6 +18,8 @@
 Tests for L{twext.enterprise.adbapi2}.
 """
 
+import gc
+
 from zope.interface.verify import verifyObject
 
 from twisted.python.failure import Failure
@@ -44,6 +46,35 @@
 from twext.enterprise.fixtures import CommitFail
 from twext.enterprise.adbapi2 import Commit
 
+
+class TrashCollector(object):
+    """
+    Test helper for monitoring gc.garbage.
+    """
+    def __init__(self, testCase):
+        self.testCase = testCase
+        testCase.addCleanup(self.checkTrash)
+        self.start()
+
+
+    def start(self):
+        gc.collect()
+        self.garbageStart = len(gc.garbage)
+
+
+    def checkTrash(self):
+        """
+        Ensure that the test has added no additional garbage.
+        """
+        gc.collect()
+        newGarbage = gc.garbage[self.garbageStart:]
+        if newGarbage:
+            # Don't clean up twice.
+            self.start()
+            self.testCase.fail("New garbage: " + repr(newGarbage))
+
+
+
 class AssertResultHelper(object):
     """
     Mixin for asserting about synchronous Deferred results.
@@ -466,7 +497,6 @@
         """
         t = self.createTransaction()
         self.resultOf(t.execSQL("echo", []))
-        import gc
         conns = self.factory.connections
         self.assertEquals(len(conns), 1)
         self.assertEquals(conns[0]._rollbackCount, 0)
@@ -478,6 +508,42 @@
         self.assertEquals(conns[0]._commitCount, 0)
 
 
+    def circularReferenceTest(self, finish):
+        """
+        Collecting a completed (committed or aborted) L{IAsyncTransaction}
+        should not leak any circular references.
+        """
+        tc = TrashCollector(self)
+        commitExecuted = []
+        def carefullyManagedScope():
+            t = self.createTransaction()
+            def holdAReference():
+                """
+                This is a hook that holds a reference to 't'.
+                """
+                commitExecuted.append(True)
+                return t.execSQL("teardown", [])
+            t.preCommit(holdAReference)
+            finish(t)
+        self.failIf(commitExecuted, "Commit hook executed.")
+        carefullyManagedScope()
+        tc.checkTrash()
+
+
+    def test_noGarbageOnCommit(self):
+        """
+        Committing a transaction does not cause gc garbage.
+        """
+        self.circularReferenceTest(lambda txn: txn.commit())
+
+
+    def test_noGarbageOnAbort(self):
+        """
+        Aborting a transaction does not cause gc garbage.
+        """
+        self.circularReferenceTest(lambda txn: txn.abort())
+
+
     def test_tooManyConnectionsWhileOthersFinish(self):
         """
         L{ConnectionPool.connection} will not spawn more than the maximum
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.macosforge.org/pipermail/calendarserver-changes/attachments/20130917/823a7e05/attachment-0001.html>