Merge ~cjwatson/launchpad:py3-traceback-reference-cycles into launchpad:master

[Colin Watson]

Avoid various traceback reference cycles

On Python 3, exceptions have their associated traceback as a `__traceback__` attribute, which means that it's possible to end up with reference cycles by storing exception objects in variables local to frames that are part of their traceback. Delete those local variables before returning to avoid various cases of this.

[Ioana Lasc (ilasc)]

looks good

review: Approve

diff --git a/lib/lp/app/widgets/date.py b/lib/lp/app/widgets/date.py
index dd8ef78..85d0756 100644
--- a/lib/lp/app/widgets/date.py
+++ b/lib/lp/app/widgets/date.py
@@ -348,8 +348,12 @@ class DateTimeWidget(TextWidget):
                     failure = e
             else:
                 return
-        if failure:
-            raise ConversionError('Invalid date value', failure)
+        try:
+            if failure:
+                raise ConversionError('Invalid date value', failure)
+        finally:
+            # Avoid traceback reference cycles.
+            del failure
 
     def _toFieldValue(self, input):
         """Return parsed input (datetime) as a date."""
diff --git a/lib/lp/bugs/mail/handler.py b/lib/lp/bugs/mail/handler.py
index b64b815..3a151e2 100644
--- a/lib/lp/bugs/mail/handler.py
+++ b/lib/lp/bugs/mail/handler.py
@@ -257,6 +257,8 @@ class MaloneHandler:
     def process(self, signed_msg, to_addr, filealias=None, log=None):
         """See IMailHandler."""
 
+        processing_errors = []
+
         try:
             (final_result, add_comment_to_bug,
                 commands, ) = self.extractAndAuthenticateCommands(
@@ -269,7 +271,6 @@ class MaloneHandler:
             bugtask = None
             bugtask_event = None
 
-            processing_errors = []
             while len(commands) > 0:
                 command = commands.pop(0)
                 try:
@@ -332,6 +333,10 @@ class MaloneHandler:
                 'Submit Request Failure',
                 error.message, signed_msg, error.failing_command)
 
+        finally:
+            # Avoid traceback reference cycles.
+            del processing_errors
+
         return True
 
     def sendHelpEmail(self, to_address):
diff --git a/lib/lp/bugs/tests/test_bzremotecomponentfinder.py b/lib/lp/bugs/tests/test_bzremotecomponentfinder.py
index 13faf66..767d84b 100644
--- a/lib/lp/bugs/tests/test_bzremotecomponentfinder.py
+++ b/lib/lp/bugs/tests/test_bzremotecomponentfinder.py
@@ -104,7 +104,11 @@ class TestBugzillaRemoteComponentFinder(TestCaseWithFactory):
             finder.getRemoteProductsAndComponents()
         except Exception as e:
             asserted = e
-        self.assertIs(None, asserted)
+        try:
+            self.assertIs(None, asserted)
+        finally:
+            # Avoid traceback reference cycles.
+            del asserted
 
     @responses.activate
     def test_store(self):
diff --git a/lib/lp/code/mail/codehandler.py b/lib/lp/code/mail/codehandler.py
index 508fb46..fb44573 100644
--- a/lib/lp/code/mail/codehandler.py
+++ b/lib/lp/code/mail/codehandler.py
@@ -262,19 +262,23 @@ class CodeHandler:
     def processCommands(self, context, commands):
         """Process the various merge proposal commands against the context."""
         processing_errors = []
-        with BranchMergeProposalNoPreviewDiffDelta.monitor(
-                context.merge_proposal):
-            for command in commands:
-                try:
-                    command.execute(context)
-                except EmailProcessingError as error:
-                    processing_errors.append((error, command))
-
-        if len(processing_errors) > 0:
-            errors, commands = zip(*processing_errors)
-            raise IncomingEmailError(
-                '\n'.join(str(error) for error in errors),
-                list(commands))
+        try:
+            with BranchMergeProposalNoPreviewDiffDelta.monitor(
+                    context.merge_proposal):
+                for command in commands:
+                    try:
+                        command.execute(context)
+                    except EmailProcessingError as error:
+                        processing_errors.append((error, command))
+
+            if len(processing_errors) > 0:
+                errors, commands = zip(*processing_errors)
+                raise IncomingEmailError(
+                    '\n'.join(str(error) for error in errors),
+                    list(commands))
+        finally:
+            # Avoid traceback reference cycles.
+            del processing_errors
 
         return len(commands)
 
diff --git a/lib/lp/code/model/branchhosting.py b/lib/lp/code/model/branchhosting.py
index a245307..57e373c 100644
--- a/lib/lp/code/model/branchhosting.py
+++ b/lib/lp/code/model/branchhosting.py
@@ -74,9 +74,13 @@ class BranchHostingClient:
             raise
         except Exception:
             _, val, tb = sys.exc_info()
-            reraise(
-                RequestExceptionWrapper, RequestExceptionWrapper(*val.args),
-                tb)
+            try:
+                reraise(
+                    RequestExceptionWrapper,
+                    RequestExceptionWrapper(*val.args), tb)
+            finally:
+                # Avoid traceback reference cycles.
+                del val, tb
         finally:
             action.finish()
         if as_json:
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index 4f07f44..787e63e 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -89,9 +89,13 @@ class GitHostingClient:
             raise
         except Exception:
             _, val, tb = sys.exc_info()
-            reraise(
-                RequestExceptionWrapper, RequestExceptionWrapper(*val.args),
-                tb)
+            try:
+                reraise(
+                    RequestExceptionWrapper,
+                    RequestExceptionWrapper(*val.args), tb)
+            finally:
+                # Avoid traceback reference cycles.
+                del val, tb
         finally:
             action.finish()
         if response.content:
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 21e728d..6e181c3 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -454,14 +454,18 @@ class GitAPI(LaunchpadXMLRPCView):
         result = run_with_login(
             requester_id, self._translatePath,
             six.ensure_text(path).strip("/"), permission, auth_params)
-        if isinstance(result, xmlrpc_client.Fault):
-            logger.error("translatePath failed: %r", result)
-        else:
-            # The results of path translation are not sensitive for logging
-            # purposes (they may refer to private artifacts, but contain no
-            # credentials).
-            logger.info("translatePath succeeded: %s", result)
-        return result
+        try:
+            if isinstance(result, xmlrpc_client.Fault):
+                logger.error("translatePath failed: %r", result)
+            else:
+                # The results of path translation are not sensitive for
+                # logging purposes (they may refer to private artifacts, but
+                # contain no credentials).
+                logger.info("translatePath succeeded: %s", result)
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
 
     @return_fault
     def _notify(self, requester, translated_path, statistics, auth_params):
@@ -498,11 +502,15 @@ class GitAPI(LaunchpadXMLRPCView):
         result = run_with_login(
             requester_id, self._notify,
             translated_path, statistics, auth_params)
-        if isinstance(result, xmlrpc_client.Fault):
-            logger.error("notify failed: %r", result)
-        else:
-            logger.info("notify succeeded: %s" % result)
-        return result
+        try:
+            if isinstance(result, xmlrpc_client.Fault):
+                logger.error("notify failed: %r", result)
+            else:
+                logger.info("notify succeeded: %s" % result)
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
 
     @return_fault
     def _getMergeProposalURL(self, requester, translated_path, branch,
@@ -536,14 +544,18 @@ class GitAPI(LaunchpadXMLRPCView):
         result = run_with_login(
             requester_id, self._getMergeProposalURL,
             translated_path, branch, auth_params)
-        if isinstance(result, xmlrpc_client.Fault):
-            logger.error("getMergeProposalURL failed: %r", result)
-        else:
-            # The result of getMergeProposalURL is not sensitive for logging
-            # purposes (it may refer to private artifacts, but contains no
-            # credentials, only the merge proposal URL).
-            logger.info("getMergeProposalURL succeeded: %s" % result)
-        return result
+        try:
+            if isinstance(result, xmlrpc_client.Fault):
+                logger.error("getMergeProposalURL failed: %r", result)
+            else:
+                # The result of getMergeProposalURL is not sensitive for
+                # logging purposes (it may refer to private artifacts, but
+                # contains no credentials, only the merge proposal URL).
+                logger.info("getMergeProposalURL succeeded: %s" % result)
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
 
     @return_fault
     def _authenticateWithPassword(self, username, password):
@@ -571,15 +583,19 @@ class GitAPI(LaunchpadXMLRPCView):
         logger.info(
             "Request received: authenticateWithPassword('%s')", username)
         result = self._authenticateWithPassword(username, password)
-        if isinstance(result, xmlrpc_client.Fault):
-            logger.error("authenticateWithPassword failed: %r", result)
-        else:
-            # The results of authentication may be sensitive, but we can at
-            # least log the authenticated user.
-            logger.info(
-                "authenticateWithPassword succeeded: %s",
-                result.get("uid", result.get("user")))
-        return result
+        try:
+            if isinstance(result, xmlrpc_client.Fault):
+                logger.error("authenticateWithPassword failed: %r", result)
+            else:
+                # The results of authentication may be sensitive, but we can
+                # at least log the authenticated user.
+                logger.info(
+                    "authenticateWithPassword succeeded: %s",
+                    result.get("uid", result.get("user")))
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
 
     def _renderPermissions(self, set_of_permissions):
         """Render a set of permission strings for XML-RPC output."""
@@ -647,17 +663,21 @@ class GitAPI(LaunchpadXMLRPCView):
         result = run_with_login(
             requester_id, self._checkRefPermissions,
             translated_path, ref_paths, auth_params)
-        if isinstance(result, xmlrpc_client.Fault):
-            logger.error("checkRefPermissions failed: %r", result)
-        else:
-            # The results of ref permission checks are not sensitive for
-            # logging purposes (they may refer to private artifacts, but
-            # contain no credentials).
-            logger.info(
-                "checkRefPermissions succeeded: %s",
-                [(ref_path.data, permissions)
-                 for ref_path, permissions in result])
-        return result
+        try:
+            if isinstance(result, xmlrpc_client.Fault):
+                logger.error("checkRefPermissions failed: %r", result)
+            else:
+                # The results of ref permission checks are not sensitive for
+                # logging purposes (they may refer to private artifacts, but
+                # contain no credentials).
+                logger.info(
+                    "checkRefPermissions succeeded: %s",
+                    [(ref_path.data, permissions)
+                     for ref_path, permissions in result])
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
 
     def _validateRequesterCanManageRepoCreation(
             self, requester, repository, auth_params):
@@ -713,11 +733,15 @@ class GitAPI(LaunchpadXMLRPCView):
                 translated_path, auth_params)
         except Exception as e:
             result = e
-        if isinstance(result, xmlrpc_client.Fault):
-            logger.error("confirmRepoCreation failed: %r", result)
-        else:
-            logger.info("confirmRepoCreation succeeded: %s" % result)
-        return result
+        try:
+            if isinstance(result, xmlrpc_client.Fault):
+                logger.error("confirmRepoCreation failed: %r", result)
+            else:
+                logger.info("confirmRepoCreation succeeded: %s" % result)
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
 
     def _abortRepoCreation(self, requester, translated_path, auth_params):
         naked_repo = removeSecurityProxy(
@@ -740,8 +764,12 @@ class GitAPI(LaunchpadXMLRPCView):
                 translated_path, auth_params)
         except Exception as e:
             result = e
-        if isinstance(result, xmlrpc_client.Fault):
-            logger.error("abortRepoCreation failed: %r", result)
-        else:
-            logger.info("abortRepoCreation succeeded: %s" % result)
-        return result
+        try:
+            if isinstance(result, xmlrpc_client.Fault):
+                logger.error("abortRepoCreation failed: %r", result)
+            else:
+                logger.info("abortRepoCreation succeeded: %s" % result)
+            return result
+        finally:
+            # Avoid traceback reference cycles.
+            del result
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 51b00af..f3e623b 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -387,18 +387,22 @@ class OCIRegistryClient:
         preloaded_data = cls._preloadFiles(build, manifest, digests)
 
         exceptions = []
-        for push_rule in build.recipe.push_rules:
-            for tag in cls._calculateTags(build.recipe):
-                try:
-                    cls._upload_to_push_rule(
-                        push_rule, build, manifest, digests, preloaded_data,
-                        tag)
-                except Exception as e:
-                    exceptions.append(e)
-        if len(exceptions) == 1:
-            raise exceptions[0]
-        elif len(exceptions) > 1:
-            raise MultipleOCIRegistryError(exceptions)
+        try:
+            for push_rule in build.recipe.push_rules:
+                for tag in cls._calculateTags(build.recipe):
+                    try:
+                        cls._upload_to_push_rule(
+                            push_rule, build, manifest, digests,
+                            preloaded_data, tag)
+                    except Exception as e:
+                        exceptions.append(e)
+            if len(exceptions) == 1:
+                raise exceptions[0]
+            elif len(exceptions) > 1:
+                raise MultipleOCIRegistryError(exceptions)
+        finally:
+            # Avoid traceback reference cycles.
+            del exceptions
 
     @classmethod
     def makeMultiArchManifest(cls, http_client, push_rule, build_request,
diff --git a/lib/lp/services/gpg/handler.py b/lib/lp/services/gpg/handler.py
index 643246b..86770c9 100644
--- a/lib/lp/services/gpg/handler.py
+++ b/lib/lp/services/gpg/handler.py
@@ -140,18 +140,16 @@ class GPGHandler:
 
     def getVerifiedSignatureResilient(self, content, signature=None):
         """See IGPGHandler."""
-        errors = []
+        stored_errors = []
 
         for i in range(3):
             try:
                 signature = self.getVerifiedSignature(content, signature)
             except GPGKeyNotFoundError as info:
-                errors.append(info)
+                stored_errors.append(str(info))
             else:
                 return signature
 
-        stored_errors = [str(err) for err in errors]
-
         raise GPGVerificationError(
             "Verification failed 3 times: %s " % stored_errors)
 
diff --git a/lib/lp/services/looptuner.py b/lib/lp/services/looptuner.py
index 6072bf6..32e95ec 100644
--- a/lib/lp/services/looptuner.py
+++ b/lib/lp/services/looptuner.py
@@ -216,7 +216,11 @@ class LoopTuner:
                 # failure, so log it.
                 self.log.exception("Unhandled exception in cleanUp")
             # Reraise the original exception.
-            reraise(exc_info[0], exc_info[1], tb=exc_info[2])
+            try:
+                reraise(exc_info[0], exc_info[1], tb=exc_info[2])
+            finally:
+                # Avoid traceback reference cycles.
+                del exc_info
         else:
             cleanup()
 
diff --git a/lib/lp/services/timeout.py b/lib/lp/services/timeout.py
index 6ba700c..b8b3a7d 100644
--- a/lib/lp/services/timeout.py
+++ b/lib/lp/services/timeout.py
@@ -239,7 +239,11 @@ class with_timeout:
                 exc_info = t.exc_info
                 # Remove the cyclic reference for faster GC.
                 del t.exc_info
-                reraise(exc_info[0], exc_info[1], tb=exc_info[2])
+                try:
+                    reraise(exc_info[0], exc_info[1], tb=exc_info[2])
+                finally:
+                    # Avoid traceback reference cycles.
+                    del exc_info
             return t.result
 
         return call_with_timeout
diff --git a/lib/lp/services/webapp/adapter.py b/lib/lp/services/webapp/adapter.py
index 37c819d..637ad17 100644
--- a/lib/lp/services/webapp/adapter.py
+++ b/lib/lp/services/webapp/adapter.py
@@ -642,7 +642,8 @@ class LaunchpadTimeoutTracer(PostgresTimeoutTracer):
             try:
                 reraise(info[0], info[1], tb=info[2])
             finally:
-                info = None
+                # Avoid traceback reference cycles.
+                del info
 
     def connection_raw_execute_error(self, connection, raw_cursor,
                                      statement, params, error):