From e140a7d44c8a129f4bdaf7afc36d0efca91e2f28 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 13 Sep 2023 14:41:07 +0100 Subject: [PATCH 1/3] Remove a reference cycle --- synapse/metrics/background_process_metrics.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/synapse/metrics/background_process_metrics.py b/synapse/metrics/background_process_metrics.py index 9ea4e23b3107..51575ebba6db 100644 --- a/synapse/metrics/background_process_metrics.py +++ b/synapse/metrics/background_process_metrics.py @@ -322,13 +322,15 @@ def __init__(self, name: str, instance_id: Optional[Union[int, str]] = None): if instance_id is None: instance_id = id(self) super().__init__("%s-%s" % (name, instance_id)) - self._proc = _BackgroundProcess(name, self) + self._proc: Optional[_BackgroundProcess] = _BackgroundProcess(name, self) def start(self, rusage: "Optional[resource.struct_rusage]") -> None: """Log context has started running (again).""" super().start(rusage) + assert self._proc is not None + # We've become active again so we make sure we're in the list of active # procs. (Note that "start" here means we've become active, as opposed # to starting for the first time.) @@ -345,6 +347,8 @@ def __exit__( super().__exit__(type, value, traceback) + assert self._proc is not None + # The background process has finished. We explicitly remove and manually # update the metrics here so that if nothing is scraping metrics the set # doesn't infinitely grow. @@ -352,3 +356,6 @@ def __exit__( _background_processes_active_since_last_scrape.discard(self._proc) self._proc.update_metrics() + + # Set proc to None to break the reference cycle. + self._proc = None From 35fc9ee3ba75f791814300a1f5e6fe83495515e4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 13 Sep 2023 14:42:00 +0100 Subject: [PATCH 2/3] Newsfile --- changelog.d/16314.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16314.misc diff --git a/changelog.d/16314.misc b/changelog.d/16314.misc new file mode 100644 index 000000000000..a32b07112a7d --- /dev/null +++ b/changelog.d/16314.misc @@ -0,0 +1 @@ +Remove a reference cycle for in background processes. From c9a216aa62820ed6d1d24a62fa8ab06c5d31cf76 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 13 Sep 2023 15:19:08 +0100 Subject: [PATCH 3/3] Error on bad background process ressurections --- synapse/metrics/background_process_metrics.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/synapse/metrics/background_process_metrics.py b/synapse/metrics/background_process_metrics.py index 51575ebba6db..f1f1f0cdf9a0 100644 --- a/synapse/metrics/background_process_metrics.py +++ b/synapse/metrics/background_process_metrics.py @@ -329,7 +329,13 @@ def start(self, rusage: "Optional[resource.struct_rusage]") -> None: super().start(rusage) - assert self._proc is not None + if self._proc is None: + logger.error( + "Background process re-entered without a proc: %s", + self.name, + stack_info=True, + ) + return # We've become active again so we make sure we're in the list of active # procs. (Note that "start" here means we've become active, as opposed @@ -347,7 +353,13 @@ def __exit__( super().__exit__(type, value, traceback) - assert self._proc is not None + if self._proc is None: + logger.error( + "Background process exited without a proc: %s", + self.name, + stack_info=True, + ) + return # The background process has finished. We explicitly remove and manually # update the metrics here so that if nothing is scraping metrics the set