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

Connection leakage #15

Open
vgough opened this issue Jun 15, 2017 · 12 comments
Open

Connection leakage #15

vgough opened this issue Jun 15, 2017 · 12 comments

Comments

@vgough
Copy link

vgough commented Jun 15, 2017

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
Copy link

qustavo commented May 20, 2018

Did you manage to work around this issue?

@vgough
Copy link
Author

vgough commented May 21, 2018

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
Copy link

qustavo commented May 22, 2018

Thank you so much!

@tsony-tsonev
Copy link

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
Copy link
Author

vgough commented Dec 7, 2019

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

@tsony-tsonev
Copy link

tsony-tsonev commented Dec 9, 2019

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

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

@gakkiyomi
Copy link

gakkiyomi commented Jul 14, 2020

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
Copy link
Author

vgough commented Jul 14, 2020

@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
Copy link

@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
Copy link

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
Copy link

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

@tsony-tsonev
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants