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: switch xdsclient watch deadlock test to e2e style #5697

Merged
merged 4 commits into from Nov 4, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Oct 6, 2022

Improve a test which verifies that the xdsclient does not deadlock if a new watch is called inline from a watch callback, by moving to an e2e style test which verifies functionality without relying on any internal state.

This PR is similar to how the LDS watchers test was cleaned up in #5506.

#resource-agnostic-xdsclient-api

RELEASE NOTES: n/a

@easwars
Copy link
Contributor Author

easwars commented Oct 27, 2022

Ping @dfawley

wait := true
if opts != nil {
wait = !opts.AllowResourceSubset
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wait := opts == nil || !opts.AllowResourceSubset

Or can we make this a required parameter (i.e. not a pointer) to simplify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the callsites to StartManagementServer are happy with the defaults and therefore they dont pass anything to this function. So, making this not a pointer will change most callsites from passing nil to an empty struct.

I changed it to the single line alternative you mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not used extensively, it might be worth changing before it's used even more often. nil checks can make the code more complicated and there isn't much benefit to passing a pointer instead of a value here.

if rdsCancel2 != nil {
rdsCancel2()
}
if rdsCancel3 != nil {
rdsCancel3()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this not race with the setting of these variables (in the event of an error)? I.e. if you make watch #2 on rdsNameNewStyle instead (or expect it to match the wrong thing), maybe the race detector will fire because rdsCancel3 could be set while you're running this defer after the Fatal.

Do we even need this defer? Can we cancel the watch inside the callback instead after doing the Send?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Moved these to use t.Cleanup() and they look much nicer now.

@dfawley dfawley assigned easwars and unassigned dfawley Nov 3, 2022
@easwars easwars assigned dfawley and unassigned easwars Nov 4, 2022
dfawley
dfawley approved these changes Nov 4, 2022
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

There are 2 other nil checks in that function already that could also be simplified.

@easwars
Copy link
Contributor Author

easwars commented Nov 4, 2022

There are 2 other nil checks in that function already that could also be simplified.

Filed an issue to track this.

@easwars easwars merged commit 7f23df0 into grpc:master Nov 4, 2022
11 checks passed
@easwars easwars deleted the watch_deadlock_test branch November 4, 2022 22:13
jronak pushed a commit to jronak/grpc-go that referenced this pull request Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants