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

enforce to close accepted connection after processing #2883

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jshen14
Copy link

@jshen14 jshen14 commented Nov 17, 2023

Enforce close of accepted connection in golang simple server to avoid memory leak

  • Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@fishy fishy added the golang patches related to go language label Nov 18, 2023
@@ -211,6 +211,10 @@ func (p *TSimpleServer) innerAccept() (int32, error) {
select {
case <-ctx.Done():
// client exited, do nothing
if client != nil {
client.Close()
Copy link
Member

Choose a reason for hiding this comment

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

can you explain more why do we need to close the client here? is there any connection leak otherwise?

Copy link
Author

@jshen14 jshen14 Nov 18, 2023

Choose a reason for hiding this comment

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

Yes, if the call to Close is not called, then any dynamically allocated data in client object is lost forever and hence the caller's golang runtime keeps eating memory.

Copy link
Member

Choose a reason for hiding this comment

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

How would that affect overall performance, e.g. when doing multiple calls?

Copy link
Author

@jshen14 jshen14 Nov 18, 2023

Choose a reason for hiding this comment

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

Could you help clarify what is your overall performance concern?

Below is my understanding of how the simpleServer works with its serverTransport regarding accept an incoming request. Please correct me if I miss anything.
A client object is created by calling the underlying serverTransport.Accept(). I think client.Close() is to signal serverTransport that thrift server has finished using this client. So serverTransport could do whatever is required on its part, either to cleanup or recycle the object for reuse.

Without the signal from thrift server, what is expected from serverTransport to manage client object created by itself? The reference to client is lost after processing the request.

I guess thrift simple server has some assumption on how serverTransport to manage client created by serverTransport itself. Could you help advise where I could find the assumption? Or help explain the choice why not to enforce close client after processing.

Thanks for helping to review.

Copy link
Member

Choose a reason for hiding this comment

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

I simply asked a question. If your answer is a validated "No it will not affect perf because ..." that's fine with me.

@@ -270,6 +274,7 @@ func (p *TSimpleServer) Stop() error {
}

<-ctx.Done()
p.stopChan = nil
Copy link
Member

Choose a reason for hiding this comment

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

this does nothing, it's immediately overwritten by the next line

@@ -356,6 +361,7 @@ func (p *TSimpleServer) processRequests(client TTransport) (err error) {
ok, err := processor.Process(ctx, inputProtocol, outputProtocol)
if errors.Is(err, ErrAbandonRequest) {
err := client.Close()
client = nil
Copy link
Member

Choose a reason for hiding this comment

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

this does not work as you expected.

here client is the local arg of processRequests function, setting it to nil only affects that copy of client. it does not affect the client you are checking on line 214, which is at the caller of processRequests.

the whole nil checking regarding client is also unnecessary, if client is already closed, closing it again will only give you an already closed error, which you ignored anyways (on line 215).

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Then is it good to have defer client.Close in processRequests()?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to defer it in innerAccept instead.

@fishy
Copy link
Member

fishy commented Nov 21, 2023

please fix the failed unit test.

@jshen14
Copy link
Author

jshen14 commented Nov 24, 2023

@fishy I updated to fix golang unit test. Now I see there is build error due to python test. I dont understand how that is related to my changes. Could you help me understand what I need to fix?

@fishy
Copy link
Member

fishy commented Jul 10, 2024

@jshen14 sorry I missed your comment last year 😓

please rebase your change on top of latest master branch and the python tests should be fixed now.

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

Successfully merging this pull request may close these issues.

3 participants