Be mindful of other SIGHUP handlers in 3rd-party code
#19095
Conversation
MadLittleMods
changed the title
SIGHUP handlersSIGHUP handlers in 3rd-party code
| return run_as_background_process( # type: ignore[untracked-background-process] | ||
| "sighup", | ||
| server_name, | ||
| _handle_sighup, | ||
| *args, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
As far as I can tell, we don't need this run_as_background_process(...) layer of indirection.
It was originally introduced as @wrap_as_background_process("sighup") in matrix-org/synapse#8817. The purpose of that PR was to handle SIGHUP on the next reactor tick but we already accomplish that with reactor.callFromThread(...) which was also introduced in that PR.
For reference, @wrap_as_background_process was recently refactored to run_as_background_process in #18670 but it's functionally equivalent to other PR it was introduced in.
There was a problem hiding this comment.
I guess this added a logcontext for the sighup callbacks to run in.
I've updated to add a logcontext ✅
| # 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) |
There was a problem hiding this comment.
This is part of the main fix of this PR. Chain any existing SIGHUP handlers like the one we'll see from running in the context of the multi_synapse shard with Synapse Pro for small hosts.
| global _already_setup_sighup_handling | ||
| # We only need to set things up once per process. | ||
| if _already_setup_sighup_handling: | ||
| return |
There was a problem hiding this comment.
Other part of the main fix is only setting up the SIGHUP handling once
MadLittleMods
force-pushed
the
madlittlemods/no-clobber-sighup-handler
branch
from
680594e to
63b3d3f
Compare
Be mindful that Synapse can be run alongside other code in the same Python process. We shouldn't clobber other
SIGHUPhandlers as only one can set at time.(no clobber)
Background
As part of Element's plan to support a light form of vhosting (virtual host) (multiple instances of Synapse in the same Python process), we're currently diving into the details and implications of running multiple instances of Synapse in the same Python process.
"Per-tenant logging" tracked internally by https://github.com/element-hq/synapse-small-hosts/issues/48
Relevant to logging as we use a
SIGHUPto reload log config in Synapse.Dev notes
@wrap_as_background_process("sighup")was introduced in Defer SIGHUP handlers to reactor. matrix-org/synapse#8817@wrap_as_background_processrefactored torun_as_background_processin Refactor background process metrics to be homeserver-scoped #18670Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.