Handle signals #510
Handle signals #510
Conversation
|
add a click option in main ? |
uvicorn/config.py
Outdated
| @@ -172,6 +174,7 @@ def __init__( | |||
| self.ssl_ciphers = ssl_ciphers | |||
| self.headers = headers if headers else [] # type: List[str] | |||
| self.encoded_headers = None # type: List[Tuple[bytes, bytes]] | |||
| self.handled_signals = handled_signals | |||
There was a problem hiding this comment.
Let's use handled_signals or [] here, so that it's always a list.
That'll simplify the install_signal_handlers implementation slightly.
| loop = asyncio.get_event_loop() | ||
| else: | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) |
There was a problem hiding this comment.
yes if we keep the test_run_signal_multi_threads.
if this isn't modified here's what happens:
================================================================ 80 failed, 92 passed, 12 warnings in 9.79s ================================================================
this happens on all tests after this one, because the loop = asyncio.get_event_loop() I replaced above is closed now so you get a E RuntimeError: Event loop is closed for the 80 failed tests after
should we skip this test then this is not required but my personal guess is that it's a good example
| s.mount(f"http://127.0.0.1:{port}", HTTPAdapter(max_retries=1)) | ||
| response = s.get(f"http://127.0.0.1:{port}") | ||
| assert response.status_code == 204 | ||
| join_t(*workers) |
There was a problem hiding this comment.
I actually don't think we need to add these tests. We already have coverage of the code that's being used - that's enough for me here.
There was a problem hiding this comment.
fair enough
however I wanted to say that the main use case for this PR, if I understanded it correctly what people wanted, I mean using handled_signals=None is for people who may want a finer control over that Config part, so that they could run uvicorn in one thread and something else in another.
that test is effectively far-fetched in the sense it runs 10 servers in 10 threads, but it just demonstrates how it could be done,
skipping it removes the need for modifying the tests/client.py above since one-side effect of the test is that it as to close the loop!
uvicorn/config.py
Outdated
| @@ -139,6 +140,7 @@ def __init__( | |||
| ssl_ca_certs=None, | |||
| ssl_ciphers="TLSv1", | |||
| headers=None, | |||
| handled_signals=(signal.SIGINT, signal.SIGTERM), | |||
|
Thanks for the work @euri10! Anything left to do here? |
attempt to replace #402
I added 2 new tests (
test_run_signalandtest_run_signal_multi_threads) that make use of the flag and therefore don't need to use theCustomServerclass that override signal handling, I guess it's a test only workaround.I would need / love guidance in how to properly handle one of the tests here, namelytest_run_signal_multi_threads, because I have currenlty an issue:-test_run_signal_multi_threadsin essence spawns 10 threads, run uvicorn in them with each time a new loop, the test works fine independently however it needs animport asyncioto set loops. And this single import break about 80 other tests while on its own the test passes fine and we can have 10 uvicorn servers running in 10 threads with 10 loops...wellSo I left the PR in a state where all tests pass but this one because of the missing import.Hope it makes sense
ok I found the fix: because of the tests I added, the
_ASGIAdapterhad a closed loop it tried to use and therefore failed, so a simple test on it and all tests pass