grpc.WrappedGrpcServer.ServerHTTP() returns response with invalid Content-Type header

[danilvpetrov]

When wrapping grpc.Server in type grpc.WrappedGrpcServer using package github.com/improbable-eng/grpc-web/go/grpcweb, the method ServeHTTP does not set a Content-Type header either to application/grpc-web-text or application/grpc-web. This causes a javascript client using protobuf generated files with https://github.com/grpc/grpc-web package to skip processing the response from the gRPC server. As a result, a callback function on the request method or on stream data event are not getting called even though the entire request is completed successfully (in case of unary request) or ongoing (in case of server-side streaming).

This behaviour of grpc.WrappedGrpcServer is manifested both when using a unary request and server-side streaming. Also the result is the same regardless of the mode the javascript files are generated with (grpcwebtext or grpcweb).

I am using the latest https://github.com/grpc/grpc-web library (1.0.3) and the latest version of github.com/improbable-eng/grpc-web/go/grpcweb package

[johanbrandhorst]

This seems like a regression to me, last time I ran my interop tests all the requests from a grpc/grpc-web client worked with the grpcwebproxy proxy. I'll try and reproduce this in https://github.com/johanbrandhorst/grpc-web-compatibility-test.

[danilvpetrov]

Thanks for the prompt reply!

https://github.com/johanbrandhorst/grpc-web-compatibility-test repository has slightly different set up as it uses https://github.com/improbable-eng/grpc-web/go/grpcwebproxy as a compiled binary run within a Docker container. This behaviour most likely will not be revealed as gRPC-native servers will supply the correct Content-Type header when connected to the proxy. These headers then will be correctly interpreted by grpc.WrappedGrpcServer.

My assumption (and it's only assumption at this stage based on my superficial review of https://github.com/improbable-eng/grpc-web/go/grpcweb package), is that the problem occurs ONLY when using github.com/improbable-eng/grpc-web/go/grpcweb package to wrap grpc.Server instance within a single binary. In this scenario the underlying gRPC-native services possibly won't supply Content-Type header at all. When looking at this section of code it appears that grpc.WrappedGrpcServer will only interpret Content-Type header from gRPC-native services if the header is in the map originally. What if there is no Content-Type header to start with?

[johanbrandhorst]

Very interesting analysis! I guess the next question is, why wouldn't the http.Handler implementation of the gRPC servers not set the content type header? Is this potentially a bug in gRPC do you think?

[johanbrandhorst]

I was able to reproduce this in the compatibility tests by adding an in-process proxy. Very interesting. Definitely need more investigation. Have you got any closer to figuring out where the problem is? Should the gRPC ServeHTTP method be setting this header?

[danilvpetrov]

I was running the code in debugger to see where the possible problem. I can confirm on my end that gRPC service sets the header correctly. However, the method WriteHeader in grpcWebResponse never gets called by the service. This is the method in grpcWebResponse that has header preparation logic for grpc-web side. I am planning to dig more on this. IMHO when used directly the underlying gRPC service behaves differently in respect to what it calls when writing the response.

[johanbrandhorst]

Very interesting, I'm also doing some debugging now that I have an easily reproducible scenario. I'll let you know if I find anything strange!

[danilvpetrov]

Will do, thanks!

[johanbrandhorst]

Hmm, in my testing it looks like the content type is being written to the response body as an in-body trailer. Maybe that's what's causing the issue? Might need some extra handling in the wrapper.

[johanbrandhorst]

Yep, the difference is definitely that the content-type is being written to the body rather than as a header. I'll see if I can try and get it written as a header with some changes.

[danilvpetrov]

Yeah, that's consistent with what i can see in my debugging. However in my case, the Content-Type header that I can see in the trailing payload is still wrong as it is original Content-Type header from gRPC service. Looks like it has not been substituted, i.e application/grpc has not been replaced with application/grpc-web-text or application/grpc-web in case of grpcwebtext or grpcweb client's modes respectively.

[johanbrandhorst]

Indeed, but I think it's probably wrong to write it as a trailer at all. I think it's not converted because it's unexpected. I'll see if I can make some changes to the proxy to get it to write it as a proper header.

[danilvpetrov]

Yeah absolutely it should not be in trailers :). There is definitely a need for some sanitisation before writing the trailers. Since copyJustHeadersToWrapped method is not called here, the modified Content-Type header is not getting into the a HTTP/1.1 response that is supposed to go back to the grpc-web client side.

[danilvpetrov]

Thanks your support!

[johanbrandhorst]

The more I look at this the more I think the way we're wrapping the http.Headers of the response is wrong. It seems not to be able to duplicate the content-type header because the gRPC server is writing the body before calling WriteHeader, and so it's too late to write the content type header. I'll need to do some more digging though.

[danilvpetrov]

That's sort of the feeling I am getting as well. In my experience I was placing a breakpoint on this line and the process never hit it. This is the case at least for the unary request, TBH I haven't tried server-side streaming. But it looks like gRPC service code does not differentiate between a unary request and the server-side stream, it just treats the response in unary request as a stream with a single chunk - I might be wrong.

The gRPC service still calls Header() method to set up its headers directly. May be its worth using this method to place the headers straight into HTTP/1.1 response? And then separate trailing headers and substitute Content-Type before the main logic both in Write() and still for compatibility in WriteHeader(). This might break something else, but if the header sanitisation is the same as in copyJustHeadersToWrapped() it should still be ok hopefully.

Trailing headers from gRPC service then might be separated in the process of header sanitisation and written into the body in exactly the same way they are written in finishRequest().

[johanbrandhorst]

That sounds like a good plan, I've got both unary and server streaming responses in my test cases so should be able to figure it out when I get the time. Feel free to play around with the compatibility tests yourself if you want an easy test case.

[johanbrandhorst]

Kind of related; #300

[johanbrandhorst]

I've decided to try to reproduce this with the local proxy tests; that should make it easier to develop a solution.

[johanbrandhorst]

I've opened #339 with a failing test for this case. I don't have time to work on it right now or tomorrow but hopefully this can help your debugging and triaging efforts :).

[danilvpetrov]

Thanks for all these tests, I will keep debugging and let you know if I find something else.