-
-
Notifications
You must be signed in to change notification settings - Fork 733
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
fix: stop server on "lifespan.shutdown.failed" #2309
fix: stop server on "lifespan.shutdown.failed" #2309
Conversation
If the app sends `"lifespan.shutdown.failed"` after having sent `"lifespan.startup.complete"`, but before the server sends `"lifespan.shutdown"` the server continues to serve requests. This PR stops the server when the shutdown failure message is received from the application. This scenario can arise when the server lifespan is used to enter into some application lifespan context which throws an error before the server has been shutdown.
@@ -71,7 +71,6 @@ async def shutdown(self) -> None: | |||
|
|||
if self.shutdown_failed or (self.error_occured and self.config.lifespan == "on"): | |||
self.logger.error("Application shutdown failed. Exiting.") | |||
self.should_exit = True |
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.
I played with this line removal in the shutdown method a little, as it surprised me at first, but it seems to have no effect whatsoever on tests ie having the line or not doesnt change any outcome.
This would advocate for removing entirely the line (provided that this is tested ofc), that said to stay on the safe side would there be a case where we'd still need it ? It's used here and a sys.exit is performed in that case:
Lines 169 to 171 in 0efd383
except OSError as exc: | |
logger.error(exc) | |
await self.lifespan.shutdown() |
wdyt ?
@@ -252,7 +252,7 @@ async def on_tick(self, counter: int) -> bool: | |||
return True | |||
if self.config.limit_max_requests is not None: | |||
return self.server_state.total_requests >= self.config.limit_max_requests | |||
return False | |||
return self.lifespan.should_exit |
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.
return self.lifespan.should_exit | |
# Determine if we should exit. | |
if self.should_exit or self.lifespan.should_exit: | |
return True | |
if self.config.limit_max_requests is not None: | |
return self.server_state.total_requests >= self.config.limit_max_requests | |
return False |
would that suggestion be marginally better ? ie we test for exit in on_tick
on both the server should_exit
flag and the lifespan one instead of delaying it at the end ? either way is ok to me though
I've explained on the issue why this is not an issue. |
Summary
Stop the server if a "lifespan.shutdown.failure" message is received from the application after "lifespan.startup.complete" has been sent by the application, but before the server has sent "lifespan.shutdown".
Applications typically use a context manager around the asgi lifespan protocol to manage resources that should share the same lifecycle as the web server. Where a managed resource encounters an error before the server has initiated shutdown, app frameworks such as starlette and litestar will send a
lifespan.shutdown.failed
message.E.g, in starlette:
This PR gives the application a way to tell the server that something critical has failed internally, and that it should shut down.
Closes #2308
Checklist