crypto/tls: use runtime.Gosched instead of time.After in TestCertCache

I noticed a failure of this test on a linux/amd64 builder and reproduced
it locally. I can only really reproduce it in a stress test when I
overload my system (`stress2 ./tls.test -test.run=TestCertCache`) but
this points to the root of the problem: it's possible for a timer to get
delayed and the timeout fires before we ever get the chance to check.

After copious debugging printlns, this is essentially what I'd observed.
There would only be one failed check of the reference count from before
it was updated.

Change the test to be a busy-loop again, but call runtime.Gosched. This
is also what we do for the os.Root tests, and in hindsight should've
been my go-to. This has a much higher likelihood of executing promptly.

We may want to go back and understand why the 1 ms timer would fire so
hilariously late the second time. This might be a real bug. For now,
this change makes the test more stable. It no longer fails when it's
hammered under `stress2`.

Fixes #73637.

Change-Id: I316bd9e30946f4c055e61d179c4efc5fe029c608
Reviewed-on: https://go-review.googlesource.com/c/go/+/671175
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/crypto/tls/cache_test.go b/src/crypto/tls/cache_test.go
index 66854299dfa41ac4fbff3bc8187d0f4cec9d1fb3..ea6b726d5e5da2ad87fc36d67c039908e0811410 100644 (file)
--- a/src/crypto/tls/cache_test.go
+++ b/src/crypto/tls/cache_test.go
@@ -41,22 +41,12 @@ func TestCertCache(t *testing.T) {
 
        timeoutRefCheck := func(t *testing.T, key string, count int64) {
                t.Helper()
-
-               // Explicitly check every 1 ms up to the timeout instead of busy-looping.
-               //
-               // On single-threaded platforms like js/wasm a busy-loop might
-               // never call into the scheduler for the full timeout, meaning
-               // that if we arrive here and the cleanup hasn't already run,
-               // we'll simply loop until the timeout. Busy-loops put us at the
-               // mercy of the Go scheduler, making this test fragile on some
-               // platforms.
                timeout := time.After(4 * time.Second)
-               check := time.After(1 * time.Millisecond)
                for {
                        select {
                        case <-timeout:
                                t.Fatal("timed out waiting for expected ref count")
-                       case <-check:
+                       default:
                                e, ok := cc.Load(key)
                                if !ok && count != 0 {
                                        t.Fatal("cache does not contain expected key")
@@ -68,6 +58,15 @@ func TestCertCache(t *testing.T) {
                                        return
                                }
                        }
+                       // Explicitly yield to the scheduler.
+                       //
+                       // On single-threaded platforms like js/wasm a busy-loop might
+                       // never call into the scheduler for the full timeout, meaning
+                       // that if we arrive here and the cleanup hasn't already run,
+                       // we'll simply loop until the timeout. Busy-loops put us at the
+                       // mercy of the Go scheduler, making this test fragile on some
+                       // platforms.
+                       runtime.Gosched()
                }
        }