Added experimental Websocket transport #137

MarcusLongmuir · 2018-02-07T12:23:12Z

This PR adds a Websocket transport that must be explicitly used via grpc.WebsocketTransportFactory (default transport factory does not include Websockets):

transport: grpc.WebsocketTransportFactory

The server compatibility with Websockets is also added behind an option:

websocketOriginFunc := grpcweb.WithWebsocketOriginFunc(func(req *http.Request) bool {
	return true
})
wrappedServer := grpcweb.WrapServer(grpcServer, grpcweb.WithWebsockets(true), websocketOriginFunc)

This is currently experimental as it has yet to be used in any production environment.

MarcusLongmuir · 2018-02-07T12:23:55Z

go/grpcweb/websocket_wrapper.go

+		if strings.ToLower(k) == "trailer" {
+			continue
+		}
+		//// Skip existing headers that were already sent.


MarcusLongmuir · 2018-02-07T12:24:45Z

go/grpcweb/websocket_wrapper.go

+// First byte of a binary WebSocket frame is used for control flow:
+// 0 = Data
+// 1 = End of client send
+func (w *WebSocketWrappedReader) Read(p []byte) (int, error) {


Here's the interesting bit

johanbrandhorst · 2018-02-07T18:07:41Z

go/grpcweb/wrapper.go

 	"google.golang.org/grpc"
+	"google.golang.org/grpc/grpclog"
+	"net/url"


This import should be with the other standard library imports

johanbrandhorst · 2018-02-07T18:08:03Z

go/grpcweb/websocket_wrapper.go

+	"bufio"
+	"bytes"
+	"encoding/binary"
+	"github.com/gorilla/websocket"


This import should not be with the standard library imports

johanbrandhorst · 2018-02-07T18:11:56Z

go/grpcweb/wrapper.go

+				return
+			}
+		}
+		resp.WriteHeader(400)


StatusBadRequest? Doesn't StatusForbidden (403) make more sense here? 403 instead of 401 because it's Forbidden from this Origin and not a matter of authentication.

Agreed, 403 is better.

johanbrandhorst · 2018-02-07T18:17:11Z

go/grpcweb/wrapper.go

+
+	wrappedReader := NewWebsocketWrappedReader(wsConn, respWriter)
+
+	req.Body = wrappedReader


This is mind blowing. Well done!

johanbrandhorst · 2018-02-16T08:09:48Z

What's the status of this? Doesn't look too bad to me but no updates for a week and a half.

easyCZ · 2018-02-16T12:06:51Z

I'm looking to do a review and some testing with this over the weekend. No concrete ETA on this yet.

johanbrandhorst · 2018-02-16T16:29:14Z

Ok cool, I'll probably start integrating it into downstream (my gopherjs library) based on the contents of this branch, it's a much cleverer implementation than my current websocket proxy. Just excited to see this in here :).

abemedia · 2018-02-16T18:10:03Z

Awesome work done here! Correct me if I'm wrong but for this to be useful the grpc-web typescript packages would need updating to support websockets too, right?

easyCZ

First pass review looks good. Nice job @MarcusLongmuir. Gonna try to use it as a user on a sample app before approving.

easyCZ · 2018-02-18T15:06:08Z

go/grpcweb/wrapper.go

@@ -41,11 +46,25 @@ func WrapServer(server *grpc.Server, options ...Option) *WrappedGrpcServer {
 		AllowCredentials: true,                                // always allow credentials, otherwise :authorization headers won't work
 		MaxAge:           int(10 * time.Minute / time.Second), // make sure pre-flights don't happen too often (every 5s for Chromium :( )
 	})
+	websocketOriginFunc := opts.websocketOriginFunc
+	if websocketOriginFunc == nil {
+		websocketOriginFunc = func(req *http.Request) bool {


Could we pull this into a separate function? Something like defaultWebsocketOriginFunc

easyCZ · 2018-02-18T15:11:41Z

go/grpcweb/wrapper.go

+				return
+			}
+		}
+		resp.WriteHeader(400)


Agreed, 403 is better.

easyCZ · 2018-02-18T15:19:10Z

test/ts/src/client.spec.ts

@@ -525,6 +528,181 @@ describe(`client`, () => {
        });
      });
    });
+
+    if (!process.env.DISABLE_WEBSOCKET_TESTS) {
+      describe("client-streaming (websockets)", () => {


Would it make sense to pull this into a separate file?

johanbrandhorst · 2018-03-04T21:48:46Z

Tested this in my downstream projects, works well :).

johanbrandhorst · 2018-03-06T08:11:24Z

ts/src/transports/Transport.ts

@@ -59,3 +60,7 @@ function detectTransport(): TransportConstructor {

  throw new Error("No suitable transport found for gRPC-Web");


Any chance we could add the WebsocketTransport to auto-detected transports?

I'd like to separate the implementation of the transport (this PR) from a discussion around how this should be used.

Currently the default transport factory just chooses the transport through a fallback process, but Websockets aren't necessarily the first in that list of transports to fallback through as they have some potentially undesirable tradeoffs.

I'd be happy to look at a PR that proposes a good standard way of using this transport after this lands 👍

johanbrandhorst · 2018-03-06T08:13:35Z

https://github.com/improbable-eng/grpc-web/pull/137/files#diff-d81ccdf67913238d5ac6fce3cb4ed098R34 this isn't true anymore

johanbrandhorst · 2018-03-06T08:16:09Z

go/grpcweb/options.go

+// WithWebsockets allows for handling grpc-web requests of websockets - enabling bidirectional requests.
+//
+// The default behaviour is false, i.e. to disallow websockets
+func WithWebsockets(enableWebsockets bool) Option {


I'd be keen to just turn this on, it's not a backwards compatibility breaking change, and unless my suggestion to add Websockets to DetectTransport is implemented it won't change anything.

johanbrandhorst · 2018-03-11T13:00:24Z

I've discovered a bit of an inconsistency with regards to headers:

When using the MozXHR transport, the headers received look like this (both in the case of unary and server streaming calls):

rawOnHeaders {
    headersMap: {
        "content-type": Array [ "application/octet-stream" ]
    }
}

When using the websocket transport, the headers received look like this:

rawOnHeaders {
    headersMap: {
        "content-type": Array [ "application/grpc" ]
        trailer: Array(3) [ "Grpc-Status", "Grpc-Message", "Grpc-Status-Details-Bin" ]
    }
}

So the curious thing to me is that we see the trailer in rawOnHeaders. Not sure we can or should do anything about it, but would be cool to hear your thoughts on this.

This is when using a Go gRPC Server as the backend.

johanbrandhorst · 2018-03-17T19:44:25Z

Disregard my previous comment, it's not particularly interesting. I've tested this pretty extensively and would be keen to see it merged sooner rather than later :).

DieMyst · 2018-04-10T10:28:38Z

hello. What is about this PR? No updates for 10 days. Can I use this branch for prototyping for now?

johanbrandhorst · 2018-04-10T11:27:41Z

@MarcusLongmuir has been pushing things recently, so I remain hopeful this will be merged soon :)

MarcusLongmuir · 2018-04-10T15:03:20Z

@johanbrandhorst + @DieMyst: Apologies for delay. Testing this has presented some issues - Browserstack's testing tunnel, websockets and self-signed certs have taken some effort to root cause and overcome. Hoping to get this merged ASAP.

… to avoid failOnEmptyTestSuite usage

johanbrandhorst · 2018-04-10T19:00:30Z

Thanks for this, super excited to expose it in my downstream package :)

DieMyst · 2018-04-11T08:13:59Z

Good news! Big thanks @MarcusLongmuir

MarcusLongmuir requested review from easyCZ and mwitkow February 7, 2018 12:23

MarcusLongmuir commented Feb 7, 2018

View reviewed changes

johanbrandhorst reviewed Feb 7, 2018

View reviewed changes

easyCZ mentioned this pull request Feb 16, 2018

Websockets ? #94

Closed

easyCZ reviewed Feb 18, 2018

View reviewed changes

easyCZ mentioned this pull request Feb 18, 2018

Websocket transport #85

Closed

anxiousmodernman mentioned this pull request Feb 23, 2018

Use grpc-web's native Websocket transport anxiousmodernman/co-chair#8

Open

RobIsHere mentioned this pull request Feb 27, 2018

Support running grpc-web in React Native #141

Open

johanbrandhorst reviewed Mar 6, 2018

View reviewed changes

johanbrandhorst mentioned this pull request Mar 21, 2018

Migrate to using upstream websocket proxy johanbrandhorst/protobuf#28

Merged

MarcusLongmuir mentioned this pull request Mar 28, 2018

Split up suite runs across Browserstack sessions #154

Merged

MarcusLongmuir added 5 commits April 9, 2018 13:24

Added experimental websockets

86f0e0c

Fixed websocket url construction

be6f1e7

Split out websocket tests

3e1ece2

Changed websocket origin check status code

e10af67

Cleaned up websocket wrapper

50b9f5c

Avoiding resending headers in trailers frame

1974458

MarcusLongmuir force-pushed the client-websockets branch from 8495203 to 4ff05c1 Compare April 9, 2018 12:30

Lint fixes

fce5c0f

MarcusLongmuir force-pushed the client-websockets branch from 4ff05c1 to fce5c0f Compare April 9, 2018 12:57

MarcusLongmuir added 2 commits April 9, 2018 14:24

Doc changes

71ba716

TS lint

ee7ec9b

MarcusLongmuir force-pushed the client-websockets branch from 56ad3d0 to ee7ec9b Compare April 9, 2018 14:11

Test linting

4cea460

MarcusLongmuir mentioned this pull request Apr 10, 2018

Safari + IE11 don't trust self-signed certs for wss:// #165

Open

Disabled websocket tests for browsers that don't support self-signed wss

1304cd9

Added no op test for skipping websocket suite in unsupported browsers…

e39fd36

… to avoid failOnEmptyTestSuite usage

MarcusLongmuir merged commit 6fb683f into improbable-eng:master Apr 10, 2018

MarcusLongmuir deleted the client-websockets branch April 10, 2018 15:54

jonny-improbable mentioned this pull request Apr 11, 2018

Change visibility of new GRPC WebSocket functions. #166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added experimental Websocket transport #137

Added experimental Websocket transport #137

MarcusLongmuir commented Feb 7, 2018 •

edited

Loading

MarcusLongmuir Feb 7, 2018

MarcusLongmuir Feb 7, 2018

johanbrandhorst Feb 7, 2018

johanbrandhorst Feb 7, 2018

johanbrandhorst Feb 7, 2018

easyCZ Feb 18, 2018

johanbrandhorst Feb 7, 2018

johanbrandhorst commented Feb 16, 2018

easyCZ commented Feb 16, 2018

johanbrandhorst commented Feb 16, 2018

abemedia commented Feb 16, 2018

easyCZ left a comment

easyCZ Feb 18, 2018

easyCZ Feb 18, 2018

easyCZ Feb 18, 2018

johanbrandhorst commented Mar 4, 2018

johanbrandhorst Mar 6, 2018

MarcusLongmuir Mar 6, 2018 •

edited

Loading

johanbrandhorst commented Mar 6, 2018

johanbrandhorst Mar 6, 2018

johanbrandhorst commented Mar 11, 2018 •

edited

Loading

johanbrandhorst commented Mar 17, 2018

DieMyst commented Apr 10, 2018

johanbrandhorst commented Apr 10, 2018

MarcusLongmuir commented Apr 10, 2018

johanbrandhorst commented Apr 10, 2018

DieMyst commented Apr 11, 2018


		wrappedReader := NewWebsocketWrappedReader(wsConn, respWriter)

		req.Body = wrappedReader

		@@ -59,3 +60,7 @@ function detectTransport(): TransportConstructor {

		throw new Error("No suitable transport found for gRPC-Web");

Added experimental Websocket transport #137

Added experimental Websocket transport #137

Conversation

MarcusLongmuir commented Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst commented Feb 16, 2018

easyCZ commented Feb 16, 2018

johanbrandhorst commented Feb 16, 2018

abemedia commented Feb 16, 2018

easyCZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst commented Mar 4, 2018

Choose a reason for hiding this comment

MarcusLongmuir Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

johanbrandhorst commented Mar 6, 2018

Choose a reason for hiding this comment

johanbrandhorst commented Mar 11, 2018 • edited Loading

johanbrandhorst commented Mar 17, 2018

DieMyst commented Apr 10, 2018

johanbrandhorst commented Apr 10, 2018

MarcusLongmuir commented Apr 10, 2018

johanbrandhorst commented Apr 10, 2018

DieMyst commented Apr 11, 2018

MarcusLongmuir commented Feb 7, 2018 •

edited

Loading

MarcusLongmuir Mar 6, 2018 •

edited

Loading

johanbrandhorst commented Mar 11, 2018 •

edited

Loading