Enable graceful shutdown when running multiple workers and sending a SIGTERM

[euri10]

Fixes #852

This is quite a huge review, so happy to explain anything that might look not easily understandable.

As @florimondmanca noticed, it takes inspiration from hypercorn by handling the multiple process shutdown through an external multiprocessing.Event that defines an infinite loop. That loop will break when that external event is set.

Where it deviates from hypercorn implementation is in the reloader case : I took the view that --reload is a particular case of --workers where there is a restart method that is invoked when a file is modified.

If necessary I can comment on the diff the main takeaways, let me know. It took me quite a while to get it right and hopefully it's working, I hope there are not leftovers from previous attempts.

one pretty big caveat but it's maybe doable, it removes support for python 3.6 : I'm using server.serve_forever() which appears in 3.7 and couldn't for this find a way to handle its absence in 3.6 (there is another case where it was quite easy to adapt for 3.6) not using server.serve_forever in fact

log of a gentle sigterm

/home/lotso/PycharmProjects/uvicorn/venv/bin/python -m uvicorn apps.app:app --workers 2 --log-level=debug
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     Started parent process [16790]
DEBUG:    run args:() kwargs:{'config': <uvicorn.config.Config object at 0x7fcff87073a0>, 'shutdown_event': <multiprocessing.synchronize.Event object at 0x7fcff78028e0>}
DEBUG:    setting multiprocess trigger using : <multiprocessing.synchronize.Event object at 0x7fcff78028e0>
DEBUG:    run args:() kwargs:{'config': <uvicorn.config.Config object at 0x7fcff87073a0>, 'shutdown_event': <multiprocessing.synchronize.Event object at 0x7fcff78028e0>}
DEBUG:    setting multiprocess trigger using : <multiprocessing.synchronize.Event object at 0x7fcff78028e0>
INFO:     Started server process [16791]
INFO:     Waiting for application startup.
INFO:     Started server process [16792]
INFO:     Waiting for application startup.
INFO:     ASGI 'lifespan' protocol appears unsupported.
INFO:     Application startup complete.
INFO:     ASGI 'lifespan' protocol appears unsupported.
INFO:     Application startup complete.
INFO:     going to await shutdown
INFO:     going to await shutdown
DEBUG:    MultiServer received: 15
DEBUG:    multiprocessing event set
INFO:     will raise shutdown
DEBUG:    multiprocessing event set
INFO:     will raise shutdown
DEBUG:    raised shutdown exc: 
INFO:     Shutting down
DEBUG:    raised shutdown exc: 
INFO:     Shutting down
INFO:     Finished server process [16791]
INFO:     Finished server process [16792]
INFO:     Stopping parent process [16790]

Process finished with exit code 0

[euri10]

I rebased this against master with new tests, adapted the run_server context manager to cope with how signals are handled in this version.
I tested gunicorn, uvicorn reload on both flavors, multiple uvicorn workers, sigterm and sigint gracefully shutdown the server and all its workers, no more hanging.

I'm quite happy with it, except for the 3.6 drop, but maybe we can keep it for after the handler change, since I dropped 3.6 because I'm using server.serve_forever()

In any case, after or before, the logic at hand wont change much and it's a pretty neat addition.
having clean shutdown is cool for orchestrators mostly, killing pods will likely be way faster with this

[florimondmanca]

@euri10 This is looking interesting, but I must say that's quite a lot of code and changes to go through. It seems you're kind of doing the same trick (using a multiprocessing event) three times. Would there be any chance you could start with only one bit, say hot reload (or whatever is easiest to add first), so we can see more easily what the various pieces are? Just asking, if it's all interlinked then okay, I can try and take the time to sit down and go through this 😄 but if there are ways to scale things down in increments, that would be interesting too...

[florimondmanca]

@euri10 Taking a closer look at this PR, I view the introduction of self.tasks and other server-coupled state as a rather bad smell when considering support for other async libraries.

I have a feeling a prerequisite for this would be to start moving asyncio-specific pieces out of Server, and switch Server slightly so that it only exposes a await serve() coroutine that does the startup/shutdown behavior, rather than exposing .startup() and .shutdown() methods. This allows managing serve-local state much more easily (with context managers). I'm basing all this on what I found while working on #863 — and I think a lot of what's there could be used for inspiration.

[euri10]

Ok will wait then, not sure sure how to 🍕 slice it right now but eventually this will come.

[gnat]

I've also given the changes a review, it looks good to me. Also it's a net addition of only 30 lines.

I tested gunicorn, uvicorn reload on both flavors, multiple uvicorn workers, sigterm and sigint gracefully shutdown the server and all its workers, no more hanging.

This is a major win for uvicorn infrastructure and solves a serious pain point.

I'm quite happy with it, except for the 3.6 drop, but maybe we can keep it for after the handler change, since I dropped 3.6 because I'm using server.serve_forever()

This should NOT be a concern because:

1. Python async is so new that depriving ourselves of newer versions can have serious ramifications on the progress of our entire ecosystem.
2. Anyone concerned about graceful shutdown are the serious users of Uvicorn.
3. Those honestly stuck on 3.6 in production wont be upgrading their other dependencies anyway. And highly doubtful they are running uvicorn given the problem this patch solves in the first place.

Thank you @euri10 for the great work here and @florimondmanca for the feedback!!

We should do what we can to move this feature forward.

[deltarod]

Is there an ETA on this merge?

[danrossi]

I am also suffering this same problem. Cannot exit properly with sigterm if workers are used on Windows. Is there a fix?

[temoto]

@euri10 what was wrong? a better patch is coming?