acme/autocert: make tests more well-behaved

This change also gets the Manager closer to being able
to cleanup in short-lived HTTP servers running in a long-lived binary.

Change-Id: I49db36156896acc76d4757146c26b99e1665423b
Reviewed-on: https://go-review.googlesource.com/28491
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

acme/autocert/autocert.go

@@ -25,7 +25,6 @@ import (
 	"strconv"
 	"strings"
 	"sync"
-	"sync/atomic"
 	"time"
 
 	"golang.org/x/crypto/acme"
@@ -36,7 +35,7 @@ import (
 var pseudoRand *lockedMathRand
 
 func init() {
-	src := mathrand.NewSource(time.Now().UnixNano())
+	src := mathrand.NewSource(timeNow().UnixNano())
 	pseudoRand = &lockedMathRand{rnd: mathrand.New(src)}
 }
 
@@ -531,7 +530,18 @@ func (m *Manager) renew(domain string, key crypto.Signer, exp time.Time) {
 	}
 	dr := &domainRenewal{m: m, domain: domain, key: key}
 	m.renewal[domain] = dr
-	time.AfterFunc(dr.next(exp), dr.renew)
+	dr.start(exp)
+}
+
+// stopRenew stops all currently running cert renewal timers.
+// The timers are not restarted during the lifetime of the Manager.
+func (m *Manager) stopRenew() {
+	m.renewalMu.Lock()
+	defer m.renewalMu.Unlock()
+	for name, dr := range m.renewal {
+		delete(m.renewal, name)
+		dr.stop()
+	}
 }
 
 func (m *Manager) acmeClient(ctx context.Context) (*acme.Client, error) {
@@ -719,23 +729,5 @@ func (r *lockedMathRand) int63n(max int64) int64 {
 	return n
 }
 
-func timeNow() time.Time {
-	return clock.Load().(func() time.Time)()
-}
-
 // for easier testing
-var (
-	// clock stores the time.Now func pointer or a fake
-	// thereof. It's an atomic.Value because tests weren't waiting
-	// for goroutines to shut down during completing causing races.
-	// TODO(crhym3,bradfitz): make tests more well-behaved, and
-	// then revert this back to just a func() time.Time type.
-	// This was the easier quick fix.
-	clock atomic.Value
-
-	testDidRenewLoop = func(next time.Duration, err error) {}
-)
-
-func init() {
-	clock.Store(time.Now)
-}
+var timeNow = time.Now

acme/autocert/autocert_test.go

@@ -109,6 +109,7 @@ func decodePayload(v interface{}, r io.Reader) error {
 func TestGetCertificate(t *testing.T) {
 	const domain = "example.org"
 	man := &Manager{Prompt: AcceptTOS}
+	defer man.stopRenew()
 
 	// echo token-02 | shasum -a 256
 	// then divide result in 2 parts separated by dot
@@ -264,7 +265,8 @@ func TestCache(t *testing.T) {
 	}
 
 	cache := make(memCache)
-	man := Manager{Cache: cache}
+	man := &Manager{Cache: cache}
+	defer man.stopRenew()
 	if err := man.cachePut("example.org", tlscert); err != nil {
 		t.Fatalf("man.cachePut: %v", err)
 	}

acme/autocert/renewal.go

@@ -6,6 +6,7 @@ package autocert
 
 import (
 	"crypto"
+	"sync"
 	"time"
 
 	"golang.org/x/net/context"
@@ -20,11 +21,45 @@ type domainRenewal struct {
 	m      *Manager
 	domain string
 	key    crypto.Signer
+
+	timerMu sync.Mutex
+	timer   *time.Timer
+}
+
+// start starts a cert renewal timer at the time
+// defined by the certificate expiration time exp.
+//
+// If the timer is already started, calling start is a noop.
+func (dr *domainRenewal) start(exp time.Time) {
+	dr.timerMu.Lock()
+	defer dr.timerMu.Unlock()
+	if dr.timer != nil {
+		return
+	}
+	dr.timer = time.AfterFunc(dr.next(exp), dr.renew)
+}
+
+// stop stops the cert renewal timer.
+// If the timer is already stopped, calling stop is a noop.
+func (dr *domainRenewal) stop() {
+	dr.timerMu.Lock()
+	defer dr.timerMu.Unlock()
+	if dr.timer == nil {
+		return
+	}
+	dr.timer.Stop()
+	dr.timer = nil
 }
 
 // renew is called periodically by a timer.
-// The first renew call is kicked off by dr.m.renew.
+// The first renew call is kicked off by dr.start.
 func (dr *domainRenewal) renew() {
+	dr.timerMu.Lock()
+	defer dr.timerMu.Unlock()
+	if dr.timer == nil {
+		return
+	}
+
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
 	defer cancel()
 	// TODO: rotate dr.key at some point?
@@ -33,8 +68,8 @@ func (dr *domainRenewal) renew() {
 		next = maxRandRenew / 2
 		next += time.Duration(pseudoRand.int63n(int64(next)))
 	}
+	dr.timer = time.AfterFunc(next, dr.renew)
 	testDidRenewLoop(next, err)
-	time.AfterFunc(next, dr.renew)
 }
 
 // do is similar to Manager.createCert but it doesn't lock a Manager.state item.
@@ -86,3 +121,5 @@ func (dr *domainRenewal) next(expiry time.Time) time.Duration {
 	}
 	return d
 }
+
+var testDidRenewLoop = func(next time.Duration, err error) {}

acme/autocert/renewal_test.go

@@ -22,10 +22,11 @@ import (
 
 func TestRenewalNext(t *testing.T) {
 	now := time.Now()
-	clock.Store(func() time.Time { return now })
-	defer clock.Store(time.Now)
+	timeNow = func() time.Time { return now }
+	defer func() { timeNow = time.Now }()
 
 	man := &Manager{RenewBefore: 7 * 24 * time.Hour}
+	defer man.stopRenew()
 	tt := []struct {
 		expiry   time.Time
 		min, max time.Duration
@@ -117,6 +118,7 @@ func TestRenewFromCache(t *testing.T) {
 			DirectoryURL: ca.URL,
 		},
 	}
+	defer man.stopRenew()
 
 	// cache an almost expired cert
 	now := time.Now()