Suggestion: Allow enabeling WebSocket transport, even if rpc isn't BiDi or server stream

[mame82]

As lately discussed on twiiter, I haven a use case where a server stream gRPC call is used to deliver events after registration. This ultimately ends up in a never ending server stream, which floods memory in case the DefaultTransportFactory decides to communicate based on XHR as the "unfinished" HTTP response body accumulates in memory. On the other hand, for WebSocket based transport this issue seems to be none existend.

Here's an excerpt of the relevant part of my proto

syntax = "proto3";

package P4wnP1_grpc;

service P4WNP1 {
     ... snip ...

    //Events
    rpc EventListen(EventRequest) returns (stream Event) {}
}

/* Events */
message EventRequest {
    int64 listenType = 1;
}

message EventValue {
    oneof val {
        string tstring = 1;
        bool tbool = 2;
        int64 tint64 = 3;
    }
}

message Event {
    int64 type = 1;
    repeated EventValue values = 2;
}
... snip ...

The relevant part of the go file generated with your compiler plugin for gopherjs

func (c *p4WNP1Client) EventListen(ctx context.Context, in *EventRequest, opts ...grpcweb.CallOption) (P4WNP1_EventListenClient, error) {
    srv, err := c.client.NewClientStream(ctx, false, true, "EventListen", opts...)
    if err != nil {
        return nil, err
    }

    err = srv.SendMsg(in.Marshal())
    if err != nil {
        return nil, err
    }

    return &p4WNP1EventListenClient{srv}, nil
}

As the isClientStream option in the generated source is set to false (which is correct in this context), your implementation decides to disable WebsocketTransportFactory.

This is because the responsible method newProperties() from here https://github.com/johanbrandhorst/protobuf/blob/master/grpcweb/properties.go#L36 decides on WebSocket usage based on the fact if the gRPC call uses client-streaming, here: https://github.com/johanbrandhorst/protobuf/blob/master/grpcweb/clientstream.go#L53

In order to overcome this behavior, I changed the generated gopherjs code to set isClientStreaming to true, even if the real method uses server streaming, only.

Manual change of generated code:

func (c *p4WNP1Client) EventListen(ctx context.Context, in *EventRequest, opts ...grpcweb.CallOption) (P4WNP1_EventListenClient, error) {
    srv, err := c.client.NewClientStream(ctx, true, true, "EventListen", opts...) //changed isClientStreaming to true to enable WebSocket usage
    if err != nil {
        return nil, err
    }

    err = srv.SendMsg(in.Marshal())
    if err != nil {
        return nil, err
    }

    return &p4WNP1EventListenClient{srv}, nil
}

After adding a filter criteria to the server, which redirects HTTP requests to the wrapped gRPC server (with WS support enabled), based on the presence of request header field "Sec-Websocket-Protocol: grpc-websockets like this ...

    //Wrap the server into a gRPC-web server
    grpc_web_srv := grpcweb.WrapServer(s, grpcweb.WithWebsockets(true)) //Wrap server to improbable grpc-web with websockets
    //define a handler for a HTTP web server using the gRPC-web proxy
    http_gRPC_web_handler := func(resp http.ResponseWriter, req *http.Request) {
        if strings.Contains(req.Header.Get("Content-Type"), "application/grpc") ||
            req.Method == "OPTIONS" ||
            strings.Contains(req.Header.Get("Sec-Websocket-Protocol"), "grpc-websockets") {
            fmt.Printf("gRPC-web req:\n %v\n", req)
            grpc_web_srv.ServeHTTP(resp, req) // if content type indicates grpc or REQUEST METHOD IS OPTIONS (pre-flight) serve gRPC-web
        } else {
            fmt.Printf("legacy web req:\n %v\n", req)
            http.FileServer(http.Dir((absWebRoot))).ServeHTTP(resp, req)
        }
    }

... the server-streaming gRPC call to EventListen is done via WebSocket without visible issues.

I haven't done much testing on this, especially not on memory consumption for an endless stream, compared to plain XHR or mozilla based XHR. Anyway, adding an option to the generated code, to allow gRPC calls via WebSockets seems to has its use cases, even if the RPC call isn't BiDi or client-streaming.

The ...grpcweb.CallOption parameters accepted by every client side RPC method, seems to be a feasible way to achieve this.

Maybe this could be called ....

EventListen(ctx context.Context, in *EventRequest, opts ...grpcweb.CallOption) (P4WNP1_EventListenClient, error)

... like this in the end:

... snip ...
evStream, err := Client.Client.EventListen(ctx, &pb.EventRequest{ListenType: common.EVT_LOG}, grpcweb.CallOptionUseWebsocket{true}) // <-- this would be great
... snip ...

[johanbrandhorst]

Hi @mame82! Thanks for raising this issue. I realised after our exchange that this might be necessary. I think it might be achievable with a call option but I'll have to do some testing. I agree with your assessment that using the websocket transport for server side streaming should work just fine.

If you want to try and make a contribution yourself, it should be possible to store some flag in the properties that can be set by the call option before the call is first created. Since the websocket transport should work with any type of request (even unary) I don't see a problem with it being available like that. Obviously the option should only be to turn it on for all requests, no way to disable it. Something like WithForceWebsocketTransport() might work.

[mame82]

I filed a PR https://github.com/johanbrandhorst/protobuf/pull/36

Unfortunately a test for exit-code is failing.

Anyway, it is a minor change, using a client-wide DialOption ( as you suggested turning it on for all requests, including unary).

I'd be happy if you could incorporate the changes of the PR or a similar change.

Please understand, that I'm currently missing the time to clean the PR to succeed the test. It is hard to do changes to your code in my test setup (IDE is running with different GOPATH and on a different host than my actual compiled project, thus I have to overwrite your code on two hosts for testing before putting it back to a forked github repo to fix the PR)

Failing test is git diff --exit-code but I'm not able to spot the error

[johanbrandhorst]

Great work, I will take a look tonight if I have some time!

[mame82]

Wow, your change works pretty well. Choosing WS usage per default or only per gRPC call... Like this granularity. Great work and fast adoption. Thank you, keep on your great work!