mwitkow / grpc-proxy

gRPC proxy is a Go reverse proxy that allows for rich routing of gRPC calls with minimum overhead.
Apache License 2.0
956 stars 208 forks source link

Connection leakage #15

Open vgough opened 7 years ago

vgough commented 7 years ago

Using grpc-proxy with a simple test server, I see every connection from the proxy to the endpoint leaking a file descriptor.

I've narrowed it down to 2 cases in handler.handler, both involve not closing backendConn.

  1. if the client goes away unexpectedly, grpc.NewClientStream can fail and the backendConn leaks. Need to close the backendConn in that case.

  2. seems that backendConn needs to be closed at some point. Experimentally, I found that closing backendConn after serverStream.SetTrailer eliminated the leaks I was seeing.

Without those two fixes, I see connections pile up within the proxy until it runs out of file descriptors and fails subsequent requests.

qustavo commented 6 years ago

Did you manage to work around this issue?

vgough commented 6 years ago

See linked issue (#16). I believe that connection management remains necessary. Those haven't been incorporated, so I have my own fork that contains those along with a number of other updates. You're welcome to use my repo or extract the improvements at github.com/vgough/grpc-proxy.

qustavo commented 6 years ago

Thank you so much!

tsony-tsonev commented 4 years ago

Hi @vgough your fork is unusable due to your repo is vendored which leads to two problems.

First: Now the method proxy.TransparentHandler(director) returns a type from your vendor dir and thus can not be passed as a grpc server configuration: image

Second: The proxy.StreamDirector interface can not be implemented for the same reason (the interface wants github.com/vgough/grpc-proxy/vendor/google.golang.org/grpc instead of google.golang.org/grpc as paramter to the function). If I add this import: "github.com/vgough/grpc-proxy/vendor/google.golang.org/grpc" I can implement the interface, but this type of imports are forbidden and can not be compiled.

vgough commented 4 years ago

@tsony-tsonev Thanks. I've updated my repo to use Go modules rather than the dep vendoring.

tsony-tsonev commented 4 years ago

@vgough thank for the fast reaction. I'll definitely try it out again.

PS: super @vgough it works and no memory leaks.

gakkiyomi commented 4 years ago

See linked issue (#16). I believe that connection management remains necessary. Those haven't been incorporated, so I have my own fork that contains those along with a number of other updates. You're welcome to use my repo or extract the improvements at github.com/vgough/grpc-proxy.

@vgough I have read your fork about memory leakage,but i have no idea when should i release those connections. (https://github.com/vgough/grpc-proxy/blob/master/proxy/examples_test.go).Can you provide me some examples about [ Release(ctx context.Context, conn *grpc.ClientConn)] ,thank you very much

vgough commented 4 years ago

@gakkiyomi There is more documentation in the readme: https://github.com/vgough/grpc-proxy/blob/master/proxy/README.md#type-streamdirector

I think that normally you would be implementing a director, not calling one. The handler calls Release on the director when it is done proxying a call: https://github.com/vgough/grpc-proxy/blob/master/proxy/handler.go#L71

When your code receives a Release call, it has the opportunity to release resources used by the director.

gakkiyomi commented 4 years ago

@gakkiyomi There is more documentation in the readme: https://github.com/vgough/grpc-proxy/blob/master/proxy/README.md#type-streamdirector

I think that normally you would be implementing a director, not calling one. The handler calls Release on the director when it is done proxying a call: https://github.com/vgough/grpc-proxy/blob/master/proxy/handler.go#L71

When your code receives a Release call, it has the opportunity to release resources used by the director.

@vgough I truly appreciate your timely help.

tsony-tsonev commented 4 years ago

Guys just want to inform you for another bug if you are using this on prod. Memory leakage is working good, in my case I'm just closing the connections in the release function of the Director. But couple of months ago I had problems with bidirectional streaming and had to fork the repo and fix it. The proxy is forwarding messages on the bidi stream successfully from the client to the proxy than to the backend service and on the other direction without problems. The bug occurs when the backend service is restarted then the client continues to be connected to the proxy and is thinking that it has connection with the backend. This can be seen if we set 1 sec keep alive for the client connection.

SergeyNarozhny commented 6 months ago

@tsony-tsonev How you managed to fix this bug? Closing client connection or redialing the backend?

tsony-tsonev commented 6 months ago

@SergeyNarozhny i don't remember exactly how, but you can check out our fork https://github.com/taxime-hq/grpc-proxy