Repository navigation
Client.close() does not close transports if unused #1317
Description
Currently, Client.close() closes its transports if and only if the client is deemed to be "open" (not self._is_closed).
When the client is instantiated, it is not considered open, rather only when something is sent, or when it is __enter__ed.
Proxies and transports are initialized when the client is instantiated.
This means that if a client is not used as a context manager, and no requests are sent with it, proxies and transport are not closed when Client.close() is called.
This obviously doesn't cause any problems with the standard transports, but a custom transport may be in use that sets up some other resources.
Could we not just .close() the transports regardless of whether the client has been used?
Activity
[-]Client.close() does not close transports if unused[/-][+]`Client.close()` does not close transports if unused[/+][-]`Client.close()` does not close transports if unused[/-][+]Client.close() does not close transports if unused[/+]florimondmanca commentedon Sep 24, 2020
Well, looks related to #1197 which I personally hadn't gone through and reviewed properly at the time…
The intiialization here in particular seems dubious: https://github.com/encode/httpx/pull/1197/files#diff-eff812167840d2389ea5f0d809b30c32R83
Instead of initializing it to
False, we're setting it toTrueand then turning the flag down when making our first request... https://github.com/encode/httpx/pull/1197/files#diff-eff812167840d2389ea5f0d809b30c32R708This seems to suppose that the client can be "reopened" when it's been closed, which I don't think is the way we want to think about a client. When it's been closed, then that's it — no more reusing it. Clients are not "re-entrant".
So in my mind we should init
self._is_closed = False, drop any switch of that flag in.request()or__enter__(), and we should be good wrt theif self.is_closedcheck in__exit__(). Thoughts?florimondmanca commentedon Sep 24, 2020
If you're up for it, seems like a quick fix we could sneak into 0.15.3 (#1318). :-)
johtso commentedon Sep 24, 2020
@florimondmanca will take a bash!
lovelydinosaur commentedon Sep 24, 2020
@johtso Ah crossed wires - I'd already started taking a go at this too. #1320
We'd not been explicit about that before, but it's probably the sensible thing to do, yup.
I'd suggest we probably want the next proper version bump to hard-enforce that, raising an error if you attempt to send a request on a closed client.
lovelydinosaur commentedon Sep 24, 2020
FWIW I'd consider this more UX than bug. It's also perfectly sensible to treat a client as "closed" until either
__enter__is called, or a request is made.If you do want clients that can be reopened, then that makes sense, which I think was partly what pushed us in this direction originally, but yeah the more constrained variation is more in line with requests, and probably more in line with user expectations.
lovelydinosaur commentedon Sep 25, 2020
Re-labeled this, since it's fuzzy.