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

Activity

1. simonw commented on Oct 9, 2020

   

   simonw

   on Oct 9, 2020

   ContributorAuthor

   More actions

   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.
2. florimondmanca

   added

   discussion

   on Oct 10, 2020
3. florimondmanca commented on Oct 10, 2020

   

   florimondmanca

   on Oct 10, 2020

   Contributor

   More actions

   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.)
4. florimondmanca

   added

   questionFurther information is requestedFurther information is requested

   user-experienceEnsuring that users have a good experience using the libraryEnsuring that users have a good experience using the library

   and removed

   discussion

   on Oct 10, 2020
5. lovelydinosaur commented on Oct 12, 2020

   

   lovelydinosaur

   on Oct 12, 2020

   Last edited by lovelydinosaur

   Member

   More actions

   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?

   👍React with 👍2Reacted by Johannes and Simon Willison
6. lovelydinosaur

   closed this as completedon Mar 24, 2021
7. encode

   locked and limited conversation to collaborators on Mar 24, 2021