improbable-eng / grpc-web

gRPC Web implementation for Golang and TypeScript
Apache License 2.0
4.36k stars 433 forks source link

Publish interop information with grpc/grpc-web #162

Open johanbrandhorst opened 6 years ago

johanbrandhorst commented 6 years ago

Now that github.com/grpc/grpc-web is publically available, we need clarity on how it interacts with the Improbable gRPC-web proxy, and how the Improbable grpc-web-client interacts with the grpc/grpc-web backend proxy.

Reference issue in grpc/grpc-web: https://github.com/grpc/grpc-web/issues/91

pieterlouw commented 6 years ago

@johanbrandhorst , I was thinking the same thing. In between work I'm busy installing the grpc-web client tools to see if a client stub can connect to Improbable's proxy and not the nginx proxy.

easyCZ commented 6 years ago

Good idea, we will try to spend some time to clarify this. It would awesome if you could provide any findings you may already have.

johanbrandhorst commented 6 years ago

The last testing I did in this area was about a year ago when both the projects were very green indeed ;). My problem last time was getting their proxy up and running at all, but that is probably easier now.

johanbrandhorst commented 5 years ago

FYI grpc/grpc-web recently created a PR that implies (I haven't tested it) that their client is now compatible with this repos proxy: https://github.com/grpc/grpc-web/pull/217

tiramiseb commented 5 years ago

After much testing, it seems to me that the aforementioned PR doesn't make grpc/grpc-web client compatible with improbable-eng/grpc-web.

The grpc/grpc-web client checks for the content-type header and wants it to be grpc-related https://github.com/grpc/grpc-web/pull/217/files#diff-384bd19901f3116fd3698d5db6f1ba87R129

... whereas the improbable-eng/grpc-web proxy/library adds the content-type as a trailer.

johanbrandhorst commented 5 years ago

@tiramiseb very interesting! Do you mean to say you've been testing the other client against this proxy and found it broken? I've been meaning to write some compatibility tests myself but not got around to it yet. Could you go into more detail? Do you have a test repo?

tiramiseb commented 5 years ago

We want to avoid using a proxy, so we are using the grpc-web library directly. I have quickly looked at the source code for the proxy, it looks like it is directly importing the same library, so the behaviour I have with the library should be the same with the proxy.

On the server side, we have implemented the library as explained in its README, listening on port 31337. There is no TLS involved.

On the client side, after generating the service files with the following command:

protoc -I../../rpc service.proto --js_out=import_style=commonjs:src --grpc-web_out=import_style=commonjs,mode=grpcweb:src

... I am doing something like this:

import {XXXClient} from './service_grpc_web_pb';
import {YYY} from './service_pb';
const XXXService = new XXXClient('https://localhost:31337', null, null);
[...]
var call = XXXService.feature(request, {}, function(err, response) {
  console.log("GOT IT");
});

The behaviour I am experiencing is the following:

Then I have dug through the source code of both the improbable-eng/grpc-web server and the grpc/grpc-web client.


On the server side, when grpcWebResponse.finishRequest is called (https://github.com/improbable-eng/grpc-web/blob/master/go/grpcweb/grpc_web_response.go#L68), if w.wroteHeaders || w.wroteBody is true, so w.copyTrailersToPayload() is called.

The payload received by the browser is base64-encoded, after decoding and hexdump I have:

0000000  \0  \0  \0  \0 006  \n 004   o   k   o   k 200  \0  \0  \0   6
0000010   C   o   n   t   e   n   t   -   T   y   p   e   :       a   p
0000020   p   l   i   c   a   t   i   o   n   /   g   r   p   c   +   p
0000030   r   o   t   o  \r  \n   G   r   p   c   -   S   t   a   t   u
0000040   s   :       0  \r  \n                                        
0000046

... which looks fine.

Its headers are:

HTTP/1.1 200 OK
Access-Control-Allow-Headers: Origin, Content-Type
Access-Control-Allow-Methods: GET, POST, OPTIONS
Access-Control-Allow-Origin: *
Vary: Origin
Date: Tue, 11 Sep 2018 06:43:38 GMT
Content-Type: application/octet-stream
Transfer-Encoding: chunked

On the client side, the library checks the Content-Type header only, and returns without any further action when it does not start with application.grpc (https://github.com/grpc/grpc-web/blob/e64e65626850ba263ae13fbecced0c2df75915f6/javascript/net/grpc/web/grpcwebclientreadablestream.js#L132).


I have tested and verified this behaviour by using the good ol' print-based debugging, I have cluttered both source codes with fmt.Println, spew.Dump or console.log. :)

Before leaving yesterday, I noticed - I don't remember where - that the chuncked response may only be with HTTP/1.1, so today I'll try installing an nginx server with HTTP2+TLS in front of the backend server (which will be there in our production environment anyway)...

johanbrandhorst commented 5 years ago

I think what you're seeing here is that the grpc/grpc-web client does not support server-side streaming in the way you expect it to. The Content-Type: application/octet-stream is set by the browser, there's nothing either proxy can do about that. I think grpc/grpc-web only support streaming in a narrow set of circumstances. https://github.com/grpc/grpc-web/issues/281 might be relevant.

tiramiseb commented 5 years ago

I don't plan to stream anything. I have just used the regular functions exposed by the generated code, as shown in the READMEs...

It looks like the grpc/grpc-web client implements regular calls as if they were streams: https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpcwebclientbase.js#L72

johanbrandhorst commented 5 years ago

Interesting - that makes sense since it's true for gRPC clients in general that a unary request is just a special case of a stream (with a single request and reply). I don't know if there's anything that proxy can do about that Content-Type header. I'd be interested in seeing what the grpc/grpc-web promoted Envoy proxy does in this case. If you get around to it, it'd be interesting to hear :). I will be doing compatbility tests myself eventually and I can loop back here when I found out more.

tiramiseb commented 5 years ago

I can confirm I have the same behaviour with Nginx/HTTP2/TLS in front of the grpc-web server.

johanbrandhorst commented 5 years ago

So I've set up some compatibility tests between the two implementations at https://github.com/johanbrandhorst/grpc-web-compatibility-test. It shows largely that they're both compatible with one another in the simple cases (unary and server side streaming with application/grpc-web+proto content-type). Are you still having problems?

danilvpetrov commented 5 years ago

I can confirm exactly the same behaviour as @tiramiseb describes. I am using improbable-eng/grpc-web/go/grpcweb package directly in my app with the latest grpc-web javascript client ( semver constraint "^1.0.3"). I am NOT using server-side streaming, just plain unary request.

Apparently, as @tiramiseb mentioned, when processing response from the server, grpc-web JS client checks for the Content-Type header to start with either application/grpc-web-text or application/grpc values, otherwise it skips processing the response.

If a gRPC service returns an error, then grpc.WrappedGrpcServer's ServeHTTP() method returns response with header Content-Type = application/grpc. In this case the client can correctly process the response.

I ended up placing a hack in http.Server definition to copy the value of JS client's Accept header into response's Content-Type header. It looks similar to this:

grpcSvr := grpc.NewServer()
wrapped := grpcweb.WrapServer(grpcSvr)
httpSvr := &http.Server{
        ReadTimeout: 5 * time.Second,
        WriteTimeout: 5 * time.Second,
        Handler: http.HandlerFunc(
            func(
                resp http.ResponseWriter,
                req *http.Request,
            ) {
                if wrapped.IsGrpcWebRequest(req) {
                    if ct := req.Header.Get("Accept"); ct != "" {
                        resp.Header().Set("Content-Type", ct)
                    }
                    wrapped.ServeHTTP(resp, req)
                } 
            }),
    }

// start serving httpSvr below ...

It seems to work now.

It seems like in certain cases grpc.WrappedGrpcServer loses response's Content-Type header.

johanbrandhorst commented 5 years ago

The content-type header cannot be set when streaming responses to a fetch request AFAIK. When you say "grpc-web JS client" you mean the github.com/grpc/grpc-web client? I think this is a separate issue btw, could you please open another issue?

danilvpetrov commented 5 years ago

When you say "grpc-web JS client" you mean the github.com/grpc/grpc-web client?

Yes, that's the library I am referring to, the links that I've provided refer to this very library.

The content-type header cannot be set when streaming responses to a fetch request AFAIK.

AFAIK, the sender should set adequate Content-Type header which is a requirement in current version of grpc web protocol regardless of response type (unary or streaming). I believe that's the primary reason why github.com/grpc/grpc-web client checks for the proper Content-Type header when processing a response from the server.

I think this is a separate issue btw, could you please open another issue?

No problem, I've opened the issue describing the problem with some additional findings.