xdsclient: switch xdsclient watch deadlock test to e2e style #5697
Conversation
easwars
force-pushed
the
watch_deadlock_test
branch
from
4656275
to
651dc63
Compare
easwars
force-pushed
the
watch_deadlock_test
branch
from
651dc63
to
654219b
Compare
|
Ping @dfawley |
internal/testutils/xds/e2e/server.go
Outdated
| wait := true | ||
| if opts != nil { | ||
| wait = !opts.AllowResourceSubset | ||
| } |
There was a problem hiding this comment.
wait := opts == nil || !opts.AllowResourceSubset
Or can we make this a required parameter (i.e. not a pointer) to simplify?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good point. Moved these to use t.Cleanup() and they look much nicer now.
Filed an issue to track this. |
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