search

improbable-eng / grpc-web

gRPC Web implementation for Golang and TypeScript
Apache License 2.0
4.37k stars 435 forks source link

`grpcwebproxy --use_websockets` doesn't close the streaming connection #543

Open sysint64 opened 5 years ago

sysint64 commented 5 years ago

Versions of relevant software used 67a6f66d8aa73943422a189214e254f5d50be8e6

What happened

Hello, when I use grpcwebproxy without web sockets, everything works fine, and the connection closes when the client closes connection (e.g. closes the tab or closes the browser), but when I try to use web sockets - the proxy holds the connection forever, even after closing the browser.

Environment:

jonny-improbable commented 5 years ago

Thanks for raising; please can you clarify the behavior you are observing - did you mean to say the server keep the connection open after the client closes/disconnects from the socket?

sysint64 commented 5 years ago

did you mean to say the server keep the connection open after the client closes/disconnects from the socket?

Exactly.

I use vertx as a backend, and usually I get exception CANCELLED when the client (or the proxy) closes the connection. I'm not sure if it's a grpcwebproxy problem, but I expect that the proxy will close the connection if the web client closes. Usually the proxy is logging about finishing connections, but with sockets it won't.

This is an example of log without web sockets:

INFO[0000] parsed scheme: ""                             system=system
INFO[0000] scheme "" not registered, fallback to default scheme  system=system
INFO[0000] ccResolverWrapper: sending update to cc: {[{localhost:7001 0  <nil>}] }  system=system
INFO[0000] ClientConn switching balancer to "pick_first"  system=system
INFO[0000] pickfirstBalancer: HandleSubConnStateChange: 0xc4200581a0, CONNECTING  system=system
INFO[0000] listening for http on: [::]:8080             
INFO[0000] pickfirstBalancer: HandleSubConnStateChange: 0xc4200581a0, READY  system=system
INFO[0029] finished streaming call with code Unauthenticated  error="rpc error: code = Unauthenticated desc = Not enough or too many segments" grpc.code=Unauthenticated grpc.method=GetAccountInfo grpc.service=core.Core grpc.start_time="2019-09-05T16:04:16+07:00" grpc.time_ms=7.433 span.kind=server system=grpc
INFO[0029] finished streaming call with code OK          grpc.code=OK grpc.method=Count grpc.service=auction.Auction grpc.start_time="2019-09-05T16:04:16+07:00" grpc.time_ms=5.731 span.kind=server system=grpc
INFO[0032] finished streaming call with code Canceled    error="rpc error: code = Canceled desc = context canceled" grpc.code=Canceled grpc.method=Bids grpc.service=auction.Auction grpc.start_time="2019-09-05T16:04:16+07:00" grpc.time_ms=3056.587 span.kind=server system=grpc

This is an example of log with web sockets:

INFO[0000] parsed scheme: ""                             system=system
INFO[0000] scheme "" not registered, fallback to default scheme  system=system
INFO[0000] ccResolverWrapper: sending update to cc: {[{localhost:7001 0  <nil>}] }  system=system
INFO[0000] ClientConn switching balancer to "pick_first"  system=system
INFO[0000] using websockets                             
INFO[0000] pickfirstBalancer: HandleSubConnStateChange: 0xc420058490, CONNECTING  system=system
INFO[0000] listening for http on: [::]:8080             
INFO[0000] pickfirstBalancer: HandleSubConnStateChange: 0xc420058490, READY  system=system
INFO[0008] finished streaming call with code Unauthenticated  error="rpc error: code = Unauthenticated desc = Not enough or too many segments" grpc.code=Unauthenticated grpc.method=GetAccountInfo grpc.service=core.Core grpc.start_time="2019-09-05T16:02:11+07:00" grpc.time_ms=35.063 span.kind=server system=grpc
INFO[0008] finished streaming call with code OK          grpc.code=OK grpc.method=Count grpc.service=auction.Auction grpc.start_time="2019-09-05T16:02:11+07:00" grpc.time_ms=2.453 span.kind=server system=grpc

So without sockets, I have an extra log entry after cancellation:

INFO[0032] finished streaming call with code Canceled    error="rpc error: code = Canceled desc = context canceled" grpc.code=Canceled grpc.method=Bids grpc.service=auction.Auction grpc.start_time="2019-09-05T16:04:16+07:00" grpc.time_ms=3056.587 span.kind=server system=grpc
Virtual-felix commented 4 years ago

I have the exact same issue. Did you find any solution or work around ?

sysint64 commented 4 years ago

@Virtual-felix no, I haven't, I've just switched to http2.

voortwis commented 4 years ago

Ran into the same issue sadly we really need streaming connections between browser and grpc-service. HTTP2 is not an option. Any way I can help?

Virtual-felix commented 4 years ago

Yeah me too, I need to fix this. I read that the browser is suppose to send a close event on the web socket when it closes of refreshs the page. So I will dig into the go code to see if it receives it. EDIT: In fact it seems that when the proxy establish a connection with a web socket, it only read what comes from the server side and forward it to the front end, but it doesn't read what comes from the front end (as bi-di isn't supported), so it can't know when the websocket is closed from the frontend side. I'm trying to find how to make a simple goroutine that will read it and just wait for a closed event coming from the frontend to close everything

voortwis commented 4 years ago

Hi, I forked to get some debug info in my console (no GO knowledge yet). As far as I can see the code does not handle onClose events - was planning on handling this on the underlying onCLose(...) handler. @Virtual-felix currently in meeting, not able to work on it today.

johanbrandhorst commented 4 years ago

Thanks for looking at this guys - I'd be happy to help review any PR you can come up with that finds a solution.

Virtual-felix commented 4 years ago

I think I found a solution. I'm trying to implement it the best way I can (but as I'm not familliar with the code base maybe some of you will be able to suggest me a better way to do it). I will open a PR when I'm done

voortwis commented 4 years ago

@Virtual-felix did you create a fork that you are working on? Can I be of any help?

Virtual-felix commented 4 years ago

Not yet, I will do it this afternoon or tomorrow morning at the latest. But I don't think help will be needed; the solution should be very trivial But thanks for asking, if I need I will keep you updated

Virtual-felix commented 4 years ago

It took me some time to get the whole thing. To be honnest I'm not 100% confident with my fix, maybe there are case that I didn't see. If the PR description isn't clear please tell me. But with this, my stream are well closed when the client refresh or close its tab.

voortwis commented 4 years ago

Hi @Virtual-felix, thnx for the update, I tried your change (returning nil) in my branch, This indeed closes the connection. In your usecase are you sending a continuous stream of data until done? We only send very sparse events over the stream, most of the time silence. What I see now is a connection that closes event without the browsertab closing. I think it sadly needs more work. When I have some time will look into it too. If your usecase (sparse events) is the same I will first look at our infrastructure. The thing we are testing on is containerized wrapped with istio sidecars, although that should be of no influence.

Virtual-felix commented 4 years ago

Hi, we do both, we have several stream opened and more than a half of them sends data every 2 seconds while the other are silent and only sends thing on model updates (which make them most of the time silent too). I will make more test on my side too, also the jarvis-ci did not pass which could answers some question (I didn' t look at the specific test that don't pass yet). On my side, the proxy and the back end are in different docker container, and I run the frontend locally with CRA (React), but I also have a docker version of it that I will also test now.

EDIT: On my side my streams stay open infinitely without any issue. But I only use unidirectional stream server to client. On the jarvis-ci it is client to server and bidirectional streams that fails, I thought it was not implemented :thinking: