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

transport : wait for goroutines to exit before transport closes #7666

Merged
merged 17 commits into from
Oct 10, 2024
Merged

transport : wait for goroutines to exit before transport closes #7666

merged 17 commits into from
Oct 10, 2024

Conversation

eshitachandwani
Copy link
Member

@eshitachandwani eshitachandwani commented Sep 24, 2024 •
edited
Loading

Fixes: #2869

Previously, clientconn.Close() did not guarantee termination of some goroutines upon return.

Now, clientconn.Close() ensures that:

  • It waits for the readerDone channel to signal completion of reader goroutine.
  • It waits for the keepAliveDone channel to signal completion of keepAlive goroutine.
  • Transports are closed simultaneously, and clientconn.Close() waits for all transports to be closed before returning.

RELEASE NOTES:

  • transport : fixed a bug where the transport was closed before all goroutines had exited.
  • grpc : transports are closed simultaneously, and clientconn.Close() waits for all transports to be closed before returning.

@eshitachandwani eshitachandwani added this to the 1.68 Release milestone Sep 24, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 24, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.08%. Comparing base (859602c) to head (4eea9aa).
Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
internal/transport/http2_client.go 91.30% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7666      +/-   ##
==========================================
+ Coverage   81.85%   82.08%   +0.23%     
==========================================
  Files         361      362       +1     
  Lines       27822    28119     +297     
==========================================
+ Hits        22773    23081     +308     
+ Misses       3851     3841      -10     
+ Partials     1198     1197       -1
Files with missing lines Coverage Δ
clientconn.go 93.02% <100.00%> (+0.42%) ⬆️
internal/transport/http2_client.go 92.21% <91.30%> (+0.58%) ⬆️

... and 35 files with indirect coverage changes

aranjans
aranjans reviewed Sep 24, 2024
View reviewed changes
clientconn.go Show resolved Hide resolved
internal/transport/http2_client.go Show resolved Hide resolved
@eshitachandwani eshitachandwani added Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. and removed Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. labels Sep 24, 2024
@eshitachandwani eshitachandwani changed the title Transport:Waiting for go routines to exit before closing transport transport : wait for goroutines to exit before transport closes Sep 25, 2024
@aranjans
Copy link
Contributor

Thanks for addressing my earlier comments, overall change LGTM but still I feel it'd be better to add e2e style tests to verify the same.

@eshitachandwani
Copy link
Member Author

Thanks for addressing my earlier comments, overall change LGTM but still I feel it'd be better to add e2e style tests to verify the same.

Yes, I am working on the tests for it.

@eshitachandwani
Copy link
Member Author

We rely on leak check to detect if keepAlive goroutine does not exit before transport closes as it is difficult to determine when keepAlive function exits to write a test for it.

easwars
easwars reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Looks good in general. Mostly nits.

internal/transport/transport_test.go Outdated Show resolved Hide resolved
internal/transport/transport_test.go Outdated
Comment on lines 2870 to 2871
// Tests that client does not close until the reader goroutine exits and closes
// once reader goroutine returns.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Tests that client does not close until the reader goroutine exits and closes
// once reader goroutine returns.
// Tests that closing a client transport does not return until the reader goroutine exits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

internal/transport/transport_test.go Outdated
// Tests that client does not close until the reader goroutine exits and closes
// once reader goroutine returns.
func (s) TestClientCloseReturnsAfterReaderCompletes(t *testing.T) {
connectCtx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/connectCtx/ctx

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

internal/transport/transport_test.go Outdated Show resolved Hide resolved
internal/transport/transport_test.go Outdated Show resolved Hide resolved
internal/transport/transport_test.go Outdated Show resolved Hide resolved
internal/transport/transport_test.go Outdated
close(hangConn)
select {
case <-transportClosed:
case <-time.After(defaultTestTimeout):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why not use ctx.Done() here? That will ensure that the whole test has only 10s to run.

Copy link
Member Author

@eshitachandwani eshitachandwani Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This change is somehow causing a race condition between TestClientCloseReturnsEarlyWhenGoAwayWriteHangs and TestClientCloseReturnsAfterReaderCompletes

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... That is weird. Tests should be independent. Do you have an output from a failed run?

Copy link
Member Author

Choose a reason for hiding this comment

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

The run : https://github.com/grpc/grpc-go/actions/runs/11235796325/job/31234515756?pr=7666

internal/transport/transport_test.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
easwars
easwars approved these changes Oct 8, 2024
View reviewed changes
internal/transport/transport_test.go Outdated
Comment on lines 2802 to 2803
err := hc.Conn.Close()
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: single line return hc.Conn.Close()

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@easwars easwars assigned eshitachandwani and unassigned easwars and purnesh42H Oct 8, 2024
purnesh42H
purnesh42H reviewed Oct 10, 2024
View reviewed changes
internal/transport/keepalive_test.go
@@ -44,6 +44,7 @@ import (
)

const defaultTestTimeout = 10 * time.Second
const defaultTestShortTimeout = 10 * time.Millisecond
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we have a similar test to verify transport doesn't close until keepalive goroutine finish?

Copy link
Member Author

Choose a reason for hiding this comment

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

We will not test that and leave that upto the leak checker becuase determining and controlling when keepalive function closes is difficult with changing the actual function
(to add a testhook or something like that).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. okay

@purnesh42H purnesh42H self-requested a review October 10, 2024 10:04
purnesh42H
purnesh42H approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

lgtm

@purnesh42H purnesh42H merged commit b850ea5 into grpc:master Oct 10, 2024
15 checks passed
janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Oct 15, 2024
@eshitachandwani @janardhankrishna-sai
transport : wait for goroutines to exit before transport closes (grpc…
eb687d6 
…#7666)
sagarsudo pushed a commit to sagarsudo/grpc-go that referenced this pull request Oct 16, 2024
@eshitachandwani @sagarsudo
transport : wait for goroutines to exit before transport closes (grpc…
f56dc21 
…#7666)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aranjans aranjans aranjans left review comments

@purnesh42H purnesh42H purnesh42H approved these changes

@easwars easwars easwars approved these changes

Assignees

@eshitachandwani eshitachandwani

Labels
Type: Bug
Projects
None yet
Milestone
1.68 Release
Development

Successfully merging this pull request may close these issues.

ClientConn.Close does not wait for connections to be closed before returning
4 participants
@eshitachandwani @aranjans @easwars @purnesh42H