V1: Initial server implementation #593
Conversation
cmd/server/main.go
Outdated
| h1Port := flag.String("h1Port", "8080", "port for HTTP/1.1 traffic") | ||
| h2Port := flag.String("h2Port", "8081", "port for HTTP/2 traffic") |
There was a problem hiding this comment.
I apologize that I did not put any details in the design doc about how the server is supposed to work. I had it in my head and failed to actually write it down 🤦.
There was some context about it in the doc, but it was very unclear. I can remedy that and revise the doc. In the meantime, it was supposed to work like this, where the messages sent to/from the server are these.
There was a problem hiding this comment.
Gotcha. My approach for this was to mainly just be able to get the server running and get the logic for each endpoint sound. I was thinking of expanding on how the server is actually fed the test cases in follow-up PRs if that's OK.
There was a problem hiding this comment.
Absolutely. Sorry I wasn't clear -- I wasn't necessarily intending that to be a blocking comment. I'm totally good with adding rampant TODO comments. That way we can always grep for TODO close to the end to see if we are really done. So let's add a TODO for it, but fine to leave as is.
internal/app/server/impl.go
Outdated
| } | ||
| payload, err := parseUnaryResponseDefinition(req.Msg.ResponseDefinition, req.Header(), []*anypb.Any{msgAsAny}) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
You still need to populate response metadata. For unary RPCs, this is done by adding header metadata to a *connect.Error. It has a Meta() method that returns a mutable http.Header.
Sadly, the API provides no way to distinguish between headers and trailers in the error metadata. I think this is because, in practice, errors from unary RPCs often result in "trailers-only responses" in gRPC, which also cannot distinguish.
So I would do something like this here:
| return nil, err | |
| addHeaders(req.Msg.ResponseDefinition.ResponseHeaders, err.Meta()) | |
| addHeaders(req.Msg.ResponseDefinition.ResponseTrailers, err.Meta()) | |
| return nil, err |
There was a problem hiding this comment.
I added this to unary and client stream requests. Should this also be included on the server and bidi requests in the event we return with an error?
There was a problem hiding this comment.
Nah. For those, the stream type the handler gets has ways to set headers and trailers separately. If you do add metadata to the error, it will get merged into the trailers. But I don't think we want or need to do that here for those endpoints.
Co-authored-by: Joshua Humphries <[email protected]>
Co-authored-by: Joshua Humphries <[email protected]>
| requestInfo := createRequestInfo(stream.RequestHeader(), reqs) | ||
| resp := &v1alpha1.BidiStreamResponse{ | ||
| Payload: &v1alpha1.ConformancePayload{ | ||
| RequestInfo: requestInfo, | ||
| Data: responseDefinition.ResponseData[respNum], | ||
| }, | ||
| } | ||
| time.Sleep((time.Duration(responseDefinition.ResponseDelayMs) * time.Millisecond)) | ||
|
|
||
| if err := stream.Send(resp); err != nil { | ||
| return connect.NewError(connect.CodeInternal, fmt.Errorf("error sending on stream: %w", err)) | ||
| } |
There was a problem hiding this comment.
This could probably be pulled out into its own function also. Lmk if you think it is worth it.
|
@jhump ready for round 2. |
|
|
||
| // Parses the given unary response definition and returns either | ||
| // a built payload or a connect error based on the definition. | ||
| func parseUnaryResponseDefinition( |
There was a problem hiding this comment.
I guess I think or "parse" differently than that. It's fine either way -- not a strongly held opinion :)
smaye81
changed the title
|
OK, just going to leave the function name as-is for now. We can definitely rename it later if it's confusing. |
jhump
changed the title
This adds a first attempt at the reference implementation for a conformance server.
It adds the full implementation of all endpoints as well as an ability to run it via
cmd/server/main.go.