Skip to content
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

Publish interop information with grpc/grpc-web #162

Open
johanbrandhorst opened this issue Apr 5, 2018 · 15 comments
Open

Publish interop information with grpc/grpc-web #162

johanbrandhorst opened this issue Apr 5, 2018 · 15 comments

Comments

@johanbrandhorst
Copy link
Collaborator

@johanbrandhorst johanbrandhorst commented Apr 5, 2018

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: grpc/grpc-web#91

@pieterlouw
Copy link

@pieterlouw pieterlouw commented Apr 5, 2018

@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
Copy link
Contributor

@easyCZ easyCZ commented Apr 21, 2018

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
Copy link
Collaborator Author

@johanbrandhorst johanbrandhorst commented Apr 21, 2018

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
Copy link
Collaborator Author

@johanbrandhorst johanbrandhorst commented Aug 4, 2018

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: grpc/grpc-web#217

@tiramiseb
Copy link

@tiramiseb tiramiseb commented Sep 10, 2018

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
Copy link
Collaborator Author

@johanbrandhorst johanbrandhorst commented Sep 10, 2018

@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
Copy link

@tiramiseb tiramiseb commented Sep 11, 2018

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:

  • When there is an error (I have forcefully made it so the server returns a "not implemented" error), I see "GOT IT" in the console.
  • When it should work, I see nothing at all, the callback function is not called.

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
Copy link
Collaborator Author

@johanbrandhorst johanbrandhorst commented Sep 11, 2018

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. grpc/grpc-web#281 might be relevant.

@tiramiseb
Copy link

@tiramiseb tiramiseb commented Sep 11, 2018

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
Copy link
Collaborator Author

@johanbrandhorst johanbrandhorst commented Sep 11, 2018

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
Copy link

@tiramiseb tiramiseb commented Sep 11, 2018

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

@johanbrandhorst
Copy link
Collaborator Author

@johanbrandhorst johanbrandhorst commented Oct 20, 2018

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
Copy link
Contributor

@danilvpetrov danilvpetrov commented Feb 6, 2019

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
Copy link
Collaborator Author

@johanbrandhorst johanbrandhorst commented Feb 6, 2019

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
Copy link
Contributor

@danilvpetrov danilvpetrov commented Feb 7, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants