Skip to content
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.

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

Map httpcore exceptions for Response read methods #1190

Merged
merged 4 commits into from
Aug 19, 2020

Conversation

j178
Copy link
Member

@j178 j178 commented Aug 17, 2020

Fix #1188
Add map_exceptions to Response.iter_raw and Response.aiter_raw, making sure all httpcore exceptions are mapped.
And add a slow_stream response test to make sure ReadTimeout ocurred while the client is reading the response body, not in the request time.

@j178 j178 requested a review from a team August 17, 2020 16:13
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Interesting…!

The main reason I'm not immediately "OK let's go" on this is that ReadError & co only have a request=... parameter, but in this case we do have a response available that we could attach to the exception. Perhaps no need to sweat it since we have the response object available in the scope anyway when doing local exception handling, but just wanted to leave this heads up.

with httpx.stream(...) as response:
    try:
        for line in response.iter_lines():
            ...
    except httpx.ReadError as exc:
        print("Stream broken", response)  # No need for `exc.response` anyway?

tests/test_exceptions.py Outdated Show resolved Hide resolved
j178 and others added 2 commits August 19, 2020 16:42
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
@tomchristie
Copy link
Member

@florimondmanca Well noted. There's already other exception classes that can fall into that category, which I'm totally okay with, because it's consistent & simple. As you say, the response will be available in the local scope anyways.

@tomchristie
Copy link
Member

There's also an alternate implementation where we wrap around the stream that we pass to the Response instance, here...

httpx/httpx/_client.py

Lines 795 to 801 in d10b7cd

response = Response(
status_code,
http_version=http_version.decode("ascii"),
headers=headers,
stream=stream, # type: ignore
request=request,
)

Eg. something along these lines...

ResponseStream(ContentStream):
    def __int__(self, request, stream):
        self._request = request
        self._transport_stream = stream

    def __iter__(self):
        with map_exceptions(HTTPCORE_EXC_MAP, request=self._request)
            for chunk in self._transport_stream:
                yield chunk

    def close(self):
         self._transport_stream.close()

Then...

response = Response(
   ...
   stream=ResponseStream(request, stream)
)

I don't see any reason for us to prefer that here & now, but mentioning it in case it's a useful reference point for anything in the future.

@tomchristie tomchristie merged commit 03cd88c into encode:master Aug 19, 2020
@j178 j178 deleted the map-response-read-methods-exceptions branch August 20, 2020 01:30
@tomchristie tomchristie mentioned this pull request Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

httpcore.ReadTimeout not mapped to httpx.ReadTimeout
3 participants