Revert "When receiving a SIGTERM supervisors should terminate their processes before joining them"

[Kludex]

Reverts #1069

Closes #1160

I've taken my time to understand the situation. I'm going to explain it and then suggest a solution to the previous issue.

Right now, the issue on #1160 (CancelledError) is caused because the CTRL + C sends a SIGINT (2) to both parent and child processes, and given that we merged #1069, we have that the parent is also sending a SIGTERM (15) to the child. In other words, the child is receiving two signals, and the way uvicorn deals with multiple signals (two) is to forcefully exit the process.

To be more precise, when we press CTRL + C we send a kill signal to the process group, not to a single process.

Ok. Now, let's go back to the original issue: if we send SIGINT to the parent process, it doesn't terminate the children. And that's expected, because kill -2 <uvicorn_pid> will only send a signal to the <uvicorn_pid>.

The solution here would be to use kill -2 -<uvicorn_pid>, which sends a signal to the process group instead of only the parent. That being said, this solves the issue that #1069 (process.terminate()) solved, but we also avoid #1160 (CancelledError).

Reference: https://stackoverflow.com/a/392155

[Kludex]

I don't think we can avoid reverting that PR, but for reference, we can also do something like this:

https://github.com/benoitc/gunicorn/blob/1299ea9e967a61ae2edebe191082fd169b864c64/gunicorn/workers/gthread.py#L176-L181

In other words, check if the parent is alive, and then terminate the worker if it's not.

[Kludex]

hypercorn just ignores the SIGINT before creating the processes: https://gitlab.com/pgjones/hypercorn/-/blob/master/src/hypercorn/run.py#L49-52. Then it adds a handler after: https://gitlab.com/pgjones/hypercorn/-/blob/master/src/hypercorn/run.py#L70-72.

[Kludex]

@euri10 Can you check this, please?

[Asday]

The solution here would be to use kill -2 -<uvicorn_pid>, which sends a signal to the process group instead of only the parent.

I was originally interested in this issue (i.e., the previous one) because it was annoying to wait for my docker container to get killed on a timeout. Keeping in mind that I am a stupid stupid idiot who can barely read my own name, does this mean that my docker environment is going to become rubbish again, or is there some way to add a hyphen to me running docker-compose stop or ^Cing docker-compose up?

[HansBrende]

@Asday if you want the container to be killed immediately without waiting for a graceful shutdown I think you could do:

docker-compose kill

[Asday]

That's not at all what I want, I want the SIGTERM to gracefully shut down uvicorn, including its workers.

[HansBrende]

Hm... if there is really no way to signal to docker to terminate the process group instead of only the parent process (which I don't know if that's the case or not because I'm not an expert on the ins and outs of docker (but if it is, that would seem a bit ridiculous in light of standard CTRL+C behavior)), one other option would be, instead of reverting the old PR, to never force shutdown on SIGTERMs... only on the second SIGINT, specifically. Which is basically equivalent to a monkeypatch I did to get around this:

def _handle_exit(self, sig, frame):
    if self.should_exit and sig == signal.Signals.SIGINT:
        self.force_exit = True
    else:
        self.should_exit = True
 
uvicorn.server.Server.handle_exit = _handle_exit

Oooh, another option I just thought of: rather than starting the server process as a foreground process, the reloader process could start the server as a background process. In that case, CTRL+C would not send a SIGINT to the server process since it specifically only sends SIGINTs to "the foreground process group" (See spec), which would align behavior between CTRL + C and docker containers (I think). But I really have only spent about a half-hour researching this so I could be totally off-base. Any thoughts @Kludex @euri10 ?