Ability to use httpx.AsyncClient for ASGI testing without persisting cookies or open/closing it

[simonw]

I've started using httpx.AsyncClient internally in Datasette, for two purposes:

• For executing the tests in the Datasette test suite by simulating requests
• To power an internal API mechanism, await datasette.client.get("/db/table.json"), which lets plugin authors make in-process calls to Datasette's external JSON API, without the overhead of an HTTP network request - see https://docs.datasette.io/en/stable/internals.html#datasette-client

In both of these cases I instantiate a new AsyncClient instance for every call.

This has a small but measurable performance impact on my unit tests, since I need to instantiate a brand new client object for every one of several hundred tests.

I experimented with reusing a client object created like this (instead of using the with ... context manager):

client = httpx.AsyncClient(app=datasette_asgi_app)

This had two problems: it persisted cookies, which isn't what I want for my test suite. It also produced warnings about unclosed clients thanks to this code:

httpx/httpx/_client.py

Lines 1779 to 1785 in ca5f524

	def __del__(self) -> None:
	if self._state == ClientState.OPENED:
	warnings.warn(
	f"Unclosed {self!r}. "
	"See https://www.python-httpx.org/async/#opening-and-closing-clients "
	"for details."
	)

As far as I can tell the need to open and close a client relates to the use of proxies and persistent HTTP connections. If I'm using the client to exercise an ASGI app directly I don't believe the open/closing mechanism is necessary at all.

So, my feature request: I'd love to be able to reuse a single AsyncClient instance in a way that doesn't persist cookies between requests and doesn't produce warnings about the client being unclosed.

[simonw]

One suggestion for how this might be implemented: the AsyncClient() constructor could take an optional parameter persist_cookies=False, and the warning code in the __del__(self) method could check to see if the client was running against an app= ASGI app and skip the warning if that is the case.

[florimondmanca]

Hello!

Those are interesting points. I can see two topics here: 1/ ability to turn off cookie persistance, and 2/ refining the "unclosed client warning" so it is skipped under certain conditions.

For 1/ — I don't think that's something we discussed or came across yet, mainly because there hasn't been a push or need for it yet. I don't think there's any workaround to a persist_cookie: bool flag if one wants to reuse a client without persisting cookies. Right now it's a bit non-obvious how we'd support it, because it's somewhat hard-coded at the model layer in Response and Cookies:

httpx/httpx/_models.py

Lines 1324 to 1332 in 7fda99f

	def extract_cookies(self, response: Response) -> None:
	"""
	Loads any cookies based on the response `Set-Cookie` headers.
	"""
	urlib_response = self._CookieCompatResponse(response)
	urllib_request = self._CookieCompatRequest(response.request)
	self.jar.extract_cookies(urlib_response, urllib_request) # type: ignore

httpx/httpx/_models.py

Lines 1118 to 1124 in 7fda99f

	@property
	def cookies(self) -> "Cookies":
	if not hasattr(self, "_cookies"):
	self._cookies = Cookies()
	self._cookies.extract_cookies(self)
	return self._cookies

But we can certainly go through and change things around, if we find out that persist_cookies is indeed something we want.

I think we may be okay with a separate ticket for optional cookie persistence, in any case?

As for 2/, for the ASGI transport case incidentally .aclose() is irrelevant for now, e.g. because we don't support streaming yet (#998). But in principle .aclose() should be relevant in the ASGITransport case too. Eg if a Starlette app fires off some BackgroundTask in a view, then we certainly want the transport's .aclose() to make sure that runs to completion, or gets canceled — and in that case not calling .aclose() will leave resources hanging around, which isn't good. But again, you're right, technically, right now, an unclosed client warning is not necessary when using the ASGITransport.

Is there a way you can call .aclose() when your tests end? Maybe if your datasette.client API has a dependency injection mechanism in place, you could do…

# conftest.py

@pytest.fixture(scope="session")
async def client():
    async with httpx.AsyncClient() as client:
        datasette.some_init_hook(client=client)  # `.some_init_hook` could also be private/internal.

Maybe that initialisation hook doesn't exist for now (I haven't looked through the code, though the docs mention "the datasette.client object", which makes me think it's probably just globally declared?), but that might be a way to solve it.

Alternatively, since Datasette uses ASGI underneath, you might be able to register client.aclose() to run during the lifespan.shutdown stage of the ASGI app? And then eg use asgi-lifespan in tests to make sure lifespan startup/shutdown handlers do run during your test suite.

(There's also a more general concern there about "what do we do with transports that don't keep persistent resources in practice?" (like for mock transports, or ones whose resources are on a per-request basis, etc). I'm not sure we can do anything better than to just ignore that specificity, and default to an agnostic "warn if a request was sent but .aclose() was not called" policy, unless we introduce some kind of way for the transport to be marked as "I don't need to be closed" that the client can use.)

[tomchristie]

Yeah so, two different things here.

I'm absolutely in favour of supporting a no-cookie-persistence option.

I've been surprised in the past that it doesn't(?) exist in requests, particularly because if you're eg. using requests in a web app context, you'll typically use a single global client for most requests, but you absolutely don't want a global client instance to persist cookies in that context, because it's only local to one web app instance, so you'd just end up with inconsistent behaviours depending on which web app each request ends up landing on.

In practise folks just tend not to notice that since they won't have differing endpoints that rely on cookie persistence between requests/responses. But all the same, you end up using a client that'll have potentially odd and inconsistent behaviour wrt cookie persistence.

So, yes I think I'd like us to support this.

Perhaps, with this API?...

client = httpx.Client(cookies=False)

In the meantime I think you can achieve the same behaviour with something like...

from http import cookiejar
import httpx

class NullCookieJar(cookiejar.CookieJar):
    def extract_cookies(self, request, response):
        pass

client = httpx.Client(cookies=NullCookieJar())

Wrt. not closing clients. Yes, it's true that the open/close constraint exists on the transport, not on the client itself.
However, I think we're probably going to want to stick with the simplest thing possible there, just with a view to keeping things uncomplicated. So if instantiation cost is a concern there, then I'd probably advise using eg. client instances that are scoped over the whole duration of the test suite, but with no cookie persistence. Tho we could potentially also look into why is it costly to instantiate multiple clients. (Probably answer: reading and re-reading .netrc files and similar.) So an alternative here might be to look into if eg. Client(trust_env=False) has a lower cost-of-instantiatation, and if you can use that throughout instead?

[backbord]

It does not do any harm as the method's body is empty, but note that the order of arguments seems to be reversed. The signature in the stdlib appears to be extract_cookies(self, response, request)

[kdavh]

@tomchristie I'm wondering if there has been any progress on not persisting cookies across requests since your 2020 comment. To me it's also a no-brainer to have that option, or even to not persist cookies between requests by default. The only reference to shared cookies I see in the docs is if one passes cookies as an argument to the httpx.AsyncClient, but there is no reference to this other implicit behavior.
I was trying to reuse httpx client for multiple request to a backend that returns set-cookie headers for specific user identity cookies on different requests. Unfortunately that would effectively make users' session cookies leak randomly to other users depending on the usage.

[kdavh]

Please correct me, but I think the issue is right here:

httpx/httpx/_client.py

Line 1010 in a934c36

self.cookies.extract_cookies(response)

and

httpx/httpx/_client.py

Line 1722 in a934c36

self.cookies.extract_cookies(response)

So if an option was added, like AsyncClient(save_response_cookies=False) (not cookies=False b/c someone may want some static cookies to be used with each request, but not have cookies stored from previous requests)

So the code would be something like:

if self._save_response_cookies:
  self.cookies.extract_cookies(response)

Maybe it's just me, but during a minor or major version bump, I'd love to see this default to False instead of True how it would have to be now for backwards compatibility. Maybe it's there for auth flows or something, you all could tell me better.