Add extensions["lifespan.state"] #354

adriangb · 2022-11-11T04:48:43Z

Closes #322

specs/lifespan.rst

specs/www.rst

Kludex · 2022-12-27T09:01:59Z

The types module needs changes as well, jfyk.

Kludex · 2022-12-27T17:06:14Z

While looking at this PR I thought: "Why not introducing a new field on lifespan.startup.complete?" I had this thought because it would be the first extension that would change the scope on the ASGI application side.

The thing is, if we do that, the scope on the shutdown event needs to have the state field anyway, because we want to have those resources available there. In case the application sends the state on the lifespan.startup.complete, the scope on the lifespan.startup, and on the lifespan.shutdown will have to differ on this field.

I'm going to ping some people that might be interested in this. 👋 @pgjones @tomchristie

Kludex

There were some rebases here, so we lost a bit the track of the previous comments. I'm going to give a bit of context.

The first implementation here was adding the "Lifespan State" extension to the "Extensions" section under the extensions["lifespan.state"], but it was also adding the extensions["state"] on the lifespan scope. But there's no precedent about using the "extensions" key outside the "Extensions" section. So... The only thing missing on that first implementation was to change the extensions["state"] to state - ofc being optional.

The current implementation, I believe is a breaking change, based on previous comments from Andrew. Since it adds a subsection under the "Lifespan" section.

Anyway, just expressing here what we talked outside GitHub.

Kludex · 2022-12-28T09:38:09Z

asgiref/typing.py

+if sys.version_info >= (3, 11):
+    from typing import NotRequired
+else:
+    from typing_extensions import NotRequired


I don't think the NotRequired should be included only here. This was previously mentioned on the HTTP trailers PR, and on the PR that introduced this module (I'm on my phone, difficult to provide links).

IMO either we change what we consider to be optional on those type hints, or we don't add the NotRequired.

The thing is that making it Optional implies that the server must set it to None, so every single server is immediately out of spec. Using NotRequired means that no changes are necessary to servers.

adriangb · 2022-12-28T10:19:08Z

Thank you for all of the clarifying comments Marcelo.

There's no precedent about using the "extensions" key outside the "Extensions" section.

I don't think this is a particularly salient reason to make it an extension or not. My original reasoning for making it an extension was to avoid conflict with Starlette's use of scope["state"]. I changed it because I realized that putting a dict in scope["state"] would not be a breaking change for Starlette and in fact would work out really nicely because Starlette would already get some support out of the box.

So... The only thing missing on that first implementation was to change the extensions["state"] to state - ofc being optional.

I didn't really understand this comment.

That said I'm happy to move it to extensions, swap NotRequired for Optional; I don't think any of these change the core proposal so I don't have strong feelings.

Kludex · 2022-12-28T10:37:30Z

I don't think this is a particularly salient reason to make it an extension or not. My original reasoning for making it an extension was to avoid conflict with Starlette's use of scope["state"]. I changed it because I realized that putting a dict in scope["state"] would not be a breaking change for Starlette and in fact would work out really nicely because Starlette would already get some support out of the box.

I don't think we are understanding each other. Let me try to improve my wording:

I do think this should be an extension.
I do think it should be called extensions["lifespan.state"].
I do not think the namespace to store the state should be extensions["state"] - it should be only state.

Complying with the 3rd point doesn't make this proposal a non-extension.

Analogous, let's take the HTTP Trailers previously implemented... The ASGI server needs to provide a extensions["http.response.trailers"] in the scope, and the Response Start needs to provide the trailers field, as mentioned here. The fact that the Response Start now has an optional trailers field doesn't make this proposal a non-extension one.

As for the fact that Starlette is using this field... I think the most correct thing would be to find a namespace that doesn't conflict with a namespace that a known ASGI framework is already using, unless we are trying to document what the ASGI framework is already doing. Because in any case, if we do this, Starlette users are going to receive a different data on that namespace, so it's a breaking change.

Maybe we can find a name that has the same meaning as "state"? - Let's read Andrew's comment before...

👇 Unrelated to this PR:

To avoid this kind of confusion in the future, I do believe that we need to go back to the discussion about a namespace for ASGI frameworks to do what they want, and not just accept "any that is unlikely to have conflicts in the future". 🤷

adriangb · 2022-12-28T10:46:19Z

I don't think we are understanding each other. Let me try to improve my wording:

I do think this should be an extension.

I do think it should be called extensions["lifespan.state"].

I do not think the namespace to store the state should be extensions["state"] - it should be only state.

Complying with the 3rd point doesn't make this proposal a non-extension.

Thanks for clarifying, that makes more sense. That sounds fine to me! Since we've gone back and forth I'll wait for Andrew to chime in though before making any changes. I do want to point out that "lifespan.state" seems a bit redundant since it's inside the lifespan scope, what other state could it be?

As for the fact that Starlette is using this field... I think the most correct thing would be to find a namespace that doesn't conflict with a namespace that a known ASGI framework is already using, unless we are trying to document what the ASGI framework is already doing. Because in any case, if we do this, Starlette users are going to receive a different data on that namespace, so it's a breaking change.

Why would it be a breaking change? User's code will only break if they were doing something like request.state.foo = getattr(request.state, "foo", 123) and also use this new feature to set scope["extensions"]["lifespan.state"]["foo"] = 456. I don't think anyone is doing that because (1) why would they check if the field is set or not that dict was always empty previously and (2) they'd have to be opting into this feature and clobbering their own namespace.

I think re-using the field that is already being used is a good thing, it means Starlette and things that depend on it or imitate it (so most of the ASGI landscape) can start using this feature without making any changes internally. For example, I could add support to https://github.com/adriangb/asgi-lifespan while we figure out the API in Starlette to actually set values in the lifespan and then those values would show up transparently in the request/response cycle's scope["state"] and thus Starlette's Request.state.

Kludex · 2022-12-28T10:55:18Z

Thanks for clarifying, that makes more sense. That sounds fine to me! Since we've gone back and forth I'll wait for Andrew to chime in though before making any changes. I do want to point out that "lifespan.state" seems a bit redundant since it's inside the lifespan scope, what other state could it be?

The extension needs to be provided in the HTTP scope as well, but I think it's fine using the extensions["state"] anyway.

Why would it be a breaking change? User's code will only break if they were doing something like request.state.foo = getattr(request.state, "foo", 123) and also use this new feature to set scope["extensions"]["lifespan.state"]["foo"] = 456. I don't think anyone is doing that because (1) why would they check if the field is set or not that dict was always empty previously and (2) they'd have to be opting into this feature and clobbering their own namespace.

Oh, yeah, actually, the user needs to add the field itself on the state initially. Got it. So yeah, the "state" field is fine. 👍

Kludex · 2023-01-17T09:18:33Z

@andrewgodwin sorry to bother, but is there anything we can do to make your life easier while checking this PR?

andrewgodwin · 2023-01-17T20:10:27Z

Sorry, I've been very busy with other things for the past month or so - I'll hopefully get to this soonish, but it's going to take time for me to re-read the initial ticket here to be in the right headspace.

adriangb · 2023-02-26T17:48:00Z

@andrewgodwin if you are too busy to review that's okay, but would you be able to recommend or be okay with tagging another reviewer? This PR/issue have been waiting for an in-depth review for almost a year now.

andrewgodwin · 2023-03-02T05:47:30Z

We don't exactly have a lot of other people who review things on this repo, sadly. I would more maintainers! But they don't exactly appear out of thin air.

I've just got back from Australia; I'll try to sit down and get to this this week.

andrewgodwin · 2023-03-03T22:08:34Z

Alright, I have re-read it, understand it, and it looks good. Fix the linting error and I'll land it!

adriangb · 2023-03-03T22:18:05Z

asgiref/typing.py

    extensions: Optional[Dict[str, Dict[object, object]]]


 class LifespanScope(TypedDict):
    type: Literal["lifespan"]
    asgi: ASGIVersions
+    state: NotRequired[Dict[str, Any]]


@andrewgodwin gave this a last pass and noticed that this was Optional[Dict[str, Any]] but I think it should be NotRequired[Dict[str, Any]] for the same reasons as https://github.com/django/asgiref/pull/354/files#r1118125962 so I changed it in 579bc3d. Just an FYI since you already reviewed.

I agree - I also missed that!

@adriangb why can't this be Dict[Any, Any] rather than Dict[str, Any]?

Below, the docs state:

Applications can store arbitrary data in this namespace.

Why wouldn't that include arbitrary keys?

applications should be cooperative by properly naming their keys such that they
will not collide with other frameworks or middleware.

The most foul-proof way to avoid collisions would be to not use string keys at all, but objects (similar to Symbol keys in javascript).

Also, that would help make dependency injection simpler in FastAPI/Starlette.
Example use-case:

# (roughly) HTTP_CLIENT: ClientSession = fastapi.Depends(lambda request: request.scope['state'][HTTP_CLIENT]) DB_POOL: Pool = fastapi.Depends(lambda request: request.scope['state'][DB_POOL]) @asynccontextmanager async def app_lifespan(app: FastAPI): async with make_http_client() as client: async with make_database_connection_pool() as db_pool: yield {HTTP_CLIENT: client, DB_POOL: db_pool}

I am already doing this and it works just fine! However, it is annoying that the typing doesn't match up quite right... and I don't see a reason keys should be limited to strings.

Starlette already assumes string keys for its state object, so I kind of copied that here.
It's always possible to do scope["state"]["mykey"] = {object(): 123}.
Finally, I think if FastAPI ends up wanting to use this feature and shows a compelling use case for making the keys Any we can re-evaluate and change the typing.

@adriangb thanks for the response. This isn't something I'd think would be part of the fastapi codebase (although, maybe, who knows). This is more a user-land example of a use-case for non-string keys. Would it be more appropriate for me to raise a similar issue in Starlette or Uvicorn then? As I mentioned, I am already doing the above approach (nothing prevents me). It's just that the typing is a lie, since I'm storing non-string keys. Since Starlette don't have any control over what kinds of keys are returned from the app lifespan function (example above), and similarly, Uvicorn doesn't have any control over what keys its lifespan state dict is updated with, it doesn't make sense to me to pretend they are definitely strings.

One thing in particular that will break is Starlette's State class: https://github.com/encode/starlette/blob/15d9350eed9437bcb0683e2c24cd4cfbf985ed7e/starlette/datastructures.py#L683-L708. Now, I'm not a fan of that class/functionality, but it exists. Of course, if you don't care you can continue doing what you are doing, but then solving the typing issue is also as simple as a type: ignore.
I'm also curious why the suggestion above of something like yield {"dependencies": {...}} or something like that wouldn't work.

All of that said, if you feel strongly please feel free to open an issue / PR so it can be discussed properly.

@adriangb thanks for the feedback, I will raise one! I did look at that class as well prior to implementing things the way that I did, but it doesn't seem like anything about it would break. In particular, __setattr__ and __getattr__ are both already typed with key: typing.Any. If they are called via state.my_property, obviously that usage simply wouldn't be applicable for non-string keys.

Right the typing is just wrong/lazy on Starlette's part there. It really should be key: str and I expect it to be changed in Starlette soonish.

@adriangb as promised, I've raised an issue here: encode/starlette#2182. Thanks again for the feedback.

andrewgodwin · 2023-03-03T22:21:11Z

Thanks for your work (and persistence) on this!

adriangb · 2023-03-03T22:22:22Z

Thank you for reviewing and pushing this across the finish line! I'm excited to explore the new APIs this will unblock.

I think I should have changed the PR title since the commit message is now out of date with the implementation, sorry about that...

Kludex · 2023-03-04T11:35:46Z

specs/www.rst

+* ``state`` Optional(*dict[Unicode string, Any]*) -- A copy of the
+  namespace passed into the lifespan corresponding to this request. (See :doc:`lifespan`).
+  Optional; if missing the server does not support this feature.
+


This is missing in the WebSocket section below.

Kludex · 2023-03-04T11:38:11Z

specs/lifespan.rst

@@ -56,6 +59,38 @@ lifespan protocol. If you want to log an error that occurs during lifespan
 startup and prevent the server from starting, then send back
 ``lifespan.startup.failed`` instead.

+The ``extensions["lifespan.state"]`` dict is an empty namespace.
+Applications can store arbitrary data in this namespace.


This line is unclear... In the next section it's specified to store in lifespan["state"], but here it mentions that applications can use extension["lifespan.state"]... 🤔

Was this supposed to be removed?

Yes I think so

adriangb mentioned this pull request Dec 15, 2022

Persistence of state from lifespan events to requests #322

Closed

Kludex reviewed Dec 27, 2022

View reviewed changes

specs/lifespan.rst Outdated Show resolved Hide resolved

specs/www.rst Outdated Show resolved Hide resolved

Introduce lifespan state

cfdcecd

adriangb force-pushed the lifespan-state branch from 5e93b99 to cfdcecd Compare December 27, 2022 18:03

Kludex reviewed Dec 28, 2022

View reviewed changes

andrewgodwin approved these changes Mar 3, 2023

View reviewed changes

adriangb added 2 commits March 3, 2023 16:13

format file

d5d87bd

make state key in lifespan notrequired

579bc3d

adriangb commented Mar 3, 2023

View reviewed changes

andrewgodwin merged commit ec63b10 into django:main Mar 3, 2023

adriangb deleted the lifespan-state branch March 3, 2023 22:20

Kludex mentioned this pull request Mar 4, 2023

Further develop startup and shutdown events fastapi/fastapi#617

Open

Kludex reviewed Mar 4, 2023

View reviewed changes

Kludex mentioned this pull request Mar 4, 2023

Support lifespan state encode/starlette#2060

Merged

adriangb mentioned this pull request Mar 4, 2023

Clarify ambigious section of lifespan["state"] spec #376

Merged

synodriver mentioned this pull request Mar 10, 2023

implement asgi lifespan state pgjones/hypercorn#107

Closed

Kludex mentioned this pull request Mar 21, 2023

Introduce lifespan state encode/uvicorn#1818

Merged

synodriver mentioned this pull request May 25, 2023

ASGI Extension support emmett-framework/granian#78

Closed

This was referenced Jun 12, 2023

Relax typing of lifespan state from Dict[str, Any] to Dict[Any, Any] encode/starlette#2182

Closed

Relax typing of lifespan state from Dict[str, Any] to Dict[Any, Any] #398

Closed

jakkdl mentioned this pull request Apr 18, 2024

Suggestion: set NotRequired on all fields that aren't required #460

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extensions["lifespan.state"] #354

Add extensions["lifespan.state"] #354

adriangb commented Nov 11, 2022 •

edited

Loading

Kludex commented Dec 27, 2022

Kludex commented Dec 27, 2022

Kludex left a comment

Kludex Dec 28, 2022

adriangb Feb 26, 2023

adriangb commented Dec 28, 2022

Kludex commented Dec 28, 2022

adriangb commented Dec 28, 2022 •

edited

Loading

Kludex commented Dec 28, 2022

Kludex commented Jan 17, 2023 •

edited

Loading

andrewgodwin commented Jan 17, 2023

adriangb commented Feb 26, 2023

andrewgodwin commented Mar 2, 2023 •

edited

Loading

andrewgodwin commented Mar 3, 2023

adriangb Mar 3, 2023 •

edited

Loading

andrewgodwin Mar 3, 2023

HansBrende Jun 12, 2023

adriangb Jun 12, 2023

HansBrende Jun 12, 2023

adriangb Jun 12, 2023

HansBrende Jun 12, 2023

adriangb Jun 12, 2023

HansBrende Jun 12, 2023

andrewgodwin commented Mar 3, 2023

adriangb commented Mar 3, 2023

Kludex Mar 4, 2023

Kludex Mar 4, 2023

adriangb Mar 4, 2023

Add extensions["lifespan.state"] #354

Add extensions["lifespan.state"] #354

Conversation

adriangb commented Nov 11, 2022 • edited Loading

Kludex commented Dec 27, 2022

Kludex commented Dec 27, 2022

Kludex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Dec 28, 2022

Kludex commented Dec 28, 2022

adriangb commented Dec 28, 2022 • edited Loading

Kludex commented Dec 28, 2022

Kludex commented Jan 17, 2023 • edited Loading

andrewgodwin commented Jan 17, 2023

adriangb commented Feb 26, 2023

andrewgodwin commented Mar 2, 2023 • edited Loading

andrewgodwin commented Mar 3, 2023

adriangb Mar 3, 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

andrewgodwin commented Mar 3, 2023

adriangb commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Nov 11, 2022 •

edited

Loading

adriangb commented Dec 28, 2022 •

edited

Loading

Kludex commented Jan 17, 2023 •

edited

Loading

andrewgodwin commented Mar 2, 2023 •

edited

Loading

adriangb Mar 3, 2023 •

edited

Loading