map_exceptions in request.aclose()#1465
Conversation
Now, clients can catch an exception that occurs during close. [closes encode#1463]
|
sigh I'm having a hard time reproducing this error on py36. Is that strictly necessary? I feel the code fix is obviously correct and the unit test is spectacularly difficult to write. |
|
@adamhooper Thanks! For the unit test — I think we can leverage |
florimondmanca
left a comment
There was a problem hiding this comment.
Looking very neat!
Just a minor suggestion to increase test coverage slightly?
| async with client.stream("GET", "http://example.com") as response: | ||
| with pytest.raises(httpx.CloseError): | ||
| await response.aclose() |
There was a problem hiding this comment.
Am I correct saying this would also reproduce for a regular client.request()? If so, should we update this to…?
| async with client.stream("GET", "http://example.com") as response: | |
| with pytest.raises(httpx.CloseError): | |
| await response.aclose() | |
| with pytest.raises(httpx.CloseError): | |
| await client.request("GET", "http://example.com") | |
| with pytest.raises(httpx.CloseError): | |
| async with client.stream("GET", "http://example.com"): | |
| pass # Close on exit. |
(Also dropped the explicit response.aclose() since the expected behavior is that exiting the stream() async context manager would do that for us, and fail with the proper exception.)
There was a problem hiding this comment.
You are correct. And the await client.request() test is a worthy addition.
You're the maintainer, so you are welcome to change to nix the aclose() in the test ... but first, I'll explain my motivation.
During debugging, I was surprised by #1464 -- a broken stream raises the "wrong" error on shutdown.
I imagine a reasonable __aexit__() might handle an errno=104 error -- by doing nothing and raising nothing. So I aimed for an explicit test that avoids __aexit__().
The change would be correct. But as a user, I find __axit__() behavior confusing; so I wrote a more-explicit test.
There was a problem hiding this comment.
Yup, I like the way @adamhooper has approached the test case here, too.
lovelydinosaur
left a comment
There was a problem hiding this comment.
Good catch here, yup. Happy with this one.
Now, clients can catch an exception that occurs during close.
[closes #1463]