Skip to content
Snippets Groups Projects
Commit f4e37b8a authored by Michael Anthony Knyszek's avatar Michael Anthony Knyszek Committed by Gopher Robot
Browse files

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: default avatarMichael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
parent cad1fc52
No related branches found
No related tags found
No related merge requests found
...@@ -41,22 +41,12 @@ func TestCertCache(t *testing.T) { ...@@ -41,22 +41,12 @@ func TestCertCache(t *testing.T) {
timeoutRefCheck := func(t *testing.T, key string, count int64) { timeoutRefCheck := func(t *testing.T, key string, count int64) {
t.Helper() 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) timeout := time.After(4 * time.Second)
check := time.After(1 * time.Millisecond)
for { for {
select { select {
case <-timeout: case <-timeout:
t.Fatal("timed out waiting for expected ref count") t.Fatal("timed out waiting for expected ref count")
case <-check: default:
e, ok := cc.Load(key) e, ok := cc.Load(key)
if !ok && count != 0 { if !ok && count != 0 {
t.Fatal("cache does not contain expected key") t.Fatal("cache does not contain expected key")
...@@ -68,6 +58,15 @@ func TestCertCache(t *testing.T) { ...@@ -68,6 +58,15 @@ func TestCertCache(t *testing.T) {
return 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()
} }
} }
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment