-
Notifications
You must be signed in to change notification settings - Fork 121
Implements "trailing_headers" for HTTP/2 #1012
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
base: master
Are you sure you want to change the base?
Implements "trailing_headers" for HTTP/2 #1012
Conversation
30a4ae9
to
e70c821
Compare
e70c821
to
3dce1d3
Compare
@tomchristie @graingert Sorry for the noisy, but gentle ping. |
Thanks. Given the overhead this functionality looks like an unwin for most users. |
@tomchristie How about I modify the implementation so that the client only processes response trailers when the request includes the header {"TE": "trailers"}? That way, there should be no additional overhead in the typical case. I noticed you tried a similar approach before in this PR: |
Hi @tsubakiky - I randomly noticed this PR thinking of what it would take to implement support for grpc protocol with httpx because grpc requires trailers. But one note, if your target is just connect protocol, you shouldn't need HTTP trailers. In your linked page "The protocol doesn't use HTTP trailers at all" (actually I also missed this point initially 😂 ) Basically, they're encoded into the final JSON message EndStreamResponse. So while it could be nice to have trailers supported in httpx, if grpc users will generally stick to grpcio anyways, then maybe the priority on this indeed isn't so high. Mostly just wanted to make sure you're not blocked when using connect protocol. PS: I have been collaborating with @i2y on https://github.com/i2y/connecpy/ It sounds like there may be some overlap in work, it would be great if we could work together on it! |
We send some information about a generated request body in trailer headers as this information is only available once the full response was generated. Sadly neither requests nor httpx supports trailer headers which makes our python customers not be able to consume that content. Would love to see this merged! |
@tomchristie Would requiring a user to specifically opt-in to receiving trailer headers for a given request be sufficient to get this moving? That way for the 99.9% case there is no additional overhead. |
@bbartels It's more the code overhead that's an issue. Particularly since the HTTP/2 implementation is already fiddly and hard to follow. "specifically opt-in" - Configuration dials == user facing complexity. |
Fair point, I am relatively unfamiliar with the project (and the python ecosystem in general)
I am all for reducing friction for developers, but I feel like in this instance you are already down a rabbit hole if you need to parse trailer headers 😅 It's just unfortunate that I have to tell python based consumers of our api to drop down to |
Summary
HTTP/2 Trailing Headers Support
This PR adds support for HTTP/2 trailing headers in httpcore. Trailing headers are now properly received, processed, and made available in the response extensions.
Background:
I’m currently developing a Connect-protocol–compliant library that doesn’t depend directly on grpcio. I’ve implemented the client with httpcore, but because I needed to receive trailer headers in HTTP/2 streaming, I introduced the following modifications.
https://connectrpc.com/docs/protocol
Changes
h2.events.TrailersReceived
eventstrailing_headers
Implementation Details
HTTP2ConnectionByteStream
to update response extensions when trailing headers are receivedThis implementation safely handles HTTP/2 trailing headers according to the HTTP/2 specification, ensuring all header data is correctly accessible to clients.
Checklist