xds: Fail xDS Server Serve() if called after Stop() or GracefulStop() #6410
Conversation
xds/server_test.go
Outdated
| // Do this in a goroutine, even though should just receive error. This is to | ||
| // prevent blocking for (which comes after, so doesn't apply here), but | ||
| // mainly to wrap this err Receive operation with a testing timeout. | ||
| go func() { serveDone.Send(server.Serve(lis)) }() |
There was a problem hiding this comment.
I don't see any value in doing this in a goroutine and plumbing the returned error value through a channel for the main test goroutine to inspect. I think all we need here is a direct call to server.Serve() and inspecting the error returned from the client.
If you want to test Stop racing with Serve, you should probably do something like this:
func (s) TestXxx(t *testing.T) {
// Setup a real management server
// Setup bootstrap file to point to the management server
for i := 0 ; i < 100; i++ {
// Start an xDS enabled server.
// Spawn a goroutine which calls Serve
// Spawn a goroutine which calls Stop/GracefulStop
}
// Wait for all the above goroutines to complete using a sync.WaitGroup
// Rely on the leakchecker to detect if any goroutines are leaked (which will be the case if not all references to the real xdsClient being created are not released).
}There was a problem hiding this comment.
Ok I had this same thought. I had option 1 but wanted to wrap the operation with a timeout, since it could possibly hang if it doesn't hit the error and does the while true, and then we have to wait for the test to hit 7 minute timeout. I think the simplicity of just doing err := server.Serve beats the reason you need to wrap in a timeout. Hmmmmm ok I never thought about testing with the leak checker. Oh I didn't want to call Serve before Close in a test because I didn't want to send an LDS response so it can call into server.Server in grpc, but thinking about it you don't need a good LDS response, because it will receive from this quit here to exit: https://github.com/grpc/grpc-go/blob/master/xds/server.go#L270, and will already have refed the client conn up 1, which is the part that is interesting to test. Will switch to err := server.Serve and implement your test.
xds/server_test.go
Outdated
| } | ||
|
|
||
| // TestServeAndCloseDoNotRace tests that Serve and Close on the xDS Server do | ||
| // not race and leak the xDS Client. A race would be found by the leak checker. |
There was a problem hiding this comment.
Nit: the leak would be found by the leak checker and not the race :)
There was a problem hiding this comment.
Hahahaha fair. Switched.
Fixes #5838.
This PR fails Serve() if called after Stop() or GracefulStop(). This is important because Serve() eventually increments the global singleton xDS Client Ref, and thus if called without a corresponding cleanup, Stop() or GracefulStop(), the singleton xDS Client will eventually leak.
RELEASE NOTES: N/A