Skip to content
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

Merged
merged 23 commits into from Mar 5, 2023
Merged

Introduce lifespan state #1818

merged 23 commits into from Mar 5, 2023

Conversation

adriangb
Copy link
Member

No description provided.

uvicorn/config.py Outdated Show resolved Hide resolved
@adriangb
Copy link
Member Author

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 test_main.py?

@adriangb adriangb requested a review from Kludex December 27, 2022 17:11
@Kludex
Copy link
Sponsor Member

Kludex commented Dec 27, 2022

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 test_main.py?

Hmm... I think we can test it on test_http.py with an application that stores something on the state.

Copy link
Sponsor Member

@Kludex Kludex left a 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?

@adriangb
Copy link
Member Author

I like having Lifespan be the thing creating it since it is “owned” by the lifespan. We could pass in lifespan_state: Dict[str, Any] instead of the Lifespan object itself. I don’t think it’s all that bad for the request/response lifecycle to have a reference to it’s corresponding lifespan.

@adriangb
Copy link
Member Author

Added support for websockets and tests for http, websockets and the lifespan itself.

@adriangb
Copy link
Member Author

I think the main questions are:

  • Who creates the lifespan state dict? Is it the "server" or the "lifespan"? There's (currently at least) a 1:1 relationship between server and lifespan, so I guess it could be in the server, but I think it fits better in the lifespan. Putting it in the lifespan would also allow a server to be started multiple times (even concurrently) which is I think a sign of the correct design (at least in theory, there's other stateful stuff in the server instances I think).
  • What do we pass to the protocols? It could be the lifespan, the server state or just a dict. I implemented this with the lifespan, but I think just the dict would be reasonable as well.
  • Do we need to include default values in the protos? They're technically public classes and adding a new required param to the constructor is a breaking change. But I don't think anyone uses Uvicorn by importing just a proto class, so I expect the close to 0% of users impacted aren't worth the extra line of code / logic branch to handle default values.

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 28, 2022

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 run_server is not caught by pytest. You can store the values you want to assert from a nonlocal, and check at the end. If you search for "nonlocal" in the test suite, you'll find what I mean.


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

@adriangb
Copy link
Member Author

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 app_state which I think it's appropriate: it's the state that the ASGI server is managing for the ASGI app.

Working on fixing the assertions, thank you for pointing that out.

@adriangb
Copy link
Member Author

adriangb commented Mar 4, 2023

We need to move this out of the extensions namespace into the top level namespace

tests/protocols/test_http.py Outdated Show resolved Hide resolved
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
@Kludex Kludex merged commit 2a94a96 into encode:master Mar 5, 2023
@NargiT
Copy link

NargiT commented Mar 21, 2023

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 ?

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 21, 2023

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants