fix: respect register_sys_signals=False (#2956)#3162
Closed
jbbqqf wants to merge 1 commit into
Closed
Conversation
When the caller passes register_sys_signals=False to Sanic.run / Sanic.serve_single, they are taking over signal handling themselves (e.g. an orchestrator that wraps Sanic in a wider process). Since commit 4499d2c ("Cleaner process management", sanic-org#2811) the unconditional calls to signal_func(SIGINT, SIG_IGN) / signal_func(SIGTERM, SIG_IGN) at the top of _setup_system_signals overwrite any handler the caller installed before Sanic started, even on this code path. Move the SIG_IGN reset inside the register_sys_signals branch so it only runs when Sanic is actually about to install its own asyncio signal handlers, restoring the documented behavior of the flag. Adds a unit-level regression test that fails on main (handlers turn into SIG_IGN) and passes on this branch (handlers preserved).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3162 +/- ##
=============================================
- Coverage 87.793% 87.707% -0.086%
=============================================
Files 105 105
Lines 8143 8143
Branches 1290 1290
=============================================
- Hits 7149 7142 -7
- Misses 687 693 +6
- Partials 307 308 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Author
|
Quick note on CI: the red checks here are not caused by this PR.
Happy to rebase / push a baseline refresh as a follow-up commit on this branch if you'd prefer, but figured separating the two changes keeps this PR reviewable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2956.
Sanic.run(..., register_sys_signals=False)is the documented escape hatch for callers that take over SIGINT/SIGTERM themselves (for instance an orchestrator that wraps Sanic inside a larger process and only wants the orchestrator to react to Ctrl-C). Since 4499d2c (#2811, "Cleaner process management"),_setup_system_signalsunconditionally callssignal_func(SIGINT, SIG_IGN)/signal_func(SIGTERM, SIG_IGN)at function entry, before checking theregister_sys_signalsflag. The flag still suppressesloop.add_signal_handler(...), but the user's pre-existing Python-level handlers are already gone, replaced withSIG_IGN. Net effect: opting out of Sanic's signal handling silently disables the caller's signal handling too.The fix moves the
SIG_IGNreset inside theregister_sys_signalsbranch. The reset is still useful there as a safety step before installing the asyncio handlers (and the existing testtest_register_system_signalsstill passes), but it no longer runs on the opt-out path.Reproduce BEFORE/AFTER yourself (copy-paste)
What I ran locally
The new test
test_dont_register_system_signals_preserves_user_handlersis the regression. It calls
_setup_system_signals(..., register_sys_signals=False)directly so the assertion is deterministic and doesn't depend on whether
app.runis spun up. Run it againstorigin/mainto see it fail withSIG_IGN is not <user_handler>; run it on this branch to see it pass.Edge cases
register_sys_signals=True(default), no prior user handlerloop.add_signal_handler; default Python handler reset toSIG_IGNfirstregister_sys_signals=True, user pre-installed a handlerSIG_IGN, then asyncio handler installedregister_sys_signals=False, no prior user handlerSIG_IGN(silent regression vs. pre-#2811)default_int_handlerfor SIGINT, default for SIGTERM)register_sys_signals=False, user pre-installed a handlerOS_IS_WINDOWS) withregister_sys_signals=Truectrlc_workaround_for_windowsrun_multiple=TrueworkersSANIC_WORKER_PROCESS=true; signals reset only when worker registers signalsPR drafted with assistance from Claude Code (Anthropic). The change was reviewed manually against sanic's source (
sanic/server/runners.pyat 785d77f, the v25.12 release commit). The reproducer block above is the one I used during development; reviewers can paste it verbatim.