search

tmc / grpc-websocket-proxy

A proxy to transparently upgrade grpc-gateway streaming endpoints to use websockets
MIT License
553 stars 72 forks source link

Fix cancelling request when websocket request is cancelled. #23

Closed brocaar closed 3 years ago

brocaar commented 4 years ago

I believe this fixes #22.

I believe the WebSocket r.Context() should be passed to the newly created request object. When the WebSocket client disconnects, r.Context().Done() is fired. Without passing this context to the newly created request, the wrapped handler is not aware when a disconnect happens (see #22).

brocaar commented 4 years ago

@tmc could you review this change?

tmc commented 4 years ago

@brocaar This looks like a good change -- do you think you could get a test case in place to demonstrate/exercise this?

s00500 commented 3 years ago

Hey everyone, just stumbled into the same problem and can confirm that this changes is the cure ! It can be tested quite easily by building a grpc stream that will not close itself and then connecting with a ws client. By monitoring the goroutine counter during a disconnect of the ws client you can clearly see the serve function not getting canceled and creating a leak. Passing the context to the request in the first place solves that Greetings, Lukas

tmc commented 3 years ago

@s00500 do you think you could assist in contributing a test case for this? Would love to get it merged!

s00500 commented 3 years ago

@tmc what is blocking the merge ?

tmc commented 3 years ago

A test demonstrating the problem. I’ll go ahead and merge this but I prefer test cases.