Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve shutdown process in Multiprocess mode #2010

Closed
wants to merge 1 commit into from
Closed

Improve shutdown process in Multiprocess mode #2010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions uvicorn/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,6 @@ async def shutdown(self, sockets: Optional[List[socket.socket]] = None) -> None:
# Stop accepting new connections.
for server in self.servers:
server.close()
for sock in sockets or []:
sock.close()
Comment on lines -270 to -271
Copy link
Author

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.

for server in self.servers:
await server.wait_closed()

Expand Down
5 changes: 5 additions & 0 deletions uvicorn/supervisors/multiprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,13 @@ def startup(self) -> None:
self.processes.append(process)

def shutdown(self) -> None:
for sock in self.sockets:
sock.close()

Comment on lines +66 to +68
Copy link
Sponsor Member

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?

Copy link
Author

@rbtz-openai rbtz-openai Jun 18, 2023 •
edited
Loading

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.

Copy link
Author

@rbtz-openai rbtz-openai Jun 19, 2023 •
edited
Loading

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)

for process in self.processes:
process.terminate()

for process in self.processes:
process.join()

message = "Stopping parent process [{}]".format(str(self.pid))
Expand Down