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

Run lifespans of mounted applications #1988

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

daanvdk
Copy link

@daanvdk daanvdk commented Dec 23, 2022

Runs lifespans of mounted apps.

Fixes issue #649.

Comment on lines +1040 to +1070
async def lifespan(scope: Scope, receive: Receive, send: Send):
assert scope["type"] == "lifespan"
assert scope["app"] == lifespan

message = await receive()
assert message["type"] == "lifespan.startup"
try:
startup()
except Exception:
await send(
{
"type": "lifespan.startup.failed",
"message": format_exc(),
}
)
return
await send({"type": "lifespan.startup.complete"})

message = await receive()
assert message["type"] == "lifespan.shutdown"
try:
shutdown()
except Exception:
await send(
{
"type": "lifespan.shutdown.failed",
"message": format_exc(),
}
)
return
await send({"type": "lifespan.shutdown.complete"})
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious what the rational was for implementing this here as opposed to just mounting a Starlette app within a Starlette app (app = Starlette(routes=[Mount(app=Starlette(lifespan=...))], lifespan=...))

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I mainly, wanted to have a lifespan where I could easily overwrite the result of what happens during startup/shutdown. Might indeed be possible to simplify with a starlette app indeed.

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Great work! I gave this an initial look, left some comments. Haven't reviewed the tests in detail but I appreciate how thorough the test cases are. If we were to move forward with this I plan to do an in-depth review of the test cases and would like several maintainers to do the same. I think this sort of code with lots of events and tasks is prone to all sorts of bugs from race conditions, cancellation, etc.

High level I think the main question (which I'd like input from other maintainers on) is if we want to adopt this complexity (in other words, is the feature worth the complexity). I know this is something that has been requested in the past, but I don't think it's been a huge issue.

I also wonder if it might be better to just take control of the event loop and run Uvicorn programmatically instead of getting into such complex lifespans and mounted apps (that is, instead of lifespans you just do whatever setup you need and then instantiate your apps).

Comment on lines 637 to 638
# This wrapper is needed because TaskGroup.start_soon does not like that
# App returns Awaitable instead of Coroutine
Copy link
Member

Choose a reason for hiding this comment

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

Yup that is the case. If we use this somewhere else in the library (I don't recall) it might be worth doing something more generic since the issue relates to awaitables/coroutines and not specifically ASGI apps.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah makes sense, for now this wrapper turned out to be a good thing because I added some code at the end to check if the app did not return in a situation where that would block the context manager.

Comment on lines +643 to +644
async with create_task_group() as tg:
tg.start_soon(coro_app, {**scope, "app": app}, receive, send)
Copy link
Member

Choose a reason for hiding this comment

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

After all of the issues caused by using tasks in BaseHTTPMiddelware I'd like to avoid using them if possible. And I think that in this case it is possible: https://github.com/adriangb/asgi-routing/blob/main/asgi_routing/_lifespan_dispatcher.py#L11-L101. It's not really more LOC either. But maybe there's a bug in there that I didn't catch.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I think the main thing is that you need a task to be able to wrap a lifespan in an async context manager, which does feel like a nice abstraction to have. But I definitely understand your reservations.

@Kludex
Copy link
Member

Kludex commented Feb 4, 2023

@adriangb if it's fine by you, I'd prefer for us to avoid this before 1.0.

@Kludex Kludex added this to the Version 1.x milestone Feb 4, 2023
@adriangb
Copy link
Member

adriangb commented Feb 4, 2023

I agree on that

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.

3 participants