Better developer experience for accessing Request/Response bodies in event_hooks and httpx.Auth

[mshober]

One inconvenience I came across when trying out httpx is that it requires that you call response.read() (or response.aread() for AsyncClients) in event hooks if you need to access the response body.

While at first glance this seems like a simple enough requirement, consider the following two code snippets:

try:
    response = httpx.get("https://httpstat.us/400")
    response.raise_for_status()
except httpx.HTTPStatusError as err:
    print(err.response.text) # Exception is caught and the response text is printed as expected

def raise_errors(response, *args, **kwargs):
    response.raise_for_status()

client = httpx.Client(event_hooks={"response": [raise_errors]})

try:
    response = client.get("https://httpstat.us/400")
except httpx.HTTPStatusError as err:
    print(err.response.text) # httpx.ResponseNotRead: Attempted to access streaming response content, without having called `read()`.

The two examples attempt to accomplish the same thing, but lead to different outcomes because of this behavior.

In the second snippet, it doesn't make sense for the event hook to call response.read() because it does not directly access the response body. It also is not always apparent to the user of the client that they should call response.read() when catching exceptions: From my experience, it's common for event hooks to be encapsulated from the user, and the user may not be aware of the event hooks used on the client (because that's none of their concern). With httpx's current design, it's not clear who's responsibility it is to call response.read().

The response body is also not accessible to subclasses of httpx.Auth by default. However, httpx.Auth handles that in a completely different way through the use of the requires_response_body property (unless you're using the callable auth type in which case you do need to directly call response.read()). This inconsistent handling of the response body makes using httpx more complex than it should be.

I don't see any benefit from requiring users to explicitly specify when they need to access the response body and throwing a ResponseNotRead exception at them when they don't.

I propose that response.content should automatically read and return the response body if it hasn't already been read. Same thing goes for request.content. This would remove the need for the ResponseNotRead and RequestNotRead exceptions as well as the requires_response_body and requires_request_body properties from httpx.Auth.

[tomchristie]

Thanks so much for a well worded proposal. 💚
Let's work through this...

What are the implications when using the client.stream(...) API?

[mshober]

Any feedback @tomchristie? Can I work on a PR for this?

[tomchristie]

Can I work on a PR for this?

Depends exactly what you're aiming for here, eg...

I propose that response.content should automatically read and return the response body if it hasn't already been read

This isn't quite something we'd want. Properties that implicitly perform I/O are a code smell "you looked at this thing but we're going to act as if you pushed a button", and in any case aren't supported by async codebase (which require the explicit async/await, essentially because they require strongly typed I/O.)

What might be a feasible proposal is changing the point at which response.read() occurs, so that response.content is available within auth and event hooks so long as a non-streaming request is being made.

[tomchristie]

What might be a feasible proposal is changing the point at which response.read() occurs, so that response.content is available within auth and event hooks so long as a non-streaming request is being made.

If you'd like to invest time in that I'd happily help you work through it.

[mshober]

Thanks for the feedback! Yes that all makes sense.

I took a look at the codebase and noticed that response event hooks will run on redirects. I was initially going to point out that it might be a negative side effect to unnecessarily be reading the response body on every redirect response (a very minor side effect though, since following redirects is not the default behavior). However is looks like we're already doing that. Not sure what purpose that serves; might be some legacy code that's no longer needed.

If that code is something that might eventually be removed and reading the response body on redirects is not a desired behavior, then I'd be fine with an alternative of just ensuring that the response from HTTPStatusError (i.e. when calling response.raise_for_status()) has been read for non-streaming requests.

What do you think?

[tomchristie]

Implementation-wise I think we'd effect this by passing stream=<bool> through the call stack to _send_single_request so that we switch between streaming and non-streaming as close to the transport layer as possible.

The implication for event hooks would be that the response might or might not be a streaming response... it'd be okay to install event hooks accessing response.content if you're not using client.stream(), but if you are then you're going to see an exception at the point you access it. So long as the error messaging is clear there "you're attempting to access response.content with a streaming response. Blergh." then that's fine.

However is looks like we're already doing that. Not sure what purpose that serves; might be some legacy code that's no longer needed.

Reading the response to completion on redirects is desired behaviour. With HTTP/1.1 this means you've read the response to completion and the connection becomes available for another request/response cycle. We'd want to keep this behaviour either ways. (A code comment there would be helpful)