fix: support persistent lifespans in asgi - #231
Conversation
| async def lifespan_state(): | ||
| nonlocal lifespan | ||
| if lifespan is None: | ||
| # Assign before awaiting so concurrent first requests share startup. | ||
| lifespan = create_task(start_application(app)) | ||
| _, state = await lifespan | ||
| return state |
There was a problem hiding this comment.
A cancelled first request propagates cancellation into this shared task, leaving lifespan permanently cancelled and causing every later request in the isolate to be cancelled. Shield the shared startup task from an individual request cancellation.
| async def lifespan_state(): | |
| nonlocal lifespan | |
| if lifespan is None: | |
| # Assign before awaiting so concurrent first requests share startup. | |
| lifespan = create_task(start_application(app)) | |
| _, state = await lifespan | |
| return state | |
| async def lifespan_state(): | |
| from asyncio import shield | |
| nonlocal lifespan | |
| if lifespan is None: | |
| # Assign before awaiting so concurrent first requests share startup. | |
| lifespan = create_task(start_application(app)) | |
| _, state = await shield(lifespan) | |
| return state |
|
I'm Bonk, and I've done a quick review of your PR. Makes ASGI entrypoint lifespan state persist across requests.
|
7fdc066 to
864cb45
Compare
864cb45 to
5b8cb5e
Compare
| ctx: Context | None = None, | ||
| state: dict[str, Any] | None = None, | ||
| ) -> js.Response: | ||
| if (req.headers.get("upgrade") or "").lower() == "websocket": |
There was a problem hiding this comment.
| if (req.headers.get("upgrade") or "").lower() == "websocket": | |
| if req.headers.get("upgrade", "").lower() == "websocket": |
| entrypoint_type = type(self) | ||
| start_future = entrypoint_type.__dict__.get("_start_future") |
There was a problem hiding this comment.
This seems weird to me, shouldn't it be an instance attribute not a class attribute? Also, why not just:
| entrypoint_type = type(self) | |
| start_future = entrypoint_type.__dict__.get("_start_future") | |
| start_future = self._start_future |
hoodmane
left a comment
There was a problem hiding this comment.
Generally looks reasonable, though I'd set _start_future on the AsgiWorkerEntrypoint instance rather than the class and initialize _start_future to None in AsgiWorkerEntrypoint.__init__(). Could adjust it in a followup if you like though.
Ensures that mutable lifespans are persisted across requests. Adds fastapi and asgi-specific tests.