Skip to content

map_exceptions in request.aclose()#1465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lovelydinosaur merged 10 commits into from
Feb 17, 2021
Merged

map_exceptions in request.aclose() #1465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
25a5a68
map_exceptions in request.aclose()
adamhooper Feb 10, 2021
9badecd
test_async_client.py: fix py36 and coverage
adamhooper Feb 10, 2021
f5daf4c
Another crack at py3.6
adamhooper Feb 10, 2021
7ef83a7
oh, now I see what py36 is all about
adamhooper Feb 10, 2021
7b29221
Mock Stream to make unit tests cross-platform
adamhooper Feb 11, 2021
aa84237
Fix coverage
adamhooper Feb 11, 2021
6f31794
Fix coverage with pragma
adamhooper Feb 11, 2021
81cac00
Merge branch 'master' into 1463-response-map-exceptions
lovelydinosaur Feb 17, 2021
b9b7ebc
Merge branch 'master' into 1463-response-map-exceptions
lovelydinosaur Feb 17, 2021
810a7ea
Merge branch 'master' into 1463-response-map-exceptions
lovelydinosaur Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion httpx/_client.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1500,7 +1500,8 @@ async def _send_single_request(
async def on_close(response: Response) -> None:
response.elapsed = datetime.timedelta(seconds=await timer.async_elapsed())
if hasattr(stream, "aclose"):
await stream.aclose()
with map_exceptions(HTTPCORE_EXC_MAP, request=request):
await stream.aclose()

response = Response(
status_code,
Expand Down
19 changes: 19 additions & 0 deletions tests/client/test_async_client.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -303,6 +303,25 @@ async def test_mounted_transport():
assert response.json() == {"app": "mounted"}


@pytest.mark.usefixtures("async_environment")
async def test_response_aclose_map_exceptions():
class BrokenStream:
async def __aiter__(self):
# so we're an AsyncIterator
pass # pragma: nocover

async def aclose(self):
raise httpcore.CloseError(OSError(104, "Connection reset by peer"))

def handle(request: httpx.Request) -> httpx.Response:
return httpx.Response(200, stream=BrokenStream())

async with httpx.AsyncClient(transport=httpx.MockTransport(handle)) as client:
async with client.stream("GET", "http://example.com") as response:
with pytest.raises(httpx.CloseError):
await response.aclose()
Comment on lines +320 to +322

@florimondmancaflorimondmancaFeb 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct saying this would also reproduce for a regular client.request()? If so, should we update this to…?

Suggested change
asyncwithclient.stream("GET", "http://example.com") asresponse:
withpytest.raises(httpx.CloseError):
awaitresponse.aclose()
withpytest.raises(httpx.CloseError):
awaitclient.request("GET", "http://example.com")
withpytest.raises(httpx.CloseError):
asyncwithclient.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.)

@adamhooperadamhooperFeb 12, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lovelydinosaurlovelydinosaurFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I like the way @adamhooper has approached the test case here, too.



@pytest.mark.usefixtures("async_environment")
async def test_async_mock_transport():
async def hello_world(request):
Expand Down