Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion asgiref/typing.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,26 @@
import sys
from typing import Awaitable, Callable, Dict, Iterable, Optional, Tuple, Type, Union
from typing import (
Any,
Awaitable,
Callable,
Dict,
Iterable,
Optional,
Tuple,
Type,
Union,
)

if sys.version_info >= (3, 8):
from typing import Literal, Protocol, TypedDict
else:
from typing_extensions import Literal, Protocol, TypedDict

if sys.version_info >= (3, 11):
from typing import NotRequired
else:
from typing_extensions import NotRequired
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


__all__ = (
"ASGIVersions",
"HTTPScope",
Expand Down Expand Up @@ -62,6 +77,7 @@ class HTTPScope(TypedDict):
headers: Iterable[Tuple[bytes, bytes]]
client: Optional[Tuple[str, int]]
server: Optional[Tuple[str, Optional[int]]]
state: NotRequired[Dict[str, Any]]
extensions: Optional[Dict[str, Dict[object, object]]]


Expand All @@ -78,12 +94,14 @@ class WebSocketScope(TypedDict):
client: Optional[Tuple[str, int]]
server: Optional[Tuple[str, Optional[int]]]
subprotocols: Iterable[str]
state: NotRequired[Dict[str, Any]]
extensions: Optional[Dict[str, Dict[object, object]]]


class LifespanScope(TypedDict):
type: Literal["lifespan"]
asgi: ASGIVersions
state: NotRequired[Dict[str, Any]]
Copy link
Copy Markdown
Contributor Author

@adriangb adriangb Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - I also missed that!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



WWWScope = Union[HTTPScope, WebSocketScope]
Expand Down
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ python_requires = >=3.7
packages = find:
include_package_data = true
install_requires =
typing_extensions; python_version < "3.8"
typing_extensions; python_version < "3.11"
zip_safe = false

[options.extras_require]
Expand Down
35 changes: 35 additions & 0 deletions specs/lifespan.rst
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ The scope information passed in ``scope`` contains basic metadata:
* ``asgi["version"]`` (*Unicode string*) -- The version of the ASGI spec.
* ``asgi["spec_version"]`` (*Unicode string*) -- The version of this spec being
used. Optional; if missing defaults to ``"1.0"``.
* ``state`` Optional(*dict[Unicode string, Any]*) -- An empty namespace where
the application can persist state to be used when handling subsequent requests.
Optional; if missing the server does not support this feature.

If an exception is raised when calling the application callable with a
``lifespan.startup`` message or a ``scope`` with type ``lifespan``,
Expand All @@ -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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think so

A *shallow copy* of this dictionary will get passed into each request handler.
This key will only be set if the server supports this extension.


Lifespan State
--------------

Applications often want to persist data from the lifespan cycle to request/response handling.
For example, a database connection can be established in the lifespan cycle and persisted to
the request/response cycle.
The ```lifespan["state"]`` namespace provides a place to store these sorts of things.
The server will ensure that a *shallow copy* of the namespace is passed into each subsequent
request/response call into the application.
Since the server manages the application lifespan and often the event loop as well this
ensures that the application is always accessing the database connection (or other stored object)
that corresponds to the right event loop and lifecycle, without using context variables,
global mutable state or having to worry about references to stale/closed connections.

ASGI servers that implement this feature will provide
``state`` as part of the ``lifespan`` scope::

"scope": {
...
"state": {},
}

The namespace is controlled completely by the ASGI application, the server will not
interact with it other than to copy it.
Nonetheless applications should be cooperative by properly naming their keys such that they
will not collide with other frameworks or middleware.

Startup - ``receive`` event
'''''''''''''''''''''''''''
Expand Down
4 changes: 4 additions & 0 deletions specs/www.rst
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ The *connection scope* information passed in ``scope`` contains:
listening port, or ``[path, None]`` where ``path`` is that of the
unix socket. Optional; if missing defaults to ``None``.

* ``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.

Comment on lines +124 to +127
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing in the WebSocket section below.

Servers are responsible for handling inbound and outbound chunked transfer
encodings. A request with a ``chunked`` encoded body should be automatically
de-chunked by the server and presented to the application as plain body bytes;
Expand Down