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

xdsclient/e2e_test: use SendContext() where appropriate #5729

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Oct 18, 2022

This got missed in the previous PR somehow. We need to use SendContext() instead of Send() in cases where we expect the resource be NACK'ed because in these cases the management server will continuously keep resending the same resource again and again. Using SendContext() in the watch callback ensures that the send operation gets unblocked when the context is cancelled as part of test termination. If we use Send() instead, this could sometimes lead to a leaked goroutine as the watch callback is blocked on the send operation.

FWIW, this is already done in the lds_watchers_test.go which is merged.

RELEASE NOTES: n/a

@easwars easwars requested a review from zasweq October 18, 2022 19:27
@easwars easwars added this to the 1.51 Release milestone Oct 18, 2022
@easwars
Copy link
Contributor Author

easwars commented Oct 27, 2022

Ping @zasweq

Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM!

@zasweq zasweq assigned easwars and unassigned zasweq Nov 1, 2022
@easwars easwars merged commit 040b795 into grpc:master Nov 2, 2022
@easwars easwars deleted the cds_watchers_test_sendContext branch November 2, 2022 00:08
jronak pushed a commit to jronak/grpc-go that referenced this pull request Nov 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023
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.

2 participants