Should server shutdown after receiving "lifespan.shutdown.failed"?

[peterschutt]

If the shutdown failure message isn't initiated by the app receiving a "lifespan.shutdown" from uvicorn, then the app continues to run after the "lifespan.shutdown.failure" message is received.

Reproducer:

import asyncio
import contextlib
from collections.abc import AsyncIterator

import anyio
import uvicorn
from starlette.applications import Starlette
from starlette.responses import JSONResponse
from starlette.routing import Route


async def homepage(_) -> JSONResponse:
    return JSONResponse({'hello': 'world'})


async def sleep_and_raise() -> None:
    await asyncio.sleep(1)
    raise RuntimeError("An error occurred")


@contextlib.asynccontextmanager
async def lifespan(_: Starlette) -> AsyncIterator[None]:
    async with contextlib.AsyncExitStack() as stack:
        tg = await stack.enter_async_context(anyio.create_task_group())
        tg.start_soon(sleep_and_raise)
        yield


app = Starlette(debug=True, routes=[Route('/', homepage)], lifespan=lifespan)


if __name__ == "__main__":
    uvicorn.run(app, lifespan="on")

After the error occurs in the lifespan task, the app continues to serve:

Given that apps like starlette and litestar encourage use of the lifespan context for orchestration of things that should have a lifespan equivalent to the application object, then I think it would make sense for the app to stop if something has failed within that lifespan after the app has sent "startup.complete" but before the server has sent "shutdown" to the app.

The spec seems to agree:

If a server sees this it should log/print the message provided and then terminate.

[graingert]

Yes this was my intention with https://github.com/encode/starlette/pull/1227/files#diff-f974056c1286c773853f3083818ffe949dc1ff3724a10bc39025585c6c16c198R628

[peterschutt]

Thanks - actually I came across that PR while researching.

Also, I came across your open issue to support context manager app factories. This was my first instinct after realizing the server doesn't stop in OP.

Short of an approach such as https://github.com/adriangb/lazgi, then there isn't really a nice way to do this type of thing at the moment (I'm calling os.kill(os.getpid(), signal.SIGINT)) in an exception handler in my lifespan context to get the server to stop).

My (maybe naive) first thought is that the server check the lifespan object in on_tick() to see if its had an exception raised, but that wouldn't solve the issue of sharing contextvars across the lifespan and server tasks mentioned in #1151.

[peterschutt]

Just to confirm, should I open an issue and PR for this?

Excluding tests the patch is quite small and doesn't break any tests:

diff --git a/uvicorn/lifespan/on.py b/uvicorn/lifespan/on.py
index 09df984..8eb681b 100644
--- a/uvicorn/lifespan/on.py
+++ b/uvicorn/lifespan/on.py
@@ -71,7 +71,6 @@ class LifespanOn:
 
         if self.shutdown_failed or (self.error_occured and self.config.lifespan == "on"):
             self.logger.error("Application shutdown failed. Exiting.")
-            self.should_exit = True
         else:
             self.logger.info("Application shutdown complete.")
 
@@ -130,6 +129,7 @@ class LifespanOn:
             assert not self.shutdown_event.is_set(), STATE_TRANSITION_ERROR
             self.shutdown_event.set()
             self.shutdown_failed = True
+            self.should_exit = True
             if message.get("message"):
                 self.logger.error(message["message"])
 
diff --git a/uvicorn/server.py b/uvicorn/server.py
index bfce1b1..155dd20 100644
--- a/uvicorn/server.py
+++ b/uvicorn/server.py
@@ -252,7 +252,7 @@ class Server:
             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
 
     async def shutdown(self, sockets: list[socket.socket] | None = None) -> None:
         logger.info("Shutting down")