TestRenewFromCache and TestRenewFromCacheAlreadyRenewed had several
races and API misuses:
1. They called t.Fatalf from a goroutine other than the one invoking
the Test function, which is explicitly disallowed (see
https://pkg.go.dev/testing#T).
2. The test did not stop the renewal timers prior to restoring
test-hook functions, and the process of stopping the renewal timers
itself did not wait for in-flight calls to complete. That could
cause data races if one of the renewals failed and triggered a
retry with a short-enough randomized backoff.
(One such race was observed in
https://build.golang.org/log/1a19e22ad826bedeb5a939c6130f368f9979208a.)
3. The testDidRenewLoop hooks accessed the Manager.renewal field
without locking the Mutex guarding that field.
4. TestGetCertificate_failedAttempt set a testDidRemoveState hook, but
didn't wait for the timers referring to that hook to complete
before restoring it, causing races with other timers. I tried
pulling on that thread a bit, but couldn't untangle the numerous
untracked goroutines in the package. Instead, I have made a smaller
and more local change to copy the value of testDidRemoveState into
a local variable in the timer's closure.
Given the number of untracked goroutines in this package, it is likely
that races and/or deadlocks remain. Notably, so far I have been unable
to spot the actual cause of golang/go#51080.
For golang/go#51080
Change-Id: I7797f6ac34ef3c272f16ca805251dac3aa7f0009
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/384594
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Bryan C. Mills
f4118a5b28