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

Move response body handling into HTTP2Response #3294

Open
sethmlarson opened this issue Jan 21, 2024 · 3 comments
Open

Move response body handling into HTTP2Response #3294

sethmlarson opened this issue Jan 21, 2024 · 3 comments
Labels
💰 Bounty $500 If you complete this issue we'll pay you $500 on OpenCollective!
Milestone

Comments

@sethmlarson
Copy link
Member

sethmlarson commented Jan 21, 2024

Currently response handling occurs within the connection itself, but once we have response headers we should be handing off the body processing to the HTTP2Response object. This includes support for decoding gzip and other compressed bodies.

@sethmlarson sethmlarson added the 💰 Bounty $500 If you complete this issue we'll pay you $500 on OpenCollective! label Jan 21, 2024
@sethmlarson sethmlarson added this to the HTTP/2 milestone Jan 21, 2024
@pquentin
Copy link
Member

The title of this issue states "Move response body handling into HTTP2Response", but the text says "hand off the body processing to the HTTPResponse object". So which did you mean? Given #3297, my understanding is that we'll have an HTTP2Response class that will subclass from BaseHTTPResponse. Is that right?

classDiagram
    BaseHTTPResponse <|-- HTTPResponse
    BaseHTTPResponse <|-- HTTP2Response

@sethmlarson
Copy link
Member Author

@pquentin That's correct!

@pquentin
Copy link
Member

pquentin commented Jan 23, 2024

Thanks for the clarification!

One subtlety compared to the HTTP/1.1 case with http.client is that today urllib3.connection.getresponse calls http.client.HTTPConnection.getresponse. From the content of the resulting http.client.HTTPResponse, it can create an urllib3.response.HTTPResponse (without inheritance). As a result, BaseHTTPResponse expects status, http version and headers when it's initialized. (Which provides a nice API: when urllib3.request returns a response, we can already inspect the headers and status code.)

For HTTP/2, if we completely move the current content of HTTP2Connection.getresponse in HTTP2Response, we won't be able to initialize the response with a status, http version and headers. Instead, I think HTTP2Connection.getresponse should start reading from the socket until it receives an h2.events.ResponseReceived event. At this point it will be able to build an HTTP2Response with status and headers and transfer the ownership of the state (socket, H2Connection, h2 lock and stream id) to HTTP2Response so that it can implement read() and stream().

To avoid ending up with two h2 event loops (one in HTTP2Connection and one in HTTP2Response), we could decide to have an object that owns the state and implement the h2 event loop, and it's that object that will be transferred between HTTP2Connection and HTTP2Response. The object will allow hiding some (but not all) specifics of the hyper-h2 API such as automatic flow control and decoding of headers. We could have one per multiplexed stream, and maybe call it ˋHTTP2Stream`

@sethmlarson What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💰 Bounty $500 If you complete this issue we'll pay you $500 on OpenCollective!
Projects
None yet
Development

No branches or pull requests

2 participants