Fix response not closed when read stream canceled

[guyskk]

See #2139 (comment), the PR only fix issues of httpx client.

[vrslev]

Maybe catch Exception and asyncio.exceptions.CancelledError?

[guyskk]

Here we need catch all exceptions to ensure response closed, just like async with context manager. And the exception will re-raised so it will not swallow errors.

[tomchristie]

Great thank you.

Yup, I'm in favour of switching over to BaseException here - it makes sense to always close the response. Including for asyncio or trio cancelations or SystemExit, and KeyboardInterrupt.

I'm not keen on us having test cases that introduce sleeps, tho. Perhaps we could change those around somehow.

[guyskk]

In Python 3.6, asyncio.wait_for will not cancel the task, so I skip the test in py36. https://stackoverflow.com/questions/69370252/asyncio-wait-for-didnt-cancel-the-task

[florimondmanca]

I think we should move the close() / aclose() calls to finally blocks instead of except BaseException blocks, see my analysis here: #2139 (comment) (The result is essentially the same - "always close" but perhaps it's cleaner)

[guyskk]

Hi @florimondmanca Seems the response not always need close, eg stream request: https://github.com/encode/httpx/blob/master/httpx/_client.py#L904 .
And in handle redirect cases, I'm not sure if the response should close https://github.com/encode/httpx/blob/master/httpx/_client.py#L969.

[tomchristie]

Thanks @guyskk.

I've taken another pass at this in #2156, with a different approach to the test case, so that we don't need to introduce slow sleep behaviour into our test suite. (Anywhere we can avoid the tests using actual network I/O we should)

[guyskk]

@tomchristie 👍👍 your test case is perfect!

[tomchristie]

Thanks folks. Closing this in favour of #2156.

We also ought to address any similar cases in httpcore.