-
-
Notifications
You must be signed in to change notification settings - Fork 756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce lifespan state #1818
Introduce lifespan state #1818
Conversation
Needs more tests but we don't currently have any tests that test both lifespans and requests. I suppose this should be some sort of end to end test in |
Hmm... I think we can test it on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I told you, I think this is a great idea. 👍
I'm just not sure about the new lifespan
parameter on the HTTP protocol implementations. Maybe your previous idea about adding it to the ServerState
may be better (just thinking here, do not change it yet).
Also, what about the WebSockets? The state
is not included on the WebSocket scope?
I like having Lifespan be the thing creating it since it is “owned” by the lifespan. We could pass in |
Added support for websockets and tests for http, websockets and the lifespan itself. |
I think the main questions are:
|
The creation of the dict should be on the lifespan class, but I don't agree with the lifespan parameter on the protocol classes. I think it can be passed through the ServerState. The way I see it, the protocol classes should avoid as much as possible ASGI related concepts. After the startup runs the lifespan, we can pass the state to the server state object. Can't we? On the test suite, you made assertions inside the app. That doesn't work, as every exception inside the This is probably going to be an extension, so we need to provide the "lifespan.state" on the extensions on each scope on the protocol classes. And if is not, then we'll probably need to bump the spec - which I doubt this will happen. 👀 |
I moved to passing in the already copied dict to the protocols. This actually fixed a bug (or at least non-intentional behavior) where the state would be double copied for websockets. So now protocols have no references to lifespans. I used the name Working on fixing the assertions, thank you for pointing that out. |
We need to move this out of the extensions namespace into the top level namespace |
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
hello, I am here because of renovate bot. I try to understand what this lifespan state is for and didn't find anything relevant in uvicorn doc. maybe you could link the discussion/issue this refers to ? |
You can read more about it here: |
No description provided.