-
-
Notifications
You must be signed in to change notification settings - Fork 756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve shutdown process in Multiprocess mode #2010
Conversation
for sock in self.sockets: | ||
sock.close() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to close the sockets here again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In UNIX-based OSes one needs to close all instances of a listening socket for it to unbind (incl. ones inherited by forks). In this case this includes:
- master process.
- all childen.
If you're hinting that in some OS'es (e.g. Windows, where my knowledge is quite limited) this would lead to an error I can wrap this in: try/except socket.error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can observe the behaviour of stalled http requests during shutdown with a server that has a health check http handler and a a shutdown lifespan handler:
@app.on_event("shutdown")
async def shutdown():
await asyncio.sleep(60)
@Kludex A friendly ping on this ^ |
Explicitly clos listening sockets in both parent and child processes early in the shutdown process. That way, no new connections can hit workers after shutdown is initiated by the parent process. While here, speedup shutdown by splitting process shutdown into terminate and join loop, so all processes are shutdown in parallel.
for sock in sockets or []: | ||
sock.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed.
asyncio.loop.create_server specifies:
Note The sock argument transfers ownership of the socket to the server created. To close the socket, call the server’s close() method.
asyncio.Server confirms that with:
close()(https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.Server.close)
Stop serving: close listening sockets and set the sockets attribute to None.
|
Summary
This PR explicitly closes listening sockets in parent processe early in the shutdown process. That way, no new connections can hit workers after shutdown is initiated by the parent process.
While here, speedup shutdown by splitting process shutdown into terminate and join loop, so all child processes are shutdown in parallel.
Current behaviour
Here is a sample application:
It is ran with:
In a second window we can run simulated traffic generator, a curtesy of curl:
In third window we
pkill(1)
the parent:What is seen in uvicorn logs:
Normal traffic:
SIGTERM
received:At this point only a single worker receives traffic:
Now we closed the last workers' sockets but parent still has opened socket, so we are dropping requests on the floor:
After that point we are throwing connection-refused.
From the client it looks like:
New behaviour
New version:
PS. The same situation can be reproduced w/o shutdown lifecycle event with
--timeout-graceful-shutdown 30
and an endless stream of slow requestsChecklist