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.

Sign up for GitHub

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

Jump to bottom

enforce to close accepted connection after processing #2883

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

enforce to close accepted connection after processing #2883

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5606c05
enforce to close accepted connection after processing
Nov 17, 2023
356da66
enforce client.Close in innerAccept
Nov 21, 2023
5c5b8eb
fix unit test
Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/go/thrift/simple_server.go
View file
Open in desktop
Expand Up @@ -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 •
edited

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 •
edited

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.

client = nil
}
case <-p.stopChan:
// TSimpleServer.Close called, close the client connection
client.Close()
Expand Down Expand Up @@ -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

p.stopChan = make(chan struct{})
return nil
}
Expand Down Expand Up @@ -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.

if errors.Is(err, net.ErrClosed) {
// In this case, it's kinda expected to get
// net.ErrClosed, treat that as no-error
Expand Down