Tweak transport closing

[lovelydinosaur]

Closes #1317

• Always close transports if client.close() is called, even if no requests have been made.
• Treat client instances as "open" as soon as they're instantiated, even if no requests have been made.

[florimondmanca]

Exactly what I had in mind :)

[johtso]

We probably want to cover the new transport closing behavior in the tests:

classTestTransportClosing:
def__init__(self):
# Note that we're including 'proxies' here to *also* run through the# proxy context management, although we can't easily test that at the# moment, since we can't add proxies as transport instances.## Once we have a more generalised Mount API we'll be able to remove this# in favour of ensuring all mounts are context managed, which will# also necessarily include proxies.self.transport=Transport()
self.client=httpx.Client(transport=self.transport, proxies="http://www.example.com")
deftest_with_context_manager(self):
assertself.transport.events== []
withself.client:
passassertself.transport.events== [
"transport.__enter__",
"transport.close",
"transport.__exit__",
]
deftest_without_context_manager(self):
assertself.transport.events== []
self.client.close()
assertself.transport.events== [
"transport.close"
]

[johtso]

Also, we now get a lot of unclosed client UserWarnings when running the tests.

[lovelydinosaur]

Also, we now get a lot of unclosed client UserWarnings when running the tests.

Okay, right, which kinda gets to the heart of the two different options here.

We can either:

• Suggest that transport instances shouldn't hold resources on init, but should either do so on __enter__ or if opened implicitly the first time a .request() is made.
• Treat clients as open as soon as they're instantiated, and if they've not been explicitly closed, then close them on __del__.

We're in a slightly different position with Client and AsyncClient because Client.__init__ might perform I/O and allocate resources, and Client.__del__ is always able to release them, whereas AsyncClient.__init__ won't ever perform I/O, and AsyncClient.__del__ is unable to perform I/O to release resources.

Stale

[lovelydinosaur]

Also, we now get a lot of unclosed client UserWarnings when running the tests.

So, I've made a slight tweak here, for AsyncClient only...

__del__ should only raise a warning, if the client is unclosed and has performed I/O. (A request or __aenter__)

(We don't care about this for Client because __del__ never needs to raise a warning, since it can always just call .close on an unclosed client)

Not sure what we think about this.

Incidentally wrt. CacheControl if you're expecting to expand it into sync+async support then you will want to only initialise the resources either on __enter__ or on the first time request is called, because async can't make I/O in the __init__ call. (because it's always a sync method)

[johtso]

Was thinking exactly along these lines, makes sense to me.

Incidentally wrt. CacheControl if you're expecting to expand it into sync+async support then you will want to only initialise the resources either on __enter__ or on the first time request is called, because async can't make I/O in the __init__ call. (because it's always a sync method)

Ah yes, look forward to exploring the a/sync rabbit hole!

[florimondmanca]

@tomchristie Heads up — we've got an interesting real-world case that we could test this new logic against in #1332.

[lovelydinosaur]

Superseeded by #1346