ensure TestClient requests run in the same EventLoop as lifespan #1213
Conversation
3d1c00d to
061f669
Compare
43da897 to
1a56a1f
Compare
JayH5
left a comment
There was a problem hiding this comment.
This gets into some pretty low-level AnyIO stuff that I'm going to have to read up on.. I'll try get back to this tomorrow. 😴
| self, | ||
| app: ASGI3App, | ||
| async_backend: _AsyncBackend, | ||
| portal_factory: _PortalFactoryType, |
There was a problem hiding this comment.
So now _AsyncBackend is only used in one place 😅
uSpike
left a comment
There was a problem hiding this comment.
Looks good to me, I had a few suggestions. I have some confusion over the mixed use of the terms "thread" and "task". Could you please clarify?
trio should complain if used incorrectly here.
|
|
|
@uSpike uvicorn has to work this way because it only uses one eventloop per process. TestClient was broken because it uses multiple eventloops per process, which I think isn't technically valid under the lifespan asgi prototocol. |
|
|
|
@uSpike lifespan is specified in asgi as always being a single task. In fact it's stricter - |
|
OK as discussed on gitter I was wrong about lifespan events being triggered in separate tasks. |
JayH5
left a comment
There was a problem hiding this comment.
Overall LGTM. I would be grateful for a bit more context in the PR description, though. I understand that you & @uSpike have had conversations about this on gitter, but it'd be good to have a clearer explanation here like: "before things were like this and this was a problem because reasons, so after this change things are like that"...if you get what I mean.
It's difficult for someone who's not following the various threads to read this PR and understand what the actual improvement is.
JayH5
left a comment
There was a problem hiding this comment.
Nice! This is cool. Thanks for the updated PR description.
Ensure TestClient requests run in the same EventLoop as lifespan. The way a TestClient context interacts with app lifespan and route task is important to support app-wide task groups.
Before, a new thread was started for every interaction with the TestClient, most critically lifespan was run in a different event loop than any subsequent request. Not only does this violate the ASGI spec, it also means that resources (locks, databases and task groups) created in lifespan cannot be used in subsequent requests.
After this change each use of the TestClient synchronous context manager creates and caches a BlockingPortal and any further interactions in this context use that cached blocking portal. Interactions outside of this context continue to acquire a new blocking portal for each interaction.