From 85d71b3c46f9405614ebc4dddfa0b33289cd5b39 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 23 Oct 2025 18:51:07 -0500 Subject: [PATCH 1/3] Be mindful of other `SIGHUP` handlers --- synapse/app/_base.py | 100 ++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 40 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 1954dbc1a0f..444114a78d8 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -29,6 +29,7 @@ import warnings from textwrap import indent from threading import Thread +from types import FrameType from typing import ( TYPE_CHECKING, Any, @@ -36,6 +37,7 @@ Callable, NoReturn, Optional, + Union, cast, ) from wsgiref.simple_server import WSGIServer @@ -72,7 +74,6 @@ from synapse.http.site import SynapseSite from synapse.logging.context import LoggingContext, PreserveLoggingContext from synapse.metrics import install_gc_manager, register_threadpool -from synapse.metrics.background_process_metrics import run_as_background_process from synapse.metrics.jemalloc import setup_jemalloc_stats from synapse.module_api.callbacks.spamchecker_callbacks import load_legacy_spam_checkers from synapse.module_api.callbacks.third_party_event_rules_callbacks import ( @@ -540,6 +541,61 @@ def refresh_certificate(hs: "HomeServer") -> None: logger.info("Context factories updated.") +_already_setup_sighup_handling = False +""" +Marks whether we've already successfully ran `setup_sighup_handling()`. +""" + + +def setup_sighup_handling() -> None: + """ + Set up SIGHUP handling to call registered callbacks. + + This can be called multiple times safely. + """ + global _already_setup_sighup_handling + # We only need to set things up once per process. + if _already_setup_sighup_handling: + return + + previous_sighup_handler: Union[ + Callable[[int, Optional[FrameType]], Any], int, None + ] = None + + # Set up the SIGHUP machinery. + if hasattr(signal, "SIGHUP"): + + def handle_sighup(*args: Any, **kwargs: Any) -> None: + # Tell systemd our state, if we're using it. This will silently fail if + # we're not using systemd. + sdnotify(b"RELOADING=1") + + if callable(previous_sighup_handler): + previous_sighup_handler(*args, **kwargs) + + for sighup_callbacks in _instance_id_to_sighup_callbacks_map.values(): + for func, args, kwargs in sighup_callbacks: + func(*args, **kwargs) + + sdnotify(b"READY=1") + + # We defer running the sighup handlers until next reactor tick. This + # is so that we're in a sane state, e.g. flushing the logs may fail + # if the sighup happens in the middle of writing a log entry. + def run_sighup(*args: Any, **kwargs: Any) -> None: + # `callFromThread` should be "signal safe" as well as thread + # safe. + reactor.callFromThread(handle_sighup, *args, **kwargs) + + # Register for the SIGHUP signal, chaining any existing handler as there can + # only be one handler per signal and we don't want to clobber any existing + # handlers (like the `multi_synapse` shard process in the context of Synapse Pro + # for small hosts) + previous_sighup_handler = signal.signal(signal.SIGHUP, run_sighup) + + _already_setup_sighup_handling = True + + async def start(hs: "HomeServer", freeze: bool = True) -> None: """ Start a Synapse server or worker. @@ -579,45 +635,9 @@ async def start(hs: "HomeServer", freeze: bool = True) -> None: name="gai_resolver", server_name=server_name, threadpool=resolver_threadpool ) - # Set up the SIGHUP machinery. - if hasattr(signal, "SIGHUP"): - - def handle_sighup(*args: Any, **kwargs: Any) -> "defer.Deferred[None]": - async def _handle_sighup(*args: Any, **kwargs: Any) -> None: - # Tell systemd our state, if we're using it. This will silently fail if - # we're not using systemd. - sdnotify(b"RELOADING=1") - - for sighup_callbacks in _instance_id_to_sighup_callbacks_map.values(): - for func, args, kwargs in sighup_callbacks: - func(*args, **kwargs) - - sdnotify(b"READY=1") - - # It's okay to ignore the linter error here and call - # `run_as_background_process` directly because `_handle_sighup` operates - # outside of the scope of a specific `HomeServer` instance and holds no - # references to it which would prevent a clean shutdown. - return run_as_background_process( # type: ignore[untracked-background-process] - "sighup", - server_name, - _handle_sighup, - *args, - **kwargs, - ) - - # We defer running the sighup handlers until next reactor tick. This - # is so that we're in a sane state, e.g. flushing the logs may fail - # if the sighup happens in the middle of writing a log entry. - def run_sighup(*args: Any, **kwargs: Any) -> None: - # `callFromThread` should be "signal safe" as well as thread - # safe. - reactor.callFromThread(handle_sighup, *args, **kwargs) - - signal.signal(signal.SIGHUP, run_sighup) - - register_sighup(hs.get_instance_id(), refresh_certificate, hs) - register_sighup(hs.get_instance_id(), reload_cache_config, hs.config) + setup_sighup_handling() + register_sighup(hs.get_instance_id(), refresh_certificate, hs) + register_sighup(hs.get_instance_id(), reload_cache_config, hs.config) # Apply the cache config. hs.config.caches.resize_all_caches() From 63b3d3f8782ddc067310bde675753ec96c1c0810 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 23 Oct 2025 18:55:14 -0500 Subject: [PATCH 2/3] Add changelog --- changelog.d/19095.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19095.misc diff --git a/changelog.d/19095.misc b/changelog.d/19095.misc new file mode 100644 index 00000000000..c9949c9cb5d --- /dev/null +++ b/changelog.d/19095.misc @@ -0,0 +1 @@ +Avoid clobbering other `SIGHUP` handlers in 3rd-party code. From 0d58feefb8d5817662ca1fc3579dd31b7b968296 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 23 Oct 2025 20:37:09 -0500 Subject: [PATCH 3/3] Add logcontext for SIGHUP callbacks to run in See https://github.com/element-hq/synapse/pull/19095#discussion_r2458308702 --- synapse/app/_base.py | 22 ++++++++++++++-------- synapse/config/logger.py | 4 +--- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 444114a78d8..c0fcf8ca299 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -109,7 +109,7 @@ def register_sighup( - homeserver_instance_id: str, + hs: "HomeServer", func: Callable[P, None], *args: P.args, **kwargs: P.kwargs, @@ -124,19 +124,25 @@ def register_sighup( *args, **kwargs: args and kwargs to be passed to the target function. """ - _instance_id_to_sighup_callbacks_map.setdefault(homeserver_instance_id, []).append( - (func, args, kwargs) + # Wrap the function so we can run it within a logcontext + def _callback_wrapper(*args: P.args, **kwargs: P.kwargs) -> None: + with LoggingContext(name="sighup", server_name=hs.hostname): + func(*args, **kwargs) + + _instance_id_to_sighup_callbacks_map.setdefault(hs.get_instance_id(), []).append( + (_callback_wrapper, args, kwargs) ) -def unregister_sighups(instance_id: str) -> None: +def unregister_sighups(homeserver_instance_id: str) -> None: """ Unregister all sighup functions associated with this Synapse instance. Args: - instance_id: Unique ID for this Synapse process instance. + homeserver_instance_id: The unique ID for this Synapse process instance to + unregister hooks for (`hs.get_instance_id()`). """ - _instance_id_to_sighup_callbacks_map.pop(instance_id, []) + _instance_id_to_sighup_callbacks_map.pop(homeserver_instance_id, []) def start_worker_reactor( @@ -636,8 +642,8 @@ async def start(hs: "HomeServer", freeze: bool = True) -> None: ) setup_sighup_handling() - register_sighup(hs.get_instance_id(), refresh_certificate, hs) - register_sighup(hs.get_instance_id(), reload_cache_config, hs.config) + register_sighup(hs, refresh_certificate, hs) + register_sighup(hs, reload_cache_config, hs.config) # Apply the cache config. hs.config.caches.resize_all_caches() diff --git a/synapse/config/logger.py b/synapse/config/logger.py index 8e355035a98..945236ed072 100644 --- a/synapse/config/logger.py +++ b/synapse/config/logger.py @@ -345,9 +345,7 @@ def setup_logging( # Add a SIGHUP handler to reload the logging configuration, if one is available. from synapse.app import _base as appbase - appbase.register_sighup( - hs.get_instance_id(), _reload_logging_config, log_config_path - ) + appbase.register_sighup(hs, _reload_logging_config, log_config_path) # Log immediately so we can grep backwards. logger.warning("***** STARTING SERVER *****")