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

testing: fix goroutine leak in TestClientUpdatesParamsAfterGoAway #5024

Conversation

charlesxsh
Copy link
Contributor

@charlesxsh charlesxsh commented Dec 1, 2021

If test failed at log like following:

    clientconn_test.go:xxx: Dial(127.0.0.1:34491, _) = _, context deadline exceeded, want _, <nil>

goroutine will be blocked at

go func() {
...
     <-connected
...
}
goroutine 8 [chan receive]:
google.golang.org/grpc.TestClientUpdatesParamsAfterGoAway.func1(0xab6d80, 0xc00000c8b8, 0xc0002c0a00, 0xc000028900)
	/fuzz/target/clientconn_test.go:978 +0x30f
created by google.golang.org/grpc.TestClientUpdatesParamsAfterGoAway
	/fuzz/target/clientconn_test.go:956 +0x212

--- FAIL: TestClientUpdatesParamsAfterGoAway (5.00s)
FAIL

RELEASE NOTES: none

@dfawley dfawley added this to the 1.44 Release milestone Dec 6, 2021
@@ -659,6 +659,8 @@ func (s) TestClientUpdatesParamsAfterGoAway(t *testing.T) {
}
defer lis.Close()
connected := make(chan struct{})
Copy link
Member

Choose a reason for hiding this comment

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

This would be a little neater (taking advantage of a grpc internal feature):

connected := grpcsync.NewEvent()
defer connected.Fire()
...
<-connected.Done()
...
connected.Fire()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice fix! updated @dfawley

@github-actions
Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Dec 13, 2021
@charlesxsh charlesxsh force-pushed the shihaox/fix-TestClientUpdatesParamsAfterGoAway branch from 5719ea3 to 64cf8de Compare December 14, 2021 16:59
@dfawley
Copy link
Member

dfawley commented Dec 14, 2021

Thank you for the fix!

@dfawley dfawley assigned menghanl and unassigned charlesxsh Dec 14, 2021
@github-actions github-actions bot removed the stale label Dec 14, 2021
@menghanl menghanl changed the title fix goroutine leak in TestClientUpdatesParamsAfterGoAway testing: fix goroutine leak in TestClientUpdatesParamsAfterGoAway Dec 14, 2021
@menghanl menghanl merged commit 7c8a932 into grpc:master Dec 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants