Cooperative signal handling #1600

maxfischer2781 · 2022-08-10T12:19:30Z

I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
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.

This PR adjust the signal handling of uvicorn.server.Server to integrate with asyncio applications by not suppressing shutdown signals. Major changes include:

Original signal handlers are restored before Server.serve finishes
When terminated by a signal, the appropriate original signal handler is invoked if possible

In the usual case, this allows CTRL+C to end both uvicorn and the containing async application. Closes #1579.

There is a caveat for tests in that signal handling on Windows is not as precise as on Unix (see e.g. Stackoverflow: How to handle a signal.SIGINT on a Windows OS machine?). For testing, I could not setup a situation where the tests receive a signal without the test environment also being interrupted by it.

JoanFM · 2022-11-18T09:47:39Z

Hey @maxfischer2781 ,

I wonder if your PR would make this PR #1768 unnecessary?

JoanFM · 2022-11-18T10:01:52Z

Hey @maxfischer2781 ,

I wonder if your PR would make this PR #1768 unnecessary?

Yes, I think ur implementation is better and involves less magic

JoanFM · 2022-11-18T10:02:52Z

It may be nice to add a test covering SIGTERM signal handling?

maxfischer2781 · 2022-11-18T10:03:40Z

Hey @maxfischer2781 ,

I wonder if your PR would make this PR #1768 unnecessary?

@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 · 2022-11-18T10:46:49Z

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:

loop.add_signal_handler(...)

instead of

signal.signal(...)

?

maxfischer2781 · 2022-11-18T12:28:02Z

One question I have, will this PR respect the signals passed to an event loop as:
loop.add_signal_handler(...)
instead of
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 · 2022-11-18T15:05:10Z

One question I have, will this PR respect the signals passed to an event loop as:
loop.add_signal_handler(...)
instead of
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 · 2022-11-18T15:39:52Z

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 · 2022-11-18T18:53:07Z

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

Does this solve #1301 as well?

Ok, review done. Happy new year! 😎

setup.cfg

Kludex · 2022-12-31T17:31:40Z

uvicorn/server.py

+        try:
+            return asyncio.run(self.serve(sockets=sockets))
+        except KeyboardInterrupt:
+            pass

    async def serve(self, sockets: Optional[List[socket.socket]] = None) -> None:


Can we use the _capture_exit_signals as decorator instead? It would generate less footprint.

Suggested change

async def serve(self, sockets: Optional[List[socket.socket]] = None) -> None:

@capture_signals

async def serve(self, sockets: Optional[List[socket.socket]] = None) -> None:

In the suggestion, capture_signals = _capture_exit_signals.

It's doable but would create a similar/larger footprint at another place.

In principle it is possible to refactor _capture_exit_signals to work as a decorator as well: a contextlib.contextmanager decorated function can be used as a decorator. However, this does not work for methods out of the box; I would have to write a new contextlib.contextmanager.

I've experimented a bit with this but ultimately decided to drop my sketch for a context manager method. Stacking a context manager on top of a descriptor (for the self binding) on top of async def is much more complexity than a regular contextmanager.

Let me know if you would prefer the decorator variant anyway.

I'm thinking if we should do something like:

async def serve(self, sockets): with self._capture_exit_signals(): self._serve(sockets)

This is not a suggestion, it's a thought.

Good thought. The diff is much more on point like this.

uvicorn/server.py

Kludex · 2022-12-31T17:35:45Z

uvicorn/server.py

+                # Windows Caveat: We have no guarantee that we can correctly send
+                #   the signal (due to CTRL+C as SIGINT) to the process (due to groups)!
+                #   For now, do not do anything in this situation.


Can we have some reference instead?

I've added a reference to an Stack Overflow description of Windows signal handling and (re-)raising SIGINT/CTRL+C. Should I remove the other comments in the method?

Kludex · 2022-12-31T17:39:30Z

uvicorn/server.py

+                signal.signal(sig, handler)
+            # if we did gracefully shut down due to a signal, try to
+            # trigger the expected behaviour for this signal now
+            if self._delayed_signal is not None:


Can you help me understand a bit better why this is needed, and the scenario on which this is triggered?

If I got it right, this is triggered on SIGINT or SIGTERM, due to handle_exit, and it's triggered after the server is already down. But... Why do we need to call the original handlers? Why the os.kill below is not enough?

The scenario is as you understood.

Calling the original signal handler directly allows avoiding the non-portability of os.kill, i.e. it works the same on Windows and others.
Would you prefer the code to unconditionally use os.kill on non-Windows? That would probably be more in line with what people are used to.

I've changed the order so that the common UNIX case is handled first. The workarounds are only tried on Windows-like platform. This should make it clearer what is the UNIX case and what are the workarounds.

uvicorn/server.py

Kludex · 2023-01-01T10:42:50Z

Relevant: #1708

maxfischer2781 · 2023-01-17T10:30:50Z

@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.

Kludex

I'll check this better later on.

setup.cfg

Kludex · 2023-01-17T10:54:58Z

uvicorn/server.py

+        try:
+            return asyncio.run(self.serve(sockets=sockets))
+        except KeyboardInterrupt:
+            pass

    async def serve(self, sockets: Optional[List[socket.socket]] = None) -> None:


I'm thinking if we should do something like:

async def serve(self, sockets): with self._capture_exit_signals(): self._serve(sockets)

This is not a suggestion, it's a thought.

uvicorn/server.py

maxfischer2781 · 2023-04-14T18:37:56Z

@Kludex Do you still see any required changes? As far as I can tell I should have everything covered.

DrInfiniteExplorer · 2023-06-04T14:56:10Z

@Kludex It would be super to see this merged! 🙏

br3ndonland · 2023-06-04T18:32:33Z

@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 a checklist. For your PR, I think it would currently look like this:

I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
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 that could work for this (it's here in the repo code). 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 · 2023-06-11T19:35:20Z

@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 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 interface 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.

br3ndonland · 2023-06-17T00:12:24Z

I think 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 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 · 2023-08-16T22:46:11Z

Has it been decided to leave the broken behaviour?

maxfischer2781 · 2023-08-21T16:13:37Z

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.

maxfischer2781 · 2024-03-10T20:23:11Z

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 · 2024-03-11T11:04:11Z

Oh! You shortened a lot of the diff here. So cool! I'll check in some hours.

docs/index.md

Kludex · 2024-03-11T12:09:26Z

uvicorn/server.py

+        # 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}


Why did we use loop.add_signal_handler before?

I haven't found a technical reason for it. It seems to have been added in #141 for no specific and then copied over the years. I assume it was used because the asyncio docs promote it as a Linux feature that is better in unspecified ways.

On the technical side the only advantage of loop.add_signal_handler is that it allows synchronously triggering async code (e.g. event.set()), and the handlers don't use that. Since the code has to be compatible with Windows, handlers cannot rely on being invoked by the loop anyways.

Kludex · 2024-03-11T12:10:22Z

tests/test_server.py

+
+@pytest.mark.anyio
+@pytest.mark.skipif(sys.platform == "win32", reason="require unix-like signal handling")
+@pytest.mark.parametrize("exception_signal", [signal.SIGTERM, signal.SIGINT])


What happens with signal.SIGBREAK on Windows?

We added this to HANDLED_SIGNALS some months ago.

I've added that case now.

docs/index.md

Kludex · 2024-03-19T08:39:00Z

Thanks for not giving up @maxfischer2781 🙏

I'll make a release shortly.

vytas7 · 2024-03-20T20:04:42Z

In Falcon's test suite, we used to check the return code for functional tests of ASGI servers. Since we terminate the server via SIGTERM, Uvicorn now returns -15 which is a proper way in Unix. However, when running Actions on Windows, we are getting codes like 3221225786 == 2**31 + 2**30 + 314. Is this expected on Windows, and how to interpret these values? @Kludex

maxfischer2781 · 2024-03-20T20:50:08Z

3221225786 is the unsigned interpretation of Windows' 0xC000013A / STATUS_CONTROL_C_EXIT. This is a magic value just like signal 15 on UNIX.

vytas7 · 2024-03-20T21:56:50Z

I see, thanks @maxfischer2781!

This comment was marked as outdated.

Sign in to view

maxfischer2781 marked this pull request as draft August 12, 2022 16:35

maxfischer2781 marked this pull request as ready for review August 15, 2022 13:42

zanieb mentioned this pull request Oct 26, 2022

uvicorn eats SIGINTs, does not propagate exceptions #1579

Closed

JoanFM mentioned this pull request Nov 18, 2022

feat: compose already existing signal handling #1768

Closed

JoanFM approved these changes Nov 18, 2022

View reviewed changes

Kludex requested changes Dec 31, 2022

View reviewed changes

Kludex added the waiting author Waiting for author's reply label Dec 31, 2022

Kludex reviewed Jan 17, 2023

View reviewed changes

maxfischer2781 force-pushed the forward-asyncio-signal branch from 5255717 to aa44f36 Compare January 18, 2023 12:53

BilalAlpaslan approved these changes Jun 4, 2023

View reviewed changes

maxfischer2781 added 4 commits March 10, 2024 17:45

checks

ab22bc5

test asyncio handlers

89f6751

add note on signal handler handling

6c53405

remove legacy signal raising

8682a94

maxfischer2781 requested review from Kludex, BilalAlpaslan and JoanFM March 10, 2024 20:24

Kludex reviewed Mar 11, 2024

View reviewed changes

maxfischer2781 and others added 4 commits March 12, 2024 20:33

test SIGBREAK on windows

103e51b

remove test guard

c8018a6

include convered branch

4abad94

Update docs/index.md

3dd7f80

Kludex reviewed Mar 19, 2024

View reviewed changes

docs/index.md Outdated Show resolved Hide resolved

Kludex added 3 commits March 19, 2024 02:32

Update docs/index.md

bb99977

Merge branch 'master' into forward-asyncio-signal

bb102dd

Merge branch 'master' into forward-asyncio-signal

ce740d8

Kludex approved these changes Mar 19, 2024

View reviewed changes

Kludex enabled auto-merge (squash) March 19, 2024 08:35

Kludex removed the waiting author Waiting for author's reply label Mar 19, 2024

Kludex mentioned this pull request Mar 19, 2024

Version 0.29.0 #2279

Merged

1 task

Kludex disabled auto-merge March 19, 2024 08:45

Kludex merged commit 9e32e8e into encode:master Mar 19, 2024
15 checks passed

maxfischer2781 mentioned this pull request Mar 19, 2024

Remove uvicorn KeyboardInterrupt workaround MatterMiners/tardis#340

Open

maxfischer2781 mentioned this pull request Apr 26, 2024

Suppress side-effects of signal propagation #2317

Open

3 tasks

	async def serve(self, sockets: Optional[List[socket.socket]] = None) -> None:
	@capture_signals
	async def serve(self, sockets: Optional[List[socket.socket]] = None) -> None:

Cooperative signal handling #1600

Cooperative signal handling #1600

Conversation

maxfischer2781 commented Aug 10, 2022 • edited

This comment was marked as outdated.

This comment was marked as outdated.

JoanFM commented Nov 18, 2022

JoanFM commented Nov 18, 2022

JoanFM commented Nov 18, 2022

maxfischer2781 commented Nov 18, 2022 • edited

JoanFM commented Nov 18, 2022 • edited

maxfischer2781 commented Nov 18, 2022

JoanFM commented Nov 18, 2022

Kludex commented Nov 18, 2022

JoanFM commented Nov 18, 2022

Kludex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxfischer2781 Jan 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxfischer2781 Jan 13, 2023 • edited

Choose a reason for hiding this comment

Kludex commented Jan 1, 2023

maxfischer2781 commented Jan 17, 2023

Kludex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxfischer2781 commented Apr 14, 2023

DrInfiniteExplorer commented Jun 4, 2023

br3ndonland commented Jun 4, 2023 • edited

maxfischer2781 commented Jun 11, 2023 • edited

br3ndonland commented Jun 17, 2023

tesioai commented Aug 16, 2023

maxfischer2781 commented Aug 21, 2023

maxfischer2781 commented Mar 10, 2024

Kludex commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex commented Mar 19, 2024

vytas7 commented Mar 20, 2024 • edited

maxfischer2781 commented Mar 20, 2024

vytas7 commented Mar 20, 2024

maxfischer2781 commented Aug 10, 2022 •

edited

maxfischer2781 commented Nov 18, 2022 •

edited

JoanFM commented Nov 18, 2022 •

edited

maxfischer2781 Jan 18, 2023 •

edited

maxfischer2781 Jan 13, 2023 •

edited

br3ndonland commented Jun 4, 2023 •

edited

maxfischer2781 commented Jun 11, 2023 •

edited

vytas7 commented Mar 20, 2024 •

edited