Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip installation of signal handlers when not in main thread #871

Merged
merged 2 commits into from Dec 7, 2020

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Nov 30, 2020

This one has been bogging me for a while, so here's a tentative pull request for it:

Since signals can only be listened to from the main thread, this PR modifies .install_signal_handlers() to skip installation of signal handlers when not in the main thread:

https://docs.python.org/3/library/signal.html#signals-and-threads

[…] Only the main thread of the main interpreter is allowed to set a new signal handler.

This way, we can also get rid of a bunch of boilerplate overrides of .install_signal_handlers() in tests — and many other people will be able to as well (eg we do it in HTTPX, HTTPCore, many of my personal packages, and essentially anywhere one needs to run Uvicorn in a Python thread right now).

Surprisingly enough I haven't found an issue open for this yet. :-) But this is somewhat related to #526 (although I don't think it's exactly fixing it).

@euri10
Copy link
Member

euri10 commented Dec 1, 2020

weird this doesnt pass tests, looks it blocked on the config one ?

Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you wanted that ?

uvicorn/server.py Outdated Show resolved Hide resolved
Co-authored-by: euri10 <euri10@users.noreply.github.com>
@euri10 euri10 self-requested a review December 1, 2020 15:11
@florimondmanca
Copy link
Member Author

@euri10 I'll merge this and then tentatively release as 0.13.0… I'm not sure whether this should be considered a new feature or a bug fix, but since it can potentially mean some people will have to undo some scaffolding, I'd treat this as an "Added" entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants