launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:tracer-action-leak into launchpad:master.

Commit message:
Clear connection._lp_statement_action after tracing it

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Since database connections are typically cached, leaving TimedAction references lying around on them makes life more difficult for the cyclic garbage collector and makes it harder to track down memory leaks.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:tracer-action-leak into launchpad:master.

diff --git a/lib/lp/services/webapp/adapter.py b/lib/lp/services/webapp/adapter.py
index 637ad17..a707e11 100644
--- a/lib/lp/services/webapp/adapter.py
+++ b/lib/lp/services/webapp/adapter.py
@@ -637,6 +637,7 @@ class LaunchpadTimeoutTracer(PostgresTimeoutTracer):
                 # action may be None if the tracer was installed after
                 # the statement was submitted.
                 action.finish()
+                connection._lp_statement_action = None
             info = sys.exc_info()
             transaction.doom()
             try:
@@ -732,6 +733,7 @@ class LaunchpadStatementTracer:
             # statement was submitted or if the timeline tracer is not
             # installed.
             action.finish()
+            connection._lp_statement_action = None
         log_sql = getattr(_local, 'sql_logging', None)
         if log_sql is not None or self._debug_sql or self._debug_sql_extra:
             data = None
diff --git a/lib/lp/services/webapp/tests/test_statementtracer.py b/lib/lp/services/webapp/tests/test_statementtracer.py
index bc5ddef..4ba6b53 100644
--- a/lib/lp/services/webapp/tests/test_statementtracer.py
+++ b/lib/lp/services/webapp/tests/test_statementtracer.py
@@ -368,3 +368,23 @@ class TestLoggingWithinRequest(TestCaseWithFactory):
                 tracer.connection_raw_execute_success(
                     self.connection, None, 'SELECT * FROM three', ())
             self.assertEqual(['SELECT * FROM three'], logger.statements)
+
+    def test_clears_timeline_action_reference(self):
+        # The tracer doesn't leave TimedAction references lying around in
+        # the connection.
+        tracer = da.LaunchpadStatementTracer()
+        with person_logged_in(self.person):
+            with StormStatementRecorder():
+                tracer.connection_raw_execute(
+                    self.connection, None, 'SELECT * FROM one', ())
+                self.assertIsNotNone(self.connection._lp_statement_action)
+                tracer.connection_raw_execute_success(
+                    self.connection, None, 'SELECT * FROM one', ())
+                self.assertIsNone(self.connection._lp_statement_action)
+                tracer.connection_raw_execute(
+                    self.connection, None, 'SELECT * FROM one', ())
+                self.assertIsNotNone(self.connection._lp_statement_action)
+                tracer.connection_raw_execute_error(
+                    self.connection, None, 'SELECT * FROM one', (),
+                    Exception())
+                self.assertIsNone(self.connection._lp_statement_action)