launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:publication-discard-previous-interaction into launchpad:master.

Commit message:
Discard previous interaction in endRequest

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/403193

Zope's `BrowserPublication.endRequest` leaves a reference to the previous interaction lying around in thread-local storage just in case somebody might want to call `restoreInteraction` later.  However, this significantly complicates cyclic garbage collection and memory leak analysis, because it means that objects referenced by a request can never be garbage-collected immediately after the request finishes, but at best only somewhat later.

There are cases in Launchpad where we do use `restoreInteraction`, but none of them are after the end of publishing a request, so simplify matters by discarding the previous interaction.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:publication-discard-previous-interaction into launchpad:master.

diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
index 8517e7f..b0dabea 100644
--- a/lib/lp/services/webapp/publication.py
+++ b/lib/lp/services/webapp/publication.py
@@ -52,7 +52,10 @@ from zope.publisher.interfaces.browser import (
     IDefaultSkin,
     )
 from zope.publisher.publish import mapply
-from zope.security.management import newInteraction
+from zope.security.management import (
+    endInteraction,
+    newInteraction,
+    )
 from zope.security.proxy import (
     isinstance as zope_isinstance,
     removeSecurityProxy,
@@ -785,6 +788,15 @@ class LaunchpadBrowserPublication(
     def endRequest(self, request, object):
         superclass = zope.app.publication.browser.BrowserPublication
         superclass.endRequest(self, request, object)
+        # BrowserPublication.endRequest calls endInteraction as well, but
+        # calling it once leaves a reference to the previous interaction
+        # around in
+        # zope.security.management.thread_local.previous_interaction just in
+        # case somebody might want to call restoreInteraction later,
+        # significantly complicating memory leak analysis.  We won't need to
+        # restore the previous interaction in this case, so call
+        # endInteraction again to discard it.
+        endInteraction()
 
         da.clear_request_started()
 
diff --git a/lib/lp/services/webapp/tests/test_publication.py b/lib/lp/services/webapp/tests/test_publication.py
index 97d67ad..f248bb1 100644
--- a/lib/lp/services/webapp/tests/test_publication.py
+++ b/lib/lp/services/webapp/tests/test_publication.py
@@ -25,6 +25,8 @@ from zope.publisher.interfaces import (
     NotFound,
     Retry,
     )
+from zope.publisher.publish import publish
+from zope.security.management import thread_local as zope_security_thread_local
 
 from lp.services.database.interfaces import IMasterStore
 from lp.services.identity.model.emailaddress import EmailAddress
@@ -85,6 +87,25 @@ class TestLaunchpadBrowserPublication(TestCase):
         self.assertEqual(request.traversed_objects, [obj1])
 
 
+class TestLaunchpadBrowserPublicationInteractionHandling(TestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_endRequest_removes_previous_interaction(self):
+        # Zope's BrowserPublication.endRequest leaves a reference to the
+        # previous interaction around in
+        # zope.security.management.thread_local.previous_interaction, which
+        # can complicate memory leak analysis.  Since we don't need this
+        # reference, LaunchpadBrowserPublication.endRequest removes it.
+        request = LaunchpadTestRequest(PATH_INFO='/')
+        request.setPublication(LaunchpadBrowserPublication(None))
+        publish(request)
+        self.assertIsNone(
+            getattr(zope_security_thread_local, 'interaction', None))
+        self.assertIsNone(
+            getattr(zope_security_thread_local, 'previous_interaction', None))
+
+
 class TestWebServicePublication(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer