fix: stop server on "lifespan.shutdown.failed" #2309

peterschutt · 2024-04-17T01:49:40Z

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:

    async def lifespan(self, scope: Scope, receive: Receive, send: Send) -> None:
        started = False
        app: typing.Any = scope.get("app")
        await receive()
        try:
            async with self.lifespan_context(app) as maybe_state:
                ...
        except BaseException:
            exc_text = traceback.format_exc()
            if started:
                await send({"type": "lifespan.shutdown.failed", "message": exc_text})
            else:
                await send({"type": "lifespan.startup.failed", "message": exc_text})
            raise
        else:
            await send({"type": "lifespan.shutdown.complete"})

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

I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
I've updated the documentation accordingly.

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.

euri10 · 2024-04-17T07:07:20Z

uvicorn/lifespan/on.py

@@ -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


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:

uvicorn/uvicorn/server.py

Lines 169 to 171 in 0efd383

except OSError as exc:

logger.error(exc)

await self.lifespan.shutdown()

wdyt ?

euri10 · 2024-04-17T07:08:44Z

uvicorn/server.py

@@ -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


Suggested change

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

euri10 reviewed Apr 17, 2024

View reviewed changes

Kludex added the hold Don't merge yet label Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: stop server on "lifespan.shutdown.failed" #2309

fix: stop server on "lifespan.shutdown.failed" #2309

peterschutt commented Apr 17, 2024 •

edited

euri10 Apr 17, 2024

euri10 Apr 17, 2024

	except OSError as exc:
	logger.error(exc)
	await self.lifespan.shutdown()

fix: stop server on "lifespan.shutdown.failed" #2309

Are you sure you want to change the base?

fix: stop server on "lifespan.shutdown.failed" #2309

Conversation

peterschutt commented Apr 17, 2024 • edited

Summary

Checklist

euri10 Apr 17, 2024

Choose a reason for hiding this comment

euri10 Apr 17, 2024

Choose a reason for hiding this comment

peterschutt commented Apr 17, 2024 •

edited