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

ensure TestClient requests run in the same EventLoop as lifespan #1213

Conversation

graingert
Copy link
Member

@graingert graingert commented Jun 23, 2021

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.

@graingert graingert requested a review from uSpike June 23, 2021 22:17
@graingert graingert force-pushed the ensure-testclient-requests-run-in-same-eventloop-as-lifespan branch 4 times, most recently from 3d1c00d to 061f669 Compare June 23, 2021 23:42
@graingert graingert force-pushed the ensure-testclient-requests-run-in-same-eventloop-as-lifespan branch from 43da897 to 1a56a1f Compare June 28, 2021 20:38
starlette/testclient.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
@graingert graingert requested a review from JayH5 June 28, 2021 20:41
tests/test_testclient.py Outdated Show resolved Hide resolved
tests/test_testclient.py Outdated Show resolved Hide resolved
Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

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

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

starlette/testclient.py Outdated Show resolved Hide resolved
@@ -106,14 +112,14 @@ class _ASGIAdapter(requests.adapters.HTTPAdapter):
def __init__(
self,
app: ASGI3App,
async_backend: _AsyncBackend,
portal_factory: _PortalFactoryType,
Copy link
Member

Choose a reason for hiding this comment

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

So now _AsyncBackend is only used in one place 😅

tests/test_testclient.py Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
Copy link
Member

@uSpike uSpike left a comment

Choose a reason for hiding this comment

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

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?

starlette/testclient.py Show resolved Hide resolved
starlette/testclient.py Show resolved Hide resolved
tests/test_testclient.py Outdated Show resolved Hide resolved
tests/test_testclient.py Outdated Show resolved Hide resolved
@graingert graingert requested review from uSpike and JayH5 June 30, 2021 13:45
trio should complain if used incorrectly here.
@uSpike
Copy link
Member

uSpike commented Jun 30, 2021

Is the guarantee of having the same task run lifespan startup/shutdown only (currently) valid for the TestClient? What I'm concerned about is that someone may write tests for their app which use a TaskGroup in the lifespan, and that works fine in the tests, but then would fail if ran in production via uvicorn for example.

@graingert
Copy link
Member Author

graingert commented Jun 30, 2021

@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
Copy link
Member

uSpike commented Jun 30, 2021

Right, but you're also checking that the same task is used for lifespan events, and using a task group to check that as well. I don't think that will currently work outside of these tests, am I wrong? Edit: i'm wrong

@graingert
Copy link
Member Author

@uSpike lifespan is specified in asgi as always being a single task. In fact it's stricter - Callable[[Scope, Rx, Tx], Awaitable[None]]

@uSpike
Copy link
Member

uSpike commented Jun 30, 2021

OK as discussed on gitter I was wrong about lifespan events being triggered in separate tasks.

Copy link
Member

@uSpike uSpike left a comment

Choose a reason for hiding this comment

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

I'm ok with this but I'd still wait for @JayH5 before merging

Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

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

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.

starlette/testclient.py Outdated Show resolved Hide resolved
tests/test_testclient.py Show resolved Hide resolved
tests/test_testclient.py Outdated Show resolved Hide resolved
tests/test_testclient.py Outdated Show resolved Hide resolved
starlette/testclient.py Outdated Show resolved Hide resolved
@graingert graingert requested a review from JayH5 July 2, 2021 08:47
Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

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

Nice! This is cool. Thanks for the updated PR description.

@graingert graingert merged commit 254d0d9 into encode:master Jul 3, 2021
@graingert graingert deleted the ensure-testclient-requests-run-in-same-eventloop-as-lifespan branch July 3, 2021 16:39
@graingert graingert mentioned this pull request Jul 4, 2021
11 tasks
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