-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add server support for application/grpc+proto
over http
#5505
Conversation
server/http/protolet/protolet.go
Outdated
|
||
"github.com/buildbuddy-io/buildbuddy/server/util/request_context" | ||
requestcontext "github.com/buildbuddy-io/buildbuddy/server/util/request_context" |
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.
nit: move down below unnamed ones
if i == 0 { | ||
w.WriteHeader(200) | ||
} | ||
|
||
// Match our current behavior where we return 500 for all errors and return the message in the response body | ||
w.WriteHeader(500) |
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.
should there be a return after line 286?
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.
Good call, I sent #5509
Not sure if this ever gets hit in practice, but there's definitely supposed to be a return there.
This adds support for server-side streaming (and paves the way for client side streaming with websockets in the future if we want it).
If we end up using this for a bit and liking it, we could delete all of the gnarly reflection protolet.go and simplify that code a ton.
The concepts used here are losely based on https://github.com/improbable-eng/grpc-web/blob/master/go/grpcweb/wrapper.go - so they've been tested in a production environment, but have been greatly simplified.
Overview
Here's how it works:
application/grpc+proto
content type, we forward requests to the grpc server'sServeHTTP
method.ResponseWriter
so we can set an http status and the error message in the response body (grpc and grpc-web always return a 200 success status and then include trailers that contain the status and status message). This matches our current protolet behavior.Binary format
The biggest difference between this and what we're currently doing is that the grpc over http protocol (you can read about this here: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md) expects data to be sent in the
Length-Prefixed-Message
format. Here's what that looks likeCurrently we only send the
Message
bit, so there are some places (like when parsing the request context) where we need to ignore those first 5 bytes. There's a separate PR that sets these 5 bytes on the client side when we're using this new content-type.Misc
In order to support streaming responses, we need all of our
ResponseWriter
s to implementhttp.Flusher
- this PR also adds that (happy to break that out into a separate PR if that helps).The final little quirk in this PR is that we
GenerateHTTPHandlers
after the grpc services are registered because we forward requests to the grpcServer.