-
Notifications
You must be signed in to change notification settings - Fork 91
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
h2/h3 trailing header support. Fixes #147 #162
Conversation
@pgjones can you provide feedback/comments on my stuff so far? Don't want to get to far into this if I'm going in the wrong direction. Thanks. |
34a4e39
to
3d991b4
Compare
Thanks for this, I've adapted it a bit and merged in d8de5f2 |
and self.scope["http_version"] in TRAILERS_VERSIONS | ||
and self.state | ||
in { | ||
ASGIHTTPState.REQUEST, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffsawatzky why allow trailers to be sent before the response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgjones because there may not be a response. gRPC uses the trailing headers for error responses, so if there was an error server side before any response was sent then it would just return a trailing header and no response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgjones if you look at this:
https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md
The response could be trailers only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the status would have to be 200 (I don't think there is a self.response
to use), does this make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my usecase with gRPC, yes that makes sense. All gRPC responses are supposed to be 200.
https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses
Whether it makes sense generally, I think so?
I did a quick test with this implementation with python-grpc-client. The asgi app just works like a grpc server, however, the client is not happy with the server and lost the connection half way. It seems that the server send trailers after EndBody been send, which closed the connection. maybe we should avoid this. I'll dig deeper tomorrow. |
Thanks both, hopefully fixed with d16b503 |
Emmmm... That didn't work either. Here is my asgi app which pretend to be a grpc server(A simple grpc echo server)
A real grpc client would expect the same response type and value when talk to the server, but with grpclib in python, I just got
And here is the client side:
Besides, grpcurl also failed with |
This is an initial implementation of trailing header suppport for http2/http3. This fixes #147, in theory.
Here's the thing though...I have no clue what I am doing. I would like some tips on how to do the following:
I assume the underlying
h2
andh3
libraries take care of (2) for me, but I'm not sure.