make available header and trailer metadata #93

kazegusuri · 2016-01-25T13:18:02Z

gRPC recommends application-level error information should be returned as Trailer metadata. We have to use grpc.CallOption for each rpc call in order to get the metadata at client side. This PR makes protoc-gen-grpc-gateway generates codes with grpc.CallOption. And the metadata are set in context, so handlers can extract the metadata by runtime.CallInfoFromContext.

Actually I had a plan to return those metadata as HTTP header or body in JSON especially in DefaultHTTPError handler. However metadata possibly includes binary or values must be escaped, so default handler doesn't care metadata by default.
EDIT: So I recommend to define a handler which utilizes metadata and overwrite default handlers.

Any ideas or comments are welcome.

yugui · 2016-01-26T07:24:19Z

protoc-gen-grpc-gateway/gengateway/template.go

@@ -208,7 +208,7 @@ var (
 	}
 {{end}}

-	return client.{{.Method.GetName}}(ctx, &protoReq)
+	return client.{{.Method.GetName}}(ctx, &protoReq, grpc.Header(&callInfo.HeaderMD), grpc.Trailer(&callInfo.TrailerMD))


These call options are effective only if .Method.GetServerStreaming and .Method.GetClientStreaming are false.
The caller of this request_xxx function must use grpc.ClientStream.Header() and grpc.ClientStream.Trailer if otherwise.

IIUC, it would get easier to handle this case if you have let this request_xxx function allocate the metadata.MDs and let the function return the MDs in addition to response and errors.

How about renaming runtime.CallInfo to runtime.ServerMetadata and declaring the ! .Method.GetServerStreaming() version of this function as something like this?

func request_{{.Method.Service.GetName}}_{{.Method.GetName}}_{{.Index}}(ctx context.Context, client {{.Method.Service.GetName}}Client, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error)

With this approach forward_xxx function does not need to care about .GetClientStreaming.

kazegusuri · 2016-01-27T14:29:07Z

Thank you for the comment. I haven't take care streaming because I have never touched.
It sounds good to rename to runtime.ServerMetadata and return it from function.
Event after that, the caller of request_xxx set the metdata to context to my understanding, right?
If this is okay, I will populate the changes.

yugui · 2016-01-28T05:01:33Z

the caller of request_xxx set the metdata to context

Yes.

Also I am thinking of forwarding headers and trailers in the forwarder functions. What do you think about it?

kazegusuri · 2016-01-30T08:50:37Z

Also I am thinking of forwarding headers and trailers in the forwarder functions. What do you think about it?

Sorry, I've missed the comment. The metadata are set to context, so we can extract byruntime.ServerMetadataFromContext(ctx) from anywhere including forwarder functions.
There is no need to pass the metadata by arguments, but it's not strong opinion.

kazegusuri · 2016-01-31T09:14:33Z

Addressed comments, but have problems in streaming rpc.

Receiving header in stream.

stream.Header() is blocking method until header is ready or context has done, so calling stream.Header at first in request_xxx blocks forever if server does not send headers first by grpc.SendHeader.
Another way is to call stream.Header() after sending message, which means add following code after here.

    msg, err := stream.CloseAndRecv()
    md, err := stream.Header()
    if err != nil {
        glog.Errorf("Failed to get stream header: %v", err)
        return nil, metadata, err
    }
    metadata.HeaderMD = md
    metadata.TrailerMD = stream.Trailer()
    return msg, metadata, err

It works well in my small example with streaming, but in integration test it fails sometimes with following error.

integration_test.go:359: {"error":"stream error: code = 1 desc = \"context canceled\"","code":2}

I'm not sure why context was canceled.

Receiving trailer in stream

stream.Trailer() must be called after stream.Recv(), so we cannot get trailer in request_xxx when .Method.GetServerStreaming is true because recv() is called in forward_xxx.
IIUC, Trailer is used for application-level error information. How about passing Trailer to handleForwardResponseStreamError when recv() returns error in ForwardResponseStream and user can define stream error handler like DefaultHTTPError?

… context

kazegusuri · 2016-02-06T10:51:00Z

I have fixed metadata handling with some limitations; Dropping support for header metadata in streaming RPC because it's a bit difficult, is it acceptable? And added support to return metadata to client by default.

Header metadata is returned to client by HTTP header with Grpc-metadata- prefix in both normal response and error response, but only on Unary RPC.
Trailer metadata is returned to client by response body with top-level trailer key and json value only on error response, but both Unary RPC and Streaming RPC are supported.

PTAL @yugui

yugui · 2016-02-08T01:32:47Z

@kazegusuri

I have fixed metadata handling with some limitations; Dropping support for header metadata in streaming RPC because it's a bit difficult, is it acceptable?

Yes. Let me examine it by myself later. But it is acceptable for now.

And added support to return metadata to client by default.
1.Header metadata is returned to client by HTTP header with Grpc-metadata- prefix in both normal response and error response, but only on Unary RPC.

OK.

2.Trailer metadata is returned to client by response body with top-level trailer key and json value only on error response, but both Unary RPC and Streaming RPC are supported.

It looks weird to put it in response body because it was metadata, and the application-level error does not belong to a specific chunk of body.
I understand that it is hard to implement for streaming RPC, but is it possible to forward trailer metadata as HTTP 1.1 trailers?

kazegusuri · 2016-02-08T02:47:35Z

It looks weird to put it in response body because it was metadata, and the application-level error does not belong to a specific chunk of body.
I understand that it is hard to implement for streaming RPC, but is it possible to forward trailer metadata as HTTP 1.1 trailers?

I never knew HTTP 1.1 trailer. I'll try it.
I think it's better to use HTTP 1.1 trailer for streaming RPC. Do you think to use it for Unary RPC?

achew22 · 2016-02-08T03:14:48Z

@yugui I can't find any evidence that any browsers actually support HTTP 1.1. How do you get access to trailers in an XHR object?

yugui · 2016-02-08T04:27:23Z

@achew22 Good point.
getResponseHeader looks to be expected to contain trailers, although Safari doesn't implement it.
https://github.com/w3c/web-platform-tests/blob/master/XMLHttpRequest/getresponseheader-chunked-trailer.htm

@kazegusuri Although the most straightforward mapping of gRPC trailer metadata is HTTP trailers, there are several things to consider.

As @achew22 pointed, it might not be compatible to browser implementations.
We can list up trailer fields before sending response body for unary RPC. But we cannot for streaming RPC. So we cannot send Trailer header in such a case -- it violates a recommendation in RFC 2616.

https://tools.ietf.org/html/rfc2616#section-14.40

An HTTP/1.1 message SHOULD include a Trailer header field in a
message using chunked transfer-coding with a non-empty trailer

kazegusuri · 2016-02-11T09:53:01Z

My first thought is the use case of gRPC trailer is specific for error details, so I attached the metadata to error message for both unary and streaming rpc.
But I understand it's metadata and it's good design to map gRPC trailer to HTTP 1.1 trailer.

I understand that it is hard to implement for streaming RPC, but is it possible to forward trailer metadata as HTTP 1.1 trailers?

Possible for unary RPC and streaming RPC with client streaming, but impossible for with server streaming.
Go 1.5 http server does not send trailers which are not included in Trailer header. When gRPC trailer is sent to gateway while receiving streaming, HTTP header was already sent to client and we cannot always know trailer before sending header.

We can list up trailer fields before sending response body for unary RPC. But we cannot for streaming RPC. So we cannot send Trailer header in such a case -- it violates a recommendation in RFC 2616.

As described above, yes we cannot use HTTP 1.1 trailer for streaming RPC.
So I used HTTP 1.1 trialer only for unary RPC, and for streaming RPC grpc trailer is returned as independent chunk like error message. In both case grpc trailer is always returned to client if set regardless of the response is error or not. Does this make sense? @yugui

As @achew22 pointed, it might not be compatible to browser implementations.

I cannot know how impact to use HTTP 1.1 trailer.
Anyway HTTP 1.1 trailer cannot be used for streaming RPC and response body is used for that case in current impl. If this can be a problem, it's a reasonable option to use response body for unary RPC, too.

yugui · 2016-02-16T04:38:42Z

@kazegusuri

As described above, yes we cannot use HTTP 1.1 trailer for streaming RPC.
So I used HTTP 1.1 trialer only for unary RPC, and for streaming RPC grpc trailer is returned as independent chunk like error message. In both case grpc trailer is always returned to client if set regardless of the response is error or not. Does this make sense? @yugui

Yes, it makes sense.
Strictly speaking, we can still send HTTP 1.1 trailers in server streaming RPCs without the corresponding Trailer header if we hijack the HTTP connection. But I don't think it is worth to try because it makes the implementation much complicated and it violates the recommendation in RFC 2616.

Summarizing, I expect the following spec. Is this correct?

Header metadata are mapped to HTTP headers which have Grpc-Metadata- prefix. It is supported in all of unary, client-streaming, server-streaming and both-streaming RPCs.
Trailer metadata are mapped to HTTP trailers which have Grpc-Trailer- prefix. But it is supported only in unary RPC and client-streaming RPC.
- We can reconsider trailers in server-streaming RPCs when we have a concrete use case.

kazegusuri · 2016-02-16T06:39:38Z

Header metadata are mapped to HTTP headers which have Grpc-Metadata- prefix. It is supported in all of unary, client-streaming, server-streaming and both-streaming RPCs.

Not supported for server-streaming and both-streaming RPCs (at least this PR) because header timing issue.

Trailer metadata are mapped to HTTP trailers which have Grpc-Trailer- prefix. But it is supported only in unary RPC and client-streaming RPC.

Yes. But for server-streaming and both-streaming, Trailer metadata are returned as an extra chunk of message body.

yugui · 2016-02-16T07:13:10Z

Not supported for server-streaming and both-streaming RPCs (at least this PR) because header timing issue.

You need to call stream.CloseSend. Then you can safely call Header() before CloseAndRecv.

Yes. But for server-streaming and both-streaming, Trailer metadata are returned as an extra chunk of message body.

I am not confident if it is the right solution. So it looks better to conservatively avoid supporting trailer metadata in server-streaming and both-streaming. Is it OK for you?

kazegusuri · 2016-02-17T13:53:50Z

You need to call stream.CloseSend. Then you can safely call Header() before CloseAndRecv.

Thank you for comment. I will check it out.

I am not confident if it is the right solution. So it looks better to conservatively avoid supporting trailer metadata in server-streaming and both-streaming. Is it OK for you?

Yes.

kazegusuri · 2016-02-17T16:11:08Z

Header metadata are mapped to HTTP headers which have Grpc-Metadata- prefix. It is supported in all of unary, client-streaming, server-streaming and both-streaming RPCs.
Trailer metadata are mapped to HTTP trailers which have Grpc-Trailer- prefix. But it is supported only in unary RPC and client-streaming RPC.

I hope all comments addressed and met the spec.
PTAL @yugui

yugui · 2016-02-18T00:28:20Z

runtime/context.go

+type serverMetadataKey struct{}
+
+// NewServerMetadataContext creates a new context with ServerMetadata
+func NewServerMetadataContext(ctx context.Context, md *ServerMetadata) context.Context {


IIUC, you no longer need to pass a *ServerMetadata. You can pass just ServerMetadata.

yugui · 2016-02-18T00:30:46Z

Thank you very much. Now the design looks good to me.
Let me add some more coding-level comments.

yugui · 2016-02-18T00:34:30Z

runtime/errors.go

-type errorBody struct {
-	Error string `json:"error"`
-	Code  int    `json:"code"`
+type ErrorBody struct {


I don't think it is a good idea to expose this type just for testing.
This struct is just a convenient way for this runtime package to construct JSON object on error. But the struct itself is not a part of API exposed for users or generated codes by this package.

kazegusuri · 2016-02-18T10:46:16Z

Addressed comments.

yugui · 2016-02-18T11:04:15Z

LGTM.

Thank you for your excellent contribution.

make available header and trailer metadata

kazegusuri · 2016-02-18T12:39:35Z

Thanks! I'm grateful for your help.

yugui reviewed Jan 26, 2016
View reviewed changes

yugui mentioned this pull request Jan 26, 2016

Header Forwarding from server. #73

Closed

kazegusuri force-pushed the metadata branch from cf5899b to 55008ed Compare January 30, 2016 08:37

kazegusuri force-pushed the metadata branch 2 times, most recently from 495fa00 to c33445c Compare January 31, 2016 08:15

kazegusuri added 4 commits February 6, 2016 00:43

get header and trailer metadata from server and set as ServerMetadata…

eae7c52

… context

add tests for metadata

8efc351

return trailer metadata in streaming rpc

ee78409

return trailer metadata in unary rpc

cb54dbd

kazegusuri force-pushed the metadata branch 2 times, most recently from 18c9848 to 6e00a64 Compare February 6, 2016 10:36

use HTTP 1.1 trailer to return grpc trailer for unary RPC

07b7358

kazegusuri force-pushed the metadata branch 2 times, most recently from 67015be to 8680a68 Compare February 17, 2016 15:52

kazegusuri added 2 commits February 18, 2016 01:05

return Header metadata for streaming

5c1ef44

test Header metadata for streaming

380a709

regenerate examples

ef85ed5

kazegusuri force-pushed the metadata branch from 8680a68 to ef85ed5 Compare February 17, 2016 16:05

yugui reviewed Feb 18, 2016
View reviewed changes

kazegusuri added 3 commits February 18, 2016 19:43

not passing pointer to context

e6a7a73

unexport struct

21081c0

regenerate examples

bf9efa4

yugui added a commit that referenced this pull request Feb 18, 2016

Merge pull request #93 from kazegusuri/metadata

b94f713

make available header and trailer metadata

yugui merged commit b94f713 into grpc-ecosystem:master Feb 18, 2016

kazegusuri deleted the metadata branch February 18, 2016 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make available header and trailer metadata #93

make available header and trailer metadata #93

kazegusuri commented Jan 25, 2016

yugui Jan 26, 2016

kazegusuri commented Jan 27, 2016

yugui commented Jan 28, 2016

kazegusuri commented Jan 30, 2016

kazegusuri commented Jan 31, 2016

kazegusuri commented Feb 6, 2016

yugui commented Feb 8, 2016

kazegusuri commented Feb 8, 2016

achew22 commented Feb 8, 2016

yugui commented Feb 8, 2016

kazegusuri commented Feb 11, 2016

yugui commented Feb 16, 2016

kazegusuri commented Feb 16, 2016

yugui commented Feb 16, 2016

kazegusuri commented Feb 17, 2016

kazegusuri commented Feb 17, 2016

yugui Feb 18, 2016

yugui commented Feb 18, 2016

yugui Feb 18, 2016

kazegusuri commented Feb 18, 2016

yugui commented Feb 18, 2016

kazegusuri commented Feb 18, 2016

make available header and trailer metadata #93

make available header and trailer metadata #93

Conversation

kazegusuri commented Jan 25, 2016

yugui Jan 26, 2016

Choose a reason for hiding this comment

kazegusuri commented Jan 27, 2016

yugui commented Jan 28, 2016

kazegusuri commented Jan 30, 2016

kazegusuri commented Jan 31, 2016

kazegusuri commented Feb 6, 2016

yugui commented Feb 8, 2016

kazegusuri commented Feb 8, 2016

achew22 commented Feb 8, 2016

yugui commented Feb 8, 2016

kazegusuri commented Feb 11, 2016

yugui commented Feb 16, 2016

kazegusuri commented Feb 16, 2016

yugui commented Feb 16, 2016

kazegusuri commented Feb 17, 2016

kazegusuri commented Feb 17, 2016

yugui Feb 18, 2016

Choose a reason for hiding this comment

yugui commented Feb 18, 2016

yugui Feb 18, 2016

Choose a reason for hiding this comment

kazegusuri commented Feb 18, 2016

yugui commented Feb 18, 2016

kazegusuri commented Feb 18, 2016