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

[Feature] multiplex over a websocket #1081

Open
borissmidt opened this issue Feb 13, 2022 · 15 comments
Open

[Feature] multiplex over a websocket #1081

borissmidt opened this issue Feb 13, 2022 · 15 comments

Comments

@borissmidt
Copy link

Dear,

I was wondering, could a websocket be multiplexed so it really mirrors the java/go api? So you create 1 connection per stub, this would allow grpc-web to have a huge amount of streams.

Maybe a simple protocol could be defined in protobuf itself to make it easy to implement in other languages to have an InProcessProxy reducing the need for proxies.

The only disadvantage is that the proxy then has to keep state to track which message maps to which stream.

Would you be interested in such a feature or would it be too complex to implement?

For the protocol i was thinking along the lines of:

syntax = "proto3";

option go_package = "./main";

message Message{
    uint32 streamId = 1;
    oneof payload {
        Start start = 2;
        Body body = 3;
        Complete complete = 4;
        Error error = 5;
        Cancel cancel = 6;
    } 
}

message Start {
    string operation = 1;
    repeated Header headers = 2;
}

message Body {
    bytes data = 1;
}

message Complete {
}

message Error {
}


message Cancel {
}

message Header {
    string name = 1;
    bytes data = 2;
}
@johanbrandhorst
Copy link
Contributor

Hi! I doubt this is something we are going to implement here, browsers are planning on implementing websockets over HTTP/2 AFAIK, which should eventually solve the maximum connection issues.

@borissmidt
Copy link
Author

how would that work then on the go side? would you get a special websocket implementation that then gives back somekind of 'virtual' websocket?

@johanbrandhorst
Copy link
Contributor

That I do not know

@borissmidt
Copy link
Author

Would it be technically feasible? (i was trying implement it but i have never written go before 😄 )

@johanbrandhorst
Copy link
Contributor

Oh I'm sure it would be, but the websocket abstraction on the backend would need to perform all the multiplexing work unless the standard library can somehow abstract that. https://datatracker.ietf.org/doc/html/rfc8441 this appears to be the RFC. coder/websocket#4 is the issue for our websocket library, which seems to depend on golang/go#32763.

@borissmidt
Copy link
Author

I will try to implement it in scala, i'm more experienced with that, and if it was easy to implement i will port it to go and the typescript client.

@borissmidt
Copy link
Author

borissmidt commented Feb 16, 2022

I tried to implement it in go but i get an
INFO[0000] streamId:1 header:{headers:{key:"Content-Type" value:"text/plain; charset=utf-8"} headers:{key:"X-Content-Type-Options" value:"nosniff"} status:500}
so i can get the headers back to the client which is already a great achievement.
https://github.com/borissmidt/grpc-web-1/blob/webscocketChannel/go/test/server_test.go

so somewhere around here it goes wrong and it gives an error but it is probably because the request doesn't have the right headers.

any suggestions how i can find out what is wrongly configured in the req which makes it fail?

@hexfusion
Copy link

I am interested in this work and will attempt to debug thank you for moving this along.

@hexfusion
Copy link

You are making good progress I ran the test through the debugger and found this. Taking a deeper look now.

gRPC requires a ResponseWriter supporting http.Flusher

ref: https://github.com/grpc/grpc-go/blob/ec717cad7395d45698b57c1df1ae36b4dbaa33dd/internal/transport/handler_server.go#L65

@hexfusion
Copy link

hexfusion commented Feb 17, 2022

Well it seems to be making better progress now that we understand the failure.

diff --git a/go/test/main.go b/go/test/main.go
index 5d893e9..65fc527 100644
--- a/go/test/main.go
+++ b/go/test/main.go
@@ -286,6 +286,8 @@ func hackIntoNormalGrpcRequest(req *http.Request) (*http.Request, bool) {
        return req, false
 }
 
+var _ http.Flusher = &GrpcStream{}
+
 type GrpcStream struct {
        id                uint32
        hasWrittenHeaders bool
@@ -352,6 +354,11 @@ func (stream *GrpcStream) Close() error {
        return nil
 }
 
+func (stream *GrpcStream) Flush() {
+       // This is most certainly not optimal :)
+}
+
time="2022-02-16T22:43:12-05:00" level=info msg="got websocket"
time="2022-02-16T22:43:12-05:00" level=info msg="accept websocket &{GET  HTTP[/1.1]() 1 1 map[Accept-Encoding:[gzip] Connection:[Upgrade] Sec-Websocket-Extensions:[permessage-deflate; client_no_context_takeover; server_no_context_takeover] Sec-Websocket-Key:[dcPgh29cRq20u2R6auJcew==] Sec-Websocket-Protocol:[grpc-websocket] Sec-Websocket-Version:[13] Upgrade:[websocket] User-Agent:[Go-http-client[/1.1]()]] {} <nil> 0 [] false 127.0.0.1:45291 map[] map[] <nil> map[] 127.0.0.1:40010  <nil> <nil> <nil> 0xc00036c2c0}"
time="2022-02-16T22:43:12-05:00" level=info msg="writing message"
time="2022-02-16T22:43:26-05:00" level=info msg="[core] Subchannel Connectivity change to CONNECTING" system=system
time="2022-02-16T22:43:26-05:00" level=info msg="Reading response"
time="2022-02-16T22:43:26-05:00" level=info msg="starting call to http server &{POST [http://localhost:50051/main.Greeter/UnaryHello]()  2 0 map[Content-Type:[application[/grpc]()+proto] Grpc-Encoding:[gzip]] 0xc00029c3c0 <nil> 0 [] false  map[] map[] <nil> map[]   <nil> <nil> <nil> 0xc0002942c0}"
time="2022-02-16T22:43:26-05:00" level=info msg="[core] Channel Connectivity change to CONNECTING" system=system
time="2022-02-16T22:43:26-05:00" level=info msg="[core] Subchannel picks a new address \"localhost:50051\" to connect" system=system
time="2022-02-16T22:43:27-05:00" level=info msg="reading from channel 1"
time="2022-02-16T22:43:27-05:00" level=info msg="[core] Subchannel Connectivity change to READY" system=system
time="2022-02-16T22:43:27-05:00" level=info msg="[core] Channel Connectivity change to READY" system=system

@borissmidt
Copy link
Author

borissmidt commented Feb 19, 2022

@hexfusion Thank you that really helped, after adding the framing like typescript,
i was able to make the requests work (streaming and unary calls).

Currently i have added the 'websocket channel' into the go proxy.

My todo list still is:

  • tests to see if the resources are cleaned up correct (its my first time with go so i might do some weird things).
  • typescript transporter
  • ping/pong for the websocket.
  • integration test, i might need some help with that later on.
  • buffer size settings.
  • backpressure in the protocl since if a client start pushing on 1 stream a lot of data and the server doesn't read it then all streams get stuck. (but is this done with any of the other features?)
  • (extra but might be better for another repo, support in java to avoid using a proxy)

@hexfusion
Copy link

ok great, I would like to try and help move the todo forward how can I collab? on your fork?

@borissmidt
Copy link
Author

borissmidt commented Feb 19, 2022 via email

@benedictjohannes
Copy link

@johanbrandhorst I was exploring this library in hope of writing a webapp (for browsers) using gRPC. Since gRPC uses a single multiplexed HTTP2 connection, when WebSocket can be multiplexed, I'd say not multiplexing websocket highly reduce the efficiency of the library (due to WS handshakes for each new function call).

@borissmidt I might not be good enough to help out, but how's your progress? I observed that your branch change (client side) exports. I'd suggest exporting another Transport, say, as MultiplexedWebSocket instead of replacing CrossBrowserHttpTransport would look better for backward-compatibility, unless of course you're planning to create a separate package altogether.

@borissmidt
Copy link
Author

I'm planning to go to a separate package to only support the 'grpc-websocket' protocol (instead of grpc-web) to make the scope of the package more clear and significantly reduce the maintenance burden.

At the moment i have not found a lot of time to continue this project, but i would really be glad with some help on the front of integration tests. So currently i have a go server which serves 'grpc-webscoket' and i implemented the typscript client.

The current working branch is here.

I got stuck at implementing integration tests, because of my lack of skill in typescript, frontend code and NodeJS.

Open issues improvements are:

  • Fair multiplexing on the websocket. I.e. have a queue per stream and round robin the messages of each stream into the websocket buffer.
  • Java client for grpc-websocket to be able to skip any proxy for the JDK.
  • Compatiblity with grpc-web and google/grpc-web to make it easy to drop into both frameworks.

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

No branches or pull requests

4 participants