Repository: encode/uvicorn

Issue:
uvicorn eats SIGINTs, does not propagate exceptions
The following snippet cannot be killed with a SIGINT (ctrl+c):

```python
import asyncio

from starlette.applications import Starlette
from uvicorn import Config, Server

async def web_ui():
    await Server(Config(Starlette())).serve()

async def task():
    await asyncio.sleep(100000000000000)

async def main():
    await asyncio.gather(web_ui(), task())

if __name__ == "__main__":
    asyncio.run(main())
```

It appears that uvicorn is eating SIGINTs and does not propagate the KeyboardInterrupt and/or Cancelled exceptions.   Thanks for having a look.

<!-- POLAR PLEDGE BADGE START -->
> [!IMPORTANT]
> - We're using [Polar.sh](https://polar.sh/encode) so you can upvote and help fund this issue.
> - We receive the funding once the issue is completed & confirmed by you.
> - Thank you in advance for helping prioritize & fund our backlog.

<a href="https://polar.sh/encode/uvicorn/issues/1579">
<picture>
  <source media="(prefers-color-scheme: dark)" srcset="https://polar.sh/api/github/encode/uvicorn/issues/1579/pledge.svg?darkmode=1">
  <img alt="Fund with Polar" src="https://polar.sh/api/github/encode/uvicorn/issues/1579/pledge.svg">
</picture>
</a>
<!-- POLAR PLEDGE BADGE END -->


Discussion:
maxfischer2781: Looks like I have to read up on Windows signals a bit. Not sure if the code or test is at fault here.
maxfischer2781: I need help how to proceed at this point with regards to coverage. The changes are covered by tests, *but* the tests cannot be run on Windows and thus the coverage check fails there.

As far as I can tell, there is no way to test this on Windows without significantly more setup. A subprocess would be required to use CREATE_NEW_PROCESS_GROUP in order to contain the interrupt to just the test.
JoanFM: Hey @maxfischer2781 ,

I wonder if your PR would make this PR #1768 unnecessary?
JoanFM: It may be nice to add a test covering `SIGTERM` signal handling?
maxfischer2781: @JoanFM Possibly, but the two changes have slightly different behaviour. This PR will trigger the original handler *after* the univocrn shutdown procedure. #1768 will trigger the original handler *before* the uvicorn shutdown procedure.

As a default, I think triggering the original handlers late is better – many handlers shutdown quickly (namely, the default handlers) and would skip the uvicorn shutdown entirely. It might be worth having a method to chain handlers explicitly, though.
JoanFM: Yes, plus your PR does not use any private function and would work with more `loops`.

One question I have, will this PR respect the signals passed to an `event loop` as:

```python
loop.add_signal_handler(...)
```
instead of 

```python
signal.signal(...)
```

¿
maxfischer2781: > One question I have, will this PR respect the signals passed to an `event loop` as:
> 
> ```python
> loop.add_signal_handler(...)
> ```
> 
> instead of
> 
> ```python
> signal.signal(...)
> ```
> 
> ?

Sadly it won't. There is no clean way to query the loop for registered handlers – handlers are stored in a private attribute of the loop and methods allow only to set or delete handlers by signal number.
JoanFM: > > One question I have, will this PR respect the signals passed to an `event loop` as:
> > ```python
> > loop.add_signal_handler(...)
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > instead of
> > ```python
> > signal.signal(...)
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > ?
> 
> Sadly it won't. There is no clean way to query the loop for registered handlers – handlers are stored in a private attribute of the loop and methods allow only to set or delete handlers by signal number.

Okey, but signal.signal should work the same except if I need to interact with event loop itself.

Any idea when this might be merged?
Kludex: > Any idea when this might be merged?

Is it me, or this question sounds a bit presumptious? 😅

Thanks for the PR @maxfischer2781 , and sorry the delay. This requires a bit of my focus, as there are many ideias related around.

I'll check the whole signal thingy for 0.23.0. I'm about to release 0.22.0.

No estimation can be provided, since I work on uvicorn on my free time.
JoanFM: > > Any idea when this might be merged?
> 
> Is it me, or this question sounds a bit presumptious? 😅
> 
> Thanks for the PR @maxfischer2781 , and sorry the delay. This requires a bit of my focus, as there are many ideias related around.
> 
> I'll check the whole signal thingy for 0.23.0. I'm about to release 0.22.0.
> 
> No estimation can be provided, since I work on uvicorn on my free time.

Sorry @Kludex, No bad intentions in this question. I really appreciate the work you guys do and understand it is not easy to dedicate time.

I just do not know how this specific project worked.

Thanks for everything
Kludex: Relevant: https://github.com/encode/uvicorn/discussions/1708
maxfischer2781: @Kludex I've addressed/implemented all suggestions now. Please take another look once you find the time.

I don't think an approach as suggested in #1708 makes sense in uvicorn: Practically, signal handling is a two-step approach of 1) handling the signal and setting the shutdown flag and 2) actually shutting down the server. That means one cannot run a "regular" signal handler before or after the uvicorn signal handler (1), since one needs to wait for the server to shut down (2) first.
maxfischer2781: @Kludex Do you still see any required changes? As far as I can tell I should have everything covered.
DrInfiniteExplorer: @Kludex It would be super to see this merged! 🙏 
br3ndonland: > @Kludex Do you still see any required changes? As far as I can tell I should have everything covered.

@maxfischer2781 thanks for your work on this. Since you opened the PR, we've added a PR template with this checklist:

- [x] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
- [x] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
- [ ] I've updated the documentation accordingly.

So it seems like what's missing is some documentation of the server's signal handling behavior. We have a [docs page on server behavior](https://www.uvicorn.org/server-behavior/) that could work for this ([it's here in the repo code](https://github.com/encode/uvicorn/blob/3924ef4d48a5778d3465666bb898245dbfbf8fe5/docs/server-behavior.md)). If you could add a section there and clearly explain how signal handling will work after this PR, I think it would help everyone understand this better.

After some docs are added to explain the signal handling behavior, I'm happy to give this PR a review as well.

maxfischer2781: @br3ndonland Thanks for the reply, I'll prepare proper docs. Can you give me some input on things I would change for making this "official" via documentation:

* I think [Deployment > Running programmatically](https://www.uvicorn.org/deployment/#running-programmatically) would be a more suitable place. The exact behaviour is only relevant for people that may fiddle with signals, to everyone else it'll "just work" now.
    * Would this location be fine?
* If this gets documented as something programmers can rely on, I would prefer to make it portable and consistent. Right now the Windows and Unix versions behave slightly differently: the Unix version uses the [`asyncio` signal](https://docs.python.org/3/library/asyncio-eventloop.html#unix-signals) interface but does not use its capabilities. This makes recovering a some signal handlers impossible, but technically some user code may rely on the `asyncio` capabilites.
     *  Would consistently using the plain `signal` package be fine? This is technically a breaking change, though not for documented behaviour.
br3ndonland: > I think [Deployment > Running programmatically](https://www.uvicorn.org/deployment/#running-programmatically) would be a more suitable place.

Sure! This PR is about running the server programmatically, so either the server behavior page or the deployment docs on running programmatically could work. Thanks for the suggestion.

> If this gets documented as something programmers can rely on, I would prefer to make it portable and consistent. Right now the Windows and Unix versions behave slightly differently: the Unix version uses the [`asyncio` signal interface](https://docs.python.org/3/library/asyncio-eventloop.html#unix-signals) but does not use its capabilities. This makes recovering some signal handlers impossible, but technically some user code may rely on the `asyncio` capabilites.
>
> Would consistently using the plain `signal` package be fine? This is technically a breaking change, though not for documented behaviour.

You make a good point here. I agree that the implementation should be as portable and consistent as possible, but given the long discussion in this PR and in #1579, it might be most pragmatic to keep the existing behavior as-is and just document it. I'm happy to take a look at the code either way.
tesioai: Has it been decided to leave the broken behaviour?
Kludex: > Has it been decided to leave the broken behaviour?

No.
maxfischer2781: I'm still committed to finishing this but seeing how long this dragged on my initial time window for working on it is gone. It may take about 1-4 weeks until I have time to work on this again.
br3ndonland: > I'm still committed to finishing this but seeing how long this dragged on my initial time window for working on it is gone. It may take about 1-4 weeks until I have time to work on this again.

No problem. Thanks for checking in.
lukasK9999: Hello, documentation is only blocker of merging this PR? 
mitaraaa: Any update on this?
maxfischer2781: To give an update here: I got delayed beyond my initial time window but am currently catching up with changes in `master` (this should not take much time but means a force push).

However, adding the changes to the docs means they are sort-of established policy, so should be bulletproof. I had to do some extra research on cross compatibility since both the windows and unix version currently behave slightly different and I need to fix that for programmers to rely on it.

At least the code/docs changes from my side should be done within about a week.
Kludex: Thanks.
maxfischer2781: I've recreated the branch from scratch now since the original state was heavily outdated and had a nasty git history. The changes are simpler now as Windows/Unix and Python version special cases are gone.

- simpler than original code because `signal.raise_signal` works on every supported version now
- use `signal.signal` on any platform since `asyncio…add_signal_handler` does not allow restoring handlers
    - this has no downside since the handler does not interact with the event loop in any case
- the "main" signal handler method is now called `capture_signals` instead of `install_signal_handlers` since changes are not backwards compatible
- the test setup is inverted to test third-party `asyncio…add_signal_handler` as well

The behaviour is documented on "Index" -> "Running programmatically" since this is the only place where the `server.serve` method is shown.
Kludex: Oh! You shortened a lot of the diff here. So cool! I'll check in some hours.
Kludex: Thanks for not giving up @maxfischer2781 🙏

I'll make a release shortly.

Patch:
diff --git a/uvicorn/server.py b/uvicorn/server.py
index c7645f3..bfce1b1 100644
--- a/uvicorn/server.py
+++ b/uvicorn/server.py
@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import asyncio
+import contextlib
 import logging
 import os
 import platform
@@ -11,7 +12,7 @@ import threading
 import time
 from email.utils import formatdate
 from types import FrameType
-from typing import TYPE_CHECKING, Sequence, Union
+from typing import TYPE_CHECKING, Generator, Sequence, Union
 
 import click
 
@@ -57,11 +58,17 @@ class Server:
         self.force_exit = False
         self.last_notified = 0.0
 
+        self._captured_signals: list[int] = []
+
     def run(self, sockets: list[socket.socket] | None = None) -> None:
         self.config.setup_event_loop()
         return asyncio.run(self.serve(sockets=sockets))
 
     async def serve(self, sockets: list[socket.socket] | None = None) -> None:
+        with self.capture_signals():
+            await self._serve(sockets)
+
+    async def _serve(self, sockets: list[socket.socket] | None = None) -> None:
         process_id = os.getpid()
 
         config = self.config
@@ -70,8 +77,6 @@ class Server:
 
         self.lifespan = config.lifespan_class(config)
 
-        self.install_signal_handlers()
-
         message = "Started server process [%d]"
         color_message = "Started server process [" + click.style("%d", fg="cyan") + "]"
         logger.info(message, process_id, extra={"color_message": color_message})
@@ -302,22 +307,28 @@ class Server:
         for server in self.servers:
             await server.wait_closed()
 
-    def install_signal_handlers(self) -> None:
+    @contextlib.contextmanager
+    def capture_signals(self) -> Generator[None, None, None]:
+        # Signals can only be listened to from the main thread.
         if threading.current_thread() is not threading.main_thread():
-            # Signals can only be listened to from the main thread.
+            yield
             return
-
-        loop = asyncio.get_event_loop()
-
+        # always use signal.signal, even if loop.add_signal_handler is available
+        # this allows to restore previous signal handlers later on
+        original_handlers = {sig: signal.signal(sig, self.handle_exit) for sig in HANDLED_SIGNALS}
         try:
-            for sig in HANDLED_SIGNALS:
-                loop.add_signal_handler(sig, self.handle_exit, sig, None)
-        except NotImplementedError:  # pragma: no cover
-            # Windows
-            for sig in HANDLED_SIGNALS:
-                signal.signal(sig, self.handle_exit)
+            yield
+        finally:
+            for sig, handler in original_handlers.items():
+                signal.signal(sig, handler)
+        # If we did gracefully shut down due to a signal, try to
+        # trigger the expected behaviour now; multiple signals would be
+        # done LIFO, see https://stackoverflow.com/questions/48434964
+        for captured_signal in reversed(self._captured_signals):
+            signal.raise_signal(captured_signal)
 
     def handle_exit(self, sig: int, frame: FrameType | None) -> None:
+        self._captured_signals.append(sig)
         if self.should_exit and sig == signal.SIGINT:
             self.force_exit = True
         else:


Test patch:
diff --git a/tests/test_server.py b/tests/test_server.py
new file mode 100644
index 0000000..dac8fb0
--- /dev/null
+++ b/tests/test_server.py
@@ -0,0 +1,67 @@
+from __future__ import annotations
+
+import asyncio
+import contextlib
+import signal
+import sys
+from typing import Callable, ContextManager, Generator
+
+import pytest
+
+from uvicorn.config import Config
+from uvicorn.server import Server
+
+
+# asyncio does NOT allow raising in signal handlers, so to detect
+# raised signals raised a mutable `witness` receives the signal
+@contextlib.contextmanager
+def capture_signal_sync(sig: signal.Signals) -> Generator[list[int], None, None]:
+    """Replace `sig` handling with a normal exception via `signal"""
+    witness: list[int] = []
+    original_handler = signal.signal(sig, lambda signum, frame: witness.append(signum))
+    yield witness
+    signal.signal(sig, original_handler)
+
+
+@contextlib.contextmanager
+def capture_signal_async(sig: signal.Signals) -> Generator[list[int], None, None]:  # pragma: py-win32
+    """Replace `sig` handling with a normal exception via `asyncio"""
+    witness: list[int] = []
+    original_handler = signal.getsignal(sig)
+    asyncio.get_running_loop().add_signal_handler(sig, witness.append, sig)
+    yield witness
+    signal.signal(sig, original_handler)
+
+
+async def dummy_app(scope, receive, send):  # pragma: py-win32
+    pass
+
+
+if sys.platform == "win32":
+    signals = [signal.SIGBREAK]
+    signal_captures = [capture_signal_sync]
+else:
+    signals = [signal.SIGTERM, signal.SIGINT]
+    signal_captures = [capture_signal_sync, capture_signal_async]
+
+
+@pytest.mark.anyio
+@pytest.mark.parametrize("exception_signal", signals)
+@pytest.mark.parametrize("capture_signal", signal_captures)
+async def test_server_interrupt(
+    exception_signal: signal.Signals, capture_signal: Callable[[signal.Signals], ContextManager[None]]
+):  # pragma: py-win32
+    """Test interrupting a Server that is run explicitly inside asyncio"""
+
+    async def interrupt_running(srv: Server):
+        while not srv.started:
+            await asyncio.sleep(0.01)
+        signal.raise_signal(exception_signal)
+
+    server = Server(Config(app=dummy_app, loop="asyncio"))
+    asyncio.create_task(interrupt_running(server))
+    with capture_signal(exception_signal) as witness:
+        await server.serve()
+    assert witness
+    # set by the server's graceful exit handler
+    assert server.should_exit