Fix graceless ctrl C

[HansBrende]

Alternative to #1165. Fixes #1160.

Edit by @Kludex: Closes #1165

[HansBrende]

@euri10 you've been silent on #1165 so I wanted to propose another fix for this issue such that termination signals are always handled gracefully, regardless of whether sent to only the parent process OR the entire foreground process group. LMK...

[dimaqq]

Simple and elegant... though it's hard to say if it works.
It would be nice to have a test, but I guess that could be hard perhaps?

[HansBrende]

Thanks @dimaqq. I'm cool with adding a test (implementation suggestions welcomed) if I get buy-in on this PR from a maintainer (e.g. @Kludex or @euri10)... otherwise I don't want to waste my time on that. But so far everyone has been strangely quiet! And a mysterious "hold" label has appeared to boot. 🧐 Maintainers' thoughts welcomed...

Easy enough to see that it works, however, by contrasting ctrl-C behavior with/without this tweak (just beware of any testing via class monkeypatching in the main process... the monkeypatch won't persist to the child process. That bit me once.)

[Kludex]

Currently, we have the following behavior:

1. SIGTERM = SIGINT
2. Once one of the above arrives, we start the graceful shutdown.
3. Once the second arrives, we force the process to shutdown.
4. Posterior SIGINT/TERM are ignored.

This PR proposes to change 3. in a way that only SIGINT is considered on that case. Just a fact, not saying it's bad.

• I have an alternative proposal to this PR on Create ProcessManager #1205:

  uvicorn/uvicorn/server.py

  Lines 312 to 316 in a4377a1

  	def handle_exit(self, sig: signal.Signals, frame: FrameType) -> None:
  	self.terminate_called += 1
  	self.should_exit = True
  	if self.terminate_called > 2:
  	self.force_exit = True

The one I've implemented there shifts the forceful shutdown to a third SIGINT/TERM received instead.
This is useful in the context of a process manager, as right now gunicorn doesn't care about shutdown events - and that PR proposes a manager that will perform the lifespan event.

For now this solution makes sense, as it just solves the issue in hands. but I'm more willing to use the strategy I propose on #1205 at some point, so I'm not sure if we should go with this for now, and then change later, or just go with that, as it also solves the issue we have.

In any case, I need to confirm that another member will be willing with my proposal on #1205. @euri10 What do you think?

As for the hold label, it's just to let others know that someone wants to say something about the PR. 😗

[HansBrende]

@Kludex changing to a 3rd sigint/sigterm would work as well, of course. However... that has the (IMO) undesirable effect of requiring 3 CTRL+C's to force-shutdown an app running in terminal!

As far as I know, there are really only 3 use-cases that anyone cares about here:

1. All termination signals should trigger at least a graceful shutdown
2. Don't force exit unless explicitly requested
3. CTRL+C x2 (i.e. SIGINT) = explicit request

I don't think anyone cares about force-shutdown after 2 SIGTERMs. Where is the use-case for that? The only things sending SIGTERMs are the process supervisors, which generally have their own force-kill logic (e.g. docker, kubernetes) so uvicorn doesn't really need to worry about handling force kills in those cases anyway (and these frameworks certainly don't signal a second time to be cute... it is SIGKILL after the first attempt has timed out.)

TL;DR: treating SIGINT & SIGTERM as exactly the same always (including for the purposes of a force-shutdown) may be slightly misguided, since CTRL C sends a SIGINT, and not a SIGTERM, to the foreground process group of the terminal, and chances are, any "supervising" process (including a human in a 2nd shell tab 😜) will not bother sending a 2nd SIGTERM if the first one failed, but rather a SIGKILL (whereas CTRL+C twice to force exit is fairly standard & convenient).

[dimaqq]

re:

* I have an alternative proposal to this PR on [Create ProcessManager #1205](https://github.com/encode/uvicorn/pull/1205): https://github.com/encode/uvicorn/blob/a4377a14e3360fa487c506b49e8c5537fb0057a1/uvicorn/server.py#L312-L316

Should if self.terminate_called > 2: perhaps be if self.terminate_called >= 2: ? 🤔

[Kludex]

I've thought about this, and reread the alternative solution. I've come to the conclusion that it's more practical to have this in place.

About the future PR that I've mentioned, I don't think it makes sense to mention that here, as we have a long way before that is merged. But... I can make that PR work with this solution anyway, so that point was useless. Also, even if a change is needed, we can do that on the process manager itself instead of changing the handler on the Server.

re:

* I have an alternative proposal to this PR on [Create ProcessManager #1205](https://github.com/encode/uvicorn/pull/1205): https://github.com/encode/uvicorn/blob/a4377a14e3360fa487c506b49e8c5537fb0057a1/uvicorn/server.py#L312-L316

Should if self.terminate_called > 2: perhaps be if self.terminate_called >= 2: ? thinking

Not really, because handle_exit() will be called twice on the first signal, so terminate_called = 2.

Thanks for sticking with me here. 👍

[Kludex]

I don't think anyone cares about force-shutdown after 2 SIGTERMs. Where is the use-case for that?

None that I can think of, but someone could have done that, as it was possible. Also, we don't document that.