internal/idle: add a test that invokes ClientConn methods concurrently

[easwars]

PR adds a test where the ClientConn methods to enter and exit idle and invoked concurrently. Helps ensure that there is no data race in the idleness code in the ClientConn.

Also includes a minor cleanup in cc.enterIdleMode to defer the unlock.

Fixes #6560

RELEASE NOTES: none

[dfawley]

Random question: how do you decide whether to use t.Cleanup vs. defer?

1. Always use t.Cleanup?
2. Use t.Cleanup in helper functions because you have to, but use defer in test main?
3. Are there different reasons to prefer either one or the other in test main?

[easwars]

t.Cleanup and defer use separate stacks. So, it is not good to mix and match them for things where the order of execution or a guarantee of mutual exclusion is important.

We could use only defer. But that would mean that helper functions would have to return a cancel func for the main test goroutine to defer. Which can be a minor annoyance.

We could use only t.Cleanup, but that would mean that if we want something like a defer wg.Done(), that wouldn't be possible.

how do you decide whether to use t.Cleanup vs. defer?

If we use t.Cleanup in any of the helper functions, I try to stick to using t.Cleanup as much as possible in the main test goroutine as well, unless something like a defer wg.Done() (which I know will not interfere with any of the other cleanup that is happening here, and where it is not possible to use t.Cleanup). I've removed the defer on the wg.Done() calls here, since that removes the confusion. I also checked the rest of this test file, and looks like the only place I use defer is where something needs to happen at the end of function or for context cancelation. I could also probably switch the context cancelations to use t.Cleanup. Do you think that is a good idea?

[easwars]

Also, I feel that the more we can move to t.Cleanup, the better it would be. I find it cleaner in most case, especially where a helper is involved.

[dfawley]

t.Cleanup and defer use separate stacks. So, it is not good to mix and match them for things where the order of execution or a guarantee of mutual exclusion is important.

I'm not sure I follow here. All the defers in the test main will run before any of the t.Cleanups, right? I think this is well defined and easy to understand.

I was mostly wondering why use a t.Cleanup here instead of a defer, since you're in a test main. But it's interesting to consider that a helper might do a thing that needs a cleanup, and the order of that cleanup needs to happen before an earlier cleanup:

func TestFoo(t *testing.) {
	ctx, cancel := context.WithTimeout(bg, time.Second)
	defer cancel()

	someFooThatUsesCleanupAndNeedsCtx(t, ctx)
...

In this case, cancelling ctx before the t.Cleanup function runs could cause problems if the t.Cleanup expects ctx to be valid to work properly.

This is kind of ugly. Is the answer "do whatever you want as long as it works" then?? Like it seems to me you could use defers here and below instead of t.Cleanup and it would also work fine.

[easwars]

The last two messages on this thread are interesting: https://groups.google.com/a/google.com/g/go-nuts/c/r8ddcbTKRK4/m/Fl7KSwXBGgAJ

[easwars]

Switched to defers in the main test goroutine. PTAL. Thanks.

[dfawley]

Maybe a for loop around this instead of repeating? Then you can use a const for 4.

[easwars]

I was able to save only a couple of lines because I need two for loops, one for the enterIdleFunc and one for the exitIdleFunc. So, decided to leave it as is.

[dfawley]

You weren't code golfing hard enough, then!:

for _, f := range []func(){enterIdleFunc, enterIdleFunc, exitIdleFunc, exitIdleFunc} {
	go func() {
		runFunc(ctx, f)
		wg.Done
	}()
}