Support lifespan state #2060

Kludex · 2023-03-04T15:43:00Z

This PR implements the Lifespan state mentioned on the ASGI spec: Add extensions["lifespan.state"] django/asgiref#354.
This PR complements Introduce lifespan state uvicorn#1818.

Changes

Implement lifespan state.
Remove generator function way to use lifespan.

How to review

You need to have both this branch and the one from encode/uvicorn#1818 on your virtual environment, and then you can use this application to test the behavior:

from contextlib import asynccontextmanager
from typing import Any, Dict

from starlette.applications import Starlette
from starlette.responses import PlainTextResponse
from starlette.routing import Route


# async def startup(state: Dict[str, Any]):
#     print(state)
#     state["potato"] = True
#     print(state)
#     print("startup")


# async def stateless_startup():
#     print("stateless_startup")


@asynccontextmanager
async def state_manager(app, state):
    state["haha"] = True
    print(state)
    yield
    print(state)

@asynccontextmanager
async def stateless_manager(app):
    print("stateless manager")
    yield

async def homepage(request):
    print(request.state)
    return PlainTextResponse("Hello, world!")


app = Starlette(
    routes=[Route("/", homepage)],
    # on_startup=[startup, stateless_startup],
    lifespan=state_manager,
    # lifespan=stateless_manager,
)

Play with the comments above. 👀

Kludex · 2023-03-04T15:45:45Z

starlette/routing.py

        """
        Run any `.on_startup` event handlers.
        """
        for handler in self.on_startup:
+            sig = inspect.signature(handler)
+            if len(sig.parameters) == 1 and state is not None:


If the state is None, it means that the server doesn't support it, so we'll maintain the same error message.

starlette/routing.py

adriangb

Overall looks good! I have some questions about deprecations, unless it makes the PR a lot more complicated I would suggest doing the deprecations in another PR. You could always support this new feature only for async context manager dependencies to avoid having to implement things for all of the other types of lifespans / startup/shutdown events we support. But not sure how that works out code wise.

adriangb · 2023-03-04T15:51:52Z

starlette/routing.py

+    def __call__(
+        self: _TDefaultLifespan,
+        app: object,
+        state: typing.Optional[typing.Dict[str, typing.Any]],
+    ) -> _TDefaultLifespan:
+        self._state = state
        return self


At first glance I'm a bit confused about this and the need for a TypeVar.

The reason for the TypeVar is that it needed to be bound, as the _state was introduced here, since the self is annotated.

starlette/routing.py

adriangb · 2023-03-04T15:53:50Z

starlette/testclient.py

@@ -243,6 +245,7 @@ def handle_request(self, request: httpx.Request) -> httpx.Response:
                "client": ["testclient", 50000],
                "server": [host, port],
                "subprotocols": subprotocols,
+                "state": self.state,


I'm not seeing where this dict is copied. Is it being copied before being passed into each request?

starlette/types.py

adriangb · 2023-03-04T15:55:43Z

tests/test_routing.py

+    async def run_startup(state):
+        nonlocal startup_complete
+        startup_complete = True
+        state["startup"] = True
+
+    async def run_shutdown(state):
+        nonlocal shutdown_complete
+        shutdown_complete = True
+        assert state["startup"]


Do we want to support state in on_startup and on_shutdown? Don't we want to deprecate that in 1.0? If we were not planning on it, we should be. I think we should only support async context manager lifespans in 1.0.

Do we want to support state in on_startup and on_shutdown?

I don't think we discussed it.

What are the problems we have there besides "multiple ways to do things"?

I'd say just that: multiple ways to do things.

I really like the on_startup and on_shutdown parameters, and given that it would be one more thing to do for 1.0, I'm not in favor of doing it.

Okay fair enough, it's besides the point of this PR. Not sure why you like them but I'm fairly certain the team decided to promote context manager lifespans as the way forward, so can we at least write the tests using that?

Do you know where it was discussed?

Not specifically, just my recollection. Maybe @graingert does, I know they advocated heavilty for the async context manager approach.

tests/test_routing.py

Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

adriangb

#2061

#2061)

Kludex · 2023-03-05T13:42:43Z

starlette/routing.py

+        server_supports_state = state is not None
+        if lifespan_needs_state and not server_supports_state:
+            raise RuntimeError(
+                'The server does not support "state" in the lifespan scope.'


I removed the "please change server", and use the created the state variable to save one line below... 😬

adriangb · 2023-03-05T17:38:00Z

Before we release this, just wondering if this API might be better:

Lifespan = Callable[[Starlette], AsyncContextManager[None | Dict[str, Any]]]

class MyStateType(TypedDict):
    client: httpx.AsyncClient

@asynccontextmanager
async def lifespan(app: Starlette) -> MyStateType:
    async with httpx.AsyncClient() as client:
        yield {"client": client}

I think it would also be even less changes in Starlette since we could do:

async with lifespan as maybe_state:
    if maybe_state:
        scope["state"].update(maybe_state)

And not have to introspect the function signature or anything.

Kludex · 2023-03-05T17:43:11Z

We'd still have to introspect in the startup and shutdown, no?

adriangb · 2023-03-05T17:44:31Z

Not sure how to handle shutdown. startup would be easy: just let it return None | Dict[str, Any]. But I'd also be okay only supporting this for context manager lifespans.

Kludex · 2023-03-05T20:31:21Z

My question was more about what you said:

I think it would also be even less changes in Starlette since we could do:

And not have to introspect the function signature or anything.

Considering that, we'd still need to introspect for shutdown, at least.

That said... I think what you propose looks better for lifespan, but I'm not sure if it's worth it... Do you want to create a PR, and we discuss there?

This reverts commit da6461b.

* Revert "Support lifespan state (#2060)" This reverts commit da6461b. * new implementation * Deprecate `on_startup` and `on_shutdown` events * Rename `events.md` by `lifespan.md` --------- Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

empicano · 2023-07-18T14:56:58Z

I discovered this yesterday after upgrading from an older version and just wanted to say that I'm delighted by it. It's so much nicer than using globals! Thanks for the work that went into this 🔥

Support lifespan state

ba1588a

Kludex force-pushed the state-map branch from 0781966 to ba1588a Compare March 4, 2023 15:43

Kludex requested a review from adriangb March 4, 2023 15:44

Kludex commented Mar 4, 2023

View reviewed changes

starlette/routing.py Show resolved Hide resolved

adriangb approved these changes Mar 4, 2023

View reviewed changes

Copy the state instead of reusing on multiple requests

3e7b804

Kludex commented Mar 4, 2023

View reviewed changes

tests/test_routing.py Show resolved Hide resolved

adriangb reviewed Mar 4, 2023

View reviewed changes

tests/test_routing.py Outdated Show resolved Hide resolved

adriangb reviewed Mar 4, 2023

View reviewed changes

tests/test_routing.py Outdated Show resolved Hide resolved

Kludex and others added 2 commits March 4, 2023 20:00

Readd deprecated conditionals

35ecd94

Apply suggestions from code review

246a61f

Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

Kludex force-pushed the state-map branch from 9fb398d to 246a61f Compare March 4, 2023 19:07

adriangb approved these changes Mar 4, 2023

View reviewed changes

Add test for asynccontextmanager

fa07156

Kludex requested a review from adriangb March 4, 2023 19:23

adriangb approved these changes Mar 4, 2023

View reviewed changes

adriangb requested changes Mar 4, 2023

View reviewed changes

Add test for server not supporting lifespan state but app requiring it (

c791251

#2061)

Kludex commented Mar 5, 2023

View reviewed changes

Cutefy Adrian's improvements

b5aac4d

Kludex force-pushed the state-map branch from f311c96 to b5aac4d Compare March 5, 2023 13:45

Kludex requested a review from adriangb March 5, 2023 13:50

adriangb approved these changes Mar 5, 2023

View reviewed changes

Kludex merged commit da6461b into master Mar 5, 2023

Kludex deleted the state-map branch March 5, 2023 14:49

Kludex mentioned this pull request Mar 5, 2023

Document Lifespan state #2064

Merged

adriangb added a commit to adriangb/starlette that referenced this pull request Mar 5, 2023

Revert "Support lifespan state (encode#2060)"

44159d0

This reverts commit da6461b.

sm-Fifteen mentioned this pull request Mar 7, 2023

Further develop startup and shutdown events fastapi/fastapi#617

Open

Kludex pushed a commit that referenced this pull request Mar 9, 2023

Revert "Support lifespan state (#2060)"

2d34ccc

This reverts commit da6461b.

synodriver mentioned this pull request May 25, 2023

ASGI Extension support emmett-framework/granian#78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support lifespan state #2060

Support lifespan state #2060

Kludex commented Mar 4, 2023

Kludex Mar 4, 2023

adriangb left a comment

adriangb Mar 4, 2023

Kludex Mar 4, 2023 •

edited

Loading

adriangb Mar 4, 2023

Kludex Mar 4, 2023

adriangb Mar 4, 2023

Kludex Mar 4, 2023

adriangb Mar 4, 2023

Kludex Mar 4, 2023

adriangb Mar 4, 2023

Kludex Mar 4, 2023

adriangb Mar 4, 2023

adriangb left a comment

Kludex Mar 5, 2023

adriangb commented Mar 5, 2023

Kludex commented Mar 5, 2023

adriangb commented Mar 5, 2023

Kludex commented Mar 5, 2023

empicano commented Jul 18, 2023

Support lifespan state #2060

Support lifespan state #2060

Conversation

Kludex commented Mar 4, 2023

Changes

How to review

Choose a reason for hiding this comment

adriangb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Mar 5, 2023

Kludex commented Mar 5, 2023

adriangb commented Mar 5, 2023

Kludex commented Mar 5, 2023

empicano commented Jul 18, 2023

Kludex Mar 4, 2023 •

edited

Loading