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

[Work in progress] Webscocket channel, Do not merge. #1084

Closed
wants to merge 23 commits into from

Conversation

borissmidt
Copy link

@borissmidt borissmidt commented Feb 19, 2022

Changes

adds support to use a websocket like a managed channel/connection and reuse it for all requests.
#1081

Todo:

  • Add websocketChannel to the go proxy
  • Test the websocketChannel proxy, to verify that there is no resource leak.
  • Backpressure between proxy and client (nice to have)
  • Java proxy implementation (probably for another repo)
  • Add integration tests.
  • Typescript: Add websocketChannel to the typescript client.
  • Typescript: Channel recovery. (how is this done in http2)
  • Cleanup commits
  • max number of streams setting per websocket channel
  • ???

The code currently has been tested at this commit. More low level tests have to be added.

@improbable-prow-robot improbable-prow-robot added the size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. label Feb 19, 2022
Copy link

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few notes on small nits I know you're this working n this. Going to implement this and test it tonight/tomorrow.

@@ -126,6 +127,16 @@ func WithWebsockets(enableWebsockets bool) Option {
}
}

// WithWebsocketsChannel allows for handling grpc-web requests of websockets charing a single websocket between requests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: s/charing/sharing

// WithWebsocketsChannel allows for handling grpc-web requests of websockets charing a single websocket between requests
// - enabling bidirectional requests.
//
// The default behaviour is false, i.e. to disallow websockets channels

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: 'Disabled by default.'

@@ -44,6 +44,8 @@ var (
websocketPingInterval = pflag.Duration("websocket_ping_interval", 0, "whether to use websocket keepalive pinging. Only used when using websockets. Configured interval must be >= 1s.")
websocketReadLimit = pflag.Int64("websocket_read_limit", 0, "sets the maximum message read limit on the underlying websocket. The default message read limit is 32769 bytes.")

useWebsocketsChannel = pflag.Bool("use_websockets_channels", false, "whether to use a chanllel over websocket transport layer (alpha)")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: s/chanllel/channel


ctx, cancelFunc := context.WithCancel(req.Context())
defer cancelFunc()
NewWebsocketChannel(wsConn, w.handler, ctx)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to show were this went :)

go/grpcweb/wrapper.go Outdated Show resolved Hide resolved
c.Close(websocket.StatusNormalClosure, "")
}

func CompleteGrpc(ws *websocket.Conn, streamId uint32, ctx context.Context) error {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the stream id managed? I might of missed that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client will give streamid's back and has to manage it properly.
I would use an incremental id and assume that you don't do 4g requests.
A max number of streams per channel might be useful as protection.

boris added 2 commits February 20, 2022 15:25
Add multiple strings to the header package
@borissmidt
Copy link
Author

It seems that yesterday i forgot to commit a lot of files, i now added the typescript code but i sitl have to add the integration test.

@borissmidt
Copy link
Author

borissmidt commented Feb 20, 2022

I really get stuck at the integration tests, i want to analyse what goes wrong, but there are no nice stack traces because of the webpack build.
I also have a lot of trouble to keep the build consistently working when i make some changes to the libary and then want to rerun the tests. (npm link ../client/web-grpc doesn't seem to work properly.)

I think the channel never opens and that is why the tests fail but i cannot debug it which makes the further development very hard.

Maybe someone else spots the error.

@johanbrandhorst
Copy link
Contributor

Hi Boris,

Sorry that you went through all this work, but I honestly don't think we can accept this large of a contribution to the code base at this stage. This code is mostly in maintenance mode and I'm not confident that we'd be able to maintain this new addition. It appears you're run into some trouble with our tests, and I'm sorry about that, but unfortunately they're one of the problems with the current code base.

@borissmidt
Copy link
Author

borissmidt commented Feb 21, 2022

Ah to bad, atleast i learned some typescript and go on these rainy days.
I still believe that this would solve some of the grpc-web problems and increase the support on the browsers.
Maybe i better start a fork and name it 'grpc-websocket' because i'm not sure that grpc/grpc-web are not open to websocket implementations.

@borissmidt
Copy link
Author

The last test run i had, it seems that it works:
logging.log

@johanbrandhorst
Copy link
Contributor

You are of course welcome to fork the project and do your own development as long as you abide by the license: https://github.com/improbable-eng/grpc-web/blob/master/LICENSE.txt

@johanbrandhorst
Copy link
Contributor

I'm going to close this PR, since we have no intention of landing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 600+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants