Merge lp:~chipaca/ubuntu-push/client-session-redialer-mash into lp:ubuntu-push

[John Lenton]

Reworked or refactored util/redialer, and a bit of client/session, and client.

Reworked and/or refactored util/redialer, so i could refactor session, so i could fix client. Or something like that anyway.

[John Lenton (chipaca)]

So, in this branch:
  * reworked redialer:
    * split Dialer into Dialer and Jitterer; only Jitterers are
      jittered. Also removed String from the Dialer interface.
    * moved the jitter default implementation to be the session's;
      moved the test
    * got rid of the AutoRedial/AutoRetry functions; always create an
      AutoRedialer (via NewAutoRedialer) and use it.
    * got rid of quitRedialing, because
    * gave AutoRedialers a Stop() method that would do the right
      thing.
  * session:
    * added AutoRedial, that sets up and launches an AutoRedialer on
      itself.
    * Close() now stops the AutoRedialer
  * client & session:
    * connectSession is now session.AutoRedial; got rid of the extra
      channel needed to be able to stop it. Renamed the channel used
      to communicate with the autoredialer for clarity.
    * disconnectSession is now session.Close()

[Samuele Pedroni (pedronis)]

636 +func (s *RedialerSuite) TestTwoStops(c *C) {
637 + endp := testibus.NewTestingEndpoint(condition.Work(false), nil)
638 + countCh := make(chan uint32)
639 + ar := NewAutoRedialer(endp)
640 + go func() { countCh <- ar.Redial() }()
641 + ar.Stop()
642 + ar.Stop()
643 }

misses some Checks? otherwise it could be simply folded in the previous test

[Samuele Pedroni (pedronis)]

260 + sess.retrier.Stop()

there is no test that fails if I comment out that line

[Samuele Pedroni (pedronis)]

254 + sess.retrier.Stop()

should this be a Close perhaps? also no test fails if I comment that out

[Samuele Pedroni (pedronis)]

otherwise looks very much an improvement

[Samuele Pedroni (pedronis)]

ah, looking at make coverage-html output:

func (sess *ClientSession) AutoRedial(doneCh chan uint32) {
        // sess.retrier.Stop()
        sess.retrier = util.NewAutoRedialer(sess)
        go func() { doneCh <- sess.retrier.Redial() }()
}

seems not to be covered at all by tests in client/session

[John Lenton (chipaca)]

TestTwoStops was just checking that the second Stop() didn't panic nor block; I've folded it into the previous test as you suggested, because there the state after the first Stop() is ensured by the select (I'd have to copy the test all over again, otherwise, really).

[lp:~chipaca/ubuntu-push/client-session-redialer-mash]

54. By John Lenton on 2014-02-05

made AutoRedialer an interface

55. By John Lenton on 2014-02-05

tested the closing of the retrier by session.Close and session.AutoRedial

[John Lenton (chipaca)]

Added tests for AutoRedial, and for stopping the retrier. Coverage now at 100%. Huzzah.

[lp:~chipaca/ubuntu-push/client-session-redialer-mash]

56. By John Lenton on 2014-02-05

stopRedial

[Samuele Pedroni (pedronis)]

goody good

review: Approve

=== modified file 'bus/connectivity/connectivity.go'
--- bus/connectivity/connectivity.go	2014-01-28 22:18:39 +0000
+++ bus/connectivity/connectivity.go	2014-02-05 13:24:10 +0000
@@ -64,7 +64,8 @@
 func (cs *connectedState) start() networkmanager.State {
 	var initial networkmanager.State
 	for {
-		cs.connAttempts += util.AutoRedial(cs.endp)
+		ar := util.NewAutoRedialer(cs.endp)
+		cs.connAttempts += ar.Redial()
 		nm := networkmanager.New(cs.endp, cs.log)
 
 		// Get the current state.

=== modified file 'bus/endpoint.go'
--- bus/endpoint.go	2014-01-27 12:41:54 +0000
+++ bus/endpoint.go	2014-02-05 13:24:10 +0000
@@ -22,7 +22,6 @@
 	"fmt"
 	"launchpad.net/go-dbus/v1"
 	"launchpad.net/ubuntu-push/logger"
-	"time"
 )
 
 /*****************************************************************
@@ -37,7 +36,6 @@
 	Dial() error
 	Close()
 	String() string
-	Jitter(time.Duration) time.Duration
 }
 
 type endpoint struct {
@@ -137,11 +135,6 @@
 	return fmt.Sprintf("<Connection to %s %#v>", endp.bus, endp.addr)
 }
 
-// Jitter() returns 0: no need to jitter D-Bus connections.
-func (endp *endpoint) Jitter(_ time.Duration) time.Duration {
-	return 0
-}
-
 /*
    private methods
 */

=== modified file 'bus/testing/testing_endpoint.go'
--- bus/testing/testing_endpoint.go	2014-02-03 20:17:14 +0000
+++ bus/testing/testing_endpoint.go	2014-02-05 13:24:10 +0000
@@ -135,8 +135,5 @@
 // see Endpoint's Close. This one does nothing.
 func (tc *testingEndpoint) Close() {}
 
-// see Endpoint's Jitter.
-func (tc *testingEndpoint) Jitter(_ time.Duration) time.Duration { return 0 }
-
 // ensure testingEndpoint implements bus.Endpoint
 var _ bus.Endpoint = &testingEndpoint{}

=== modified file 'bus/testing/testing_endpoint_test.go'
--- bus/testing/testing_endpoint_test.go	2014-02-03 20:17:14 +0000
+++ bus/testing/testing_endpoint_test.go	2014-02-05 13:24:10 +0000
@@ -171,9 +171,3 @@
 	endp := NewTestingEndpoint(condition.Fail2Work(2), nil, "hello there")
 	c.Check(endp.String(), Matches, ".*Still Broken.*hello there.*")
 }
-
-// Test testingEndpoints have no jitters
-func (s *TestingBusSuite) TestEndpointJitter(c *C) {
-	endp := NewTestingEndpoint(nil, nil)
-	c.Check(endp.Jitter(time.Duration(42)), Equals, time.Duration(0))
-}

=== modified file 'client/client.go'
--- client/client.go	2014-02-04 18:19:14 +0000
+++ client/client.go	2014-02-05 13:24:10 +0000
@@ -51,20 +51,19 @@
 
 // Client is the Ubuntu Push Notifications client-side daemon.
 type Client struct {
-	config                ClientConfig
-	log                   logger.Logger
-	pem                   []byte
-	idder                 identifier.Id
-	deviceId              string
-	notificationsEndp     bus.Endpoint
-	urlDispatcherEndp     bus.Endpoint
-	connectivityEndp      bus.Endpoint
-	connCh                chan bool
-	hasConnectivity       bool
-	actionsCh             <-chan notifications.RawActionReply
-	session               *session.ClientSession
-	sessionRetrierStopper chan bool
-	sessionRetryCh        chan uint32
+	config             ClientConfig
+	log                logger.Logger
+	pem                []byte
+	idder              identifier.Id
+	deviceId           string
+	notificationsEndp  bus.Endpoint
+	urlDispatcherEndp  bus.Endpoint
+	connectivityEndp   bus.Endpoint
+	connCh             chan bool
+	hasConnectivity    bool
+	actionsCh          <-chan notifications.RawActionReply
+	session            *session.ClientSession
+	sessionConnectedCh chan uint32
 }
 
 // Configure loads the configuration specified in configPath, and sets it up.
@@ -86,8 +85,8 @@
 	client.urlDispatcherEndp = bus.SessionBus.Endpoint(urldispatcher.BusAddress, client.log)
 	client.connectivityEndp = bus.SystemBus.Endpoint(networkmanager.BusAddress, client.log)
 
-	client.connCh = make(chan bool)
-	client.sessionRetryCh = make(chan uint32)
+	client.connCh = make(chan bool, 1)
+	client.sessionConnectedCh = make(chan uint32, 1)
 
 	if client.config.CertPEMFile != "" {
 		client.pem, err = ioutil.ReadFile(client.config.CertPEMFile)
@@ -119,8 +118,8 @@
 	go connectivity.ConnectedState(client.connectivityEndp,
 		client.config.ConnectivityConfig, client.log, client.connCh)
 	iniCh := make(chan uint32)
-	go func() { iniCh <- util.AutoRedial(client.notificationsEndp) }()
-	go func() { iniCh <- util.AutoRedial(client.urlDispatcherEndp) }()
+	go func() { iniCh <- util.NewAutoRedialer(client.notificationsEndp).Redial() }()
+	go func() { iniCh <- util.NewAutoRedialer(client.urlDispatcherEndp).Redial() }()
 	<-iniCh
 	<-iniCh
 
@@ -140,32 +139,6 @@
 	return nil
 }
 
-// connectSession kicks off the session connection dance
-func (client *Client) connectSession() {
-	// XXX: lp:1276199
-	if client.sessionRetrierStopper != nil {
-		client.sessionRetrierStopper <- true
-		client.sessionRetrierStopper = nil
-	}
-	ar := &util.AutoRetrier{
-		make(chan bool, 1),
-		client.session.Dial,
-		util.Jitter}
-	client.sessionRetrierStopper = ar.Stop
-	go func() { client.sessionRetryCh <- ar.Retry() }()
-}
-
-// disconnectSession disconnects the session
-func (client *Client) disconnectSession() {
-	// XXX: lp:1276199
-	if client.sessionRetrierStopper != nil {
-		client.sessionRetrierStopper <- true
-		client.sessionRetrierStopper = nil
-	} else {
-		client.session.Close()
-	}
-}
-
 // handleConnState deals with connectivity events
 func (client *Client) handleConnState(hasConnectivity bool) {
 	if client.hasConnectivity == hasConnectivity {
@@ -174,9 +147,9 @@
 	}
 	client.hasConnectivity = hasConnectivity
 	if hasConnectivity {
-		client.connectSession()
+		client.session.AutoRedial(client.sessionConnectedCh)
 	} else {
-		client.disconnectSession()
+		client.session.Close()
 	}
 }
 
@@ -185,7 +158,7 @@
 	// if we're not connected, we don't really care
 	client.log.Errorf("session exited: %s", err)
 	if client.hasConnectivity {
-		client.connectSession()
+		client.session.AutoRedial(client.sessionConnectedCh)
 	}
 }
 
@@ -232,7 +205,7 @@
 			notifhandler()
 		case err := <-client.session.ErrCh:
 			errhandler(err)
-		case count := <-client.sessionRetryCh:
+		case count := <-client.sessionConnectedCh:
 			client.log.Debugf("Session connected after %d attempts", count)
 		}
 	}

=== modified file 'client/client_test.go'
--- client/client_test.go	2014-02-04 13:39:06 +0000
+++ client/client_test.go	2014-02-05 13:24:10 +0000
@@ -312,14 +312,9 @@
 	cli := new(Client)
 	cli.log = debuglog
 	cli.initSession()
-	// let's pretend the client had a previous attempt at connecting still pending
-	// (hard to trigger in real life, but possible)
-	ch := make(chan bool, 1)
-	cli.sessionRetrierStopper = ch
 
 	c.Assert(cli.hasConnectivity, Equals, false)
 	cli.handleConnState(true)
-	c.Check(len(ch), Equals, 1)
 	c.Check(cli.hasConnectivity, Equals, true)
 	c.Assert(cli.session, NotNil)
 }
@@ -355,12 +350,10 @@
 	cli := new(Client)
 	cli.log = debuglog
 	cli.session, _ = session.NewSession(string(cli.config.Addr), cli.pem, cli.config.ExchangeTimeout.Duration, cli.deviceId, debuglog)
-	cli.sessionRetrierStopper = make(chan bool, 1)
 	cli.hasConnectivity = true
 
 	cli.handleConnState(false)
 	c.Check(cli.session.State(), Equals, session.Disconnected)
-	c.Check(cli.sessionRetrierStopper, IsNil)
 }
 
 /*****************************************************************

=== modified file 'client/session/session.go'
--- client/session/session.go	2014-02-04 13:10:18 +0000
+++ client/session/session.go	2014-02-05 13:24:10 +0000
@@ -26,6 +26,8 @@
 	"launchpad.net/ubuntu-push/client/session/levelmap"
 	"launchpad.net/ubuntu-push/logger"
 	"launchpad.net/ubuntu-push/protocol"
+	"launchpad.net/ubuntu-push/util"
+	"math/rand"
 	"net"
 	"sync/atomic"
 	"time"
@@ -68,6 +70,7 @@
 	TLS          *tls.Config
 	proto        protocol.Protocol
 	pingInterval time.Duration
+	retrier      util.AutoRedialer
 	// status
 	stateP *uint32
 	ErrCh  chan error
@@ -119,7 +122,24 @@
 	return nil
 }
 
+func (sess *ClientSession) stopRedial() {
+	if sess.retrier != nil {
+		sess.retrier.Stop()
+		sess.retrier = nil
+	}
+}
+
+func (sess *ClientSession) AutoRedial(doneCh chan uint32) {
+	sess.stopRedial()
+	sess.retrier = util.NewAutoRedialer(sess)
+	go func() { doneCh <- sess.retrier.Redial() }()
+}
+
 func (sess *ClientSession) Close() {
+	sess.stopRedial()
+	sess.doClose()
+}
+func (sess *ClientSession) doClose() {
 	if sess.Connection != nil {
 		sess.Connection.Close()
 		// we ignore Close errors, on purpose (the thinking being that
@@ -258,6 +278,15 @@
 	return err
 }
 
+// This Jitter returns a random time.Duration somewhere in [-spread, spread].
+func (sess *ClientSession) Jitter(spread time.Duration) time.Duration {
+	if spread < 0 {
+		panic("spread must be non-negative")
+	}
+	n := int64(spread)
+	return time.Duration(rand.Int63n(2*n+1) - n)
+}
+
 // Dial takes the session from newly created (or newly disconnected)
 // to running the main loop.
 func (sess *ClientSession) Dial() error {
@@ -267,5 +296,9 @@
 		// keep on trying.
 		panic("can't Dial() without a protocol constructor.")
 	}
-	return sess.run(sess.Close, sess.connect, sess.start, sess.loop)
+	return sess.run(sess.doClose, sess.connect, sess.start, sess.loop)
+}
+
+func init() {
+	rand.Seed(time.Now().Unix()) // good enough for us (we're not using it for crypto)
 }

=== modified file 'client/session/session_test.go'
--- client/session/session_test.go	2014-02-04 13:02:38 +0000
+++ client/session/session_test.go	2014-02-05 13:24:10 +0000
@@ -256,6 +256,50 @@
 	c.Check(sess.State(), Equals, Disconnected)
 }
 
+type derp struct{ stopped bool }
+
+func (*derp) Redial() uint32 { return 0 }
+func (d *derp) Stop()        { d.stopped = true }
+
+func (cs *clientSessionSuite) TestCloseStopsRetrier(c *C) {
+	sess, err := NewSession("", nil, 0, "wah", debuglog)
+	c.Assert(err, IsNil)
+	ar := new(derp)
+	sess.retrier = ar
+	c.Check(ar.stopped, Equals, false)
+	sess.Close()
+	c.Check(ar.stopped, Equals, true)
+	sess.Close() // double close check
+	c.Check(ar.stopped, Equals, true)
+}
+
+/****************************************************************
+  AutoRedial() tests
+****************************************************************/
+
+func (cs *clientSessionSuite) TestAutoRedialWorks(c *C) {
+	// checks that AutoRedial sets up a retrier and tries redialing it
+	sess, err := NewSession("", nil, 0, "wah", debuglog)
+	c.Assert(err, IsNil)
+	ar := new(derp)
+	sess.retrier = ar
+	c.Check(ar.stopped, Equals, false)
+	sess.AutoRedial(nil)
+	c.Check(ar.stopped, Equals, true)
+}
+
+func (cs *clientSessionSuite) TestAutoRedialStopsRetrier(c *C) {
+	// checks that AutoRedial stops the previous retrier
+	sess, err := NewSession("", nil, 0, "wah", debuglog)
+	c.Assert(err, IsNil)
+	ch := make(chan uint32)
+	c.Check(sess.retrier, IsNil)
+	sess.AutoRedial(ch)
+	c.Assert(sess.retrier, NotNil)
+	sess.retrier.Stop()
+	c.Check(<-ch, Not(Equals), 0)
+}
+
 /****************************************************************
   handlePing() tests
 ****************************************************************/
@@ -624,12 +668,43 @@
 }
 
 /****************************************************************
+  Jitter() tests
+****************************************************************/
+
+func (s *clientSessionSuite) TestJitter(c *C) {
+	sess, err := NewSession("", nil, 0, "wah", helpers.NewTestLogger(c, "debug"))
+	c.Assert(err, IsNil)
+	num_tries := 20       // should do the math
+	spread := time.Second //
+	has_neg := false
+	has_pos := false
+	has_zero := true
+	for i := 0; i < num_tries; i++ {
+		n := sess.Jitter(spread)
+		if n > 0 {
+			has_pos = true
+		} else if n < 0 {
+			has_neg = true
+		} else {
+			has_zero = true
+		}
+	}
+	c.Check(has_neg, Equals, true)
+	c.Check(has_pos, Equals, true)
+	c.Check(has_zero, Equals, true)
+
+	// a negative spread is caught in the reasonable place
+	c.Check(func() { sess.Jitter(time.Duration(-1)) }, PanicMatches,
+		"spread must be non-negative")
+}
+
+/****************************************************************
   Dial() tests
 ****************************************************************/
 
 func (cs *clientSessionSuite) TestDialPanics(c *C) {
 	// one last unhappy test
-	sess, err := NewSession("", nil, 0, "wah", debuglog)
+	sess, err := NewSession("", nil, 0, "wah", helpers.NewTestLogger(c, "debug"))
 	c.Assert(err, IsNil)
 	sess.Protocolator = nil
 	c.Check(sess.Dial, PanicMatches, ".*protocol constructor.")

=== modified file 'util/redialer.go'
--- util/redialer.go	2014-02-04 13:02:38 +0000
+++ util/redialer.go	2014-02-05 13:24:10 +0000
@@ -17,7 +17,6 @@
 package util
 
 import (
-	"math/rand"
 	"sync"
 	"time"
 )
@@ -27,7 +26,12 @@
 // fails.
 type Dialer interface {
 	Dial() error
-	String() string
+}
+
+// A Jitterer is a Dialer that wants to vary the backoff a little (to avoid a
+// thundering herd, for example).
+type Jitterer interface {
+	Dialer
 	Jitter(time.Duration) time.Duration
 }
 
@@ -35,17 +39,16 @@
 var timeouts []time.Duration
 var trwlock sync.RWMutex
 
-var ( //  for use in testing
-	quitRedialing chan bool = make(chan bool)
-)
-
+// retrieve the list of timeouts used for exponential backoff
 func Timeouts() []time.Duration {
 	trwlock.RLock()
 	defer trwlock.RUnlock()
 	return timeouts
 }
 
-// for testing
+// for testing: change the default timeouts for the provided ones,
+// returning the defaults (the idea being you reset them on test
+// teardown)
 func SwapTimeouts(newTimeouts []time.Duration) (oldTimeouts []time.Duration) {
 	trwlock.Lock()
 	defer trwlock.Unlock()
@@ -53,32 +56,60 @@
 	return
 }
 
-// Jitter returns a random time.Duration somewhere in [-spread, spread].
-//
-// This is meant as a default implementation for Dialers to use if wanted.
-func Jitter(spread time.Duration) time.Duration {
-	if spread < 0 {
-		panic("spread must be non-negative")
+// An AutoRedialer's Redial() method retries its dialer's Dial() method until
+// it stops returning an error. It does exponential (optionally jitter'ed)
+// backoff.
+type AutoRedialer interface {
+	Redial() uint32 // Redial keeps on calling Dial until it stops returning an error.
+	Stop()          // Stop shuts down the given AutoRedialer, if it is still retrying.
+}
+
+type autoRedialer struct {
+	stop   chan bool
+	lock   sync.RWMutex
+	dial   func() error
+	jitter func(time.Duration) time.Duration
+}
+
+func (ar *autoRedialer) Stop() {
+	if ar != nil {
+		ar.lock.RLock()
+		defer ar.lock.RUnlock()
+		if ar.stop != nil {
+			ar.stop <- true
+		}
 	}
-	n := int64(spread)
-	return time.Duration(rand.Int63n(2*n+1) - n)
-}
-
-type AutoRetrier struct {
-	Stop   chan bool
-	Dial   func() error
-	Jitter func(time.Duration) time.Duration
-}
-
-// AutoRetry keeps on calling Dial until it stops returning an error.  It does
+}
+
+func (ar *autoRedialer) shutdown() {
+	ar.lock.Lock()
+	defer ar.lock.Unlock()
+	close(ar.stop)
+	ar.stop = nil
+}
+
+// Redial keeps on calling Dial until it stops returning an error.  It does
 // exponential backoff, adding the output of Jitter at each step back.
-func (ar *AutoRetrier) Retry() uint32 {
+func (ar *autoRedialer) Redial() uint32 {
+	if ar == nil {
+		// at least it's better than the segfault...
+		panic("you can't Redial a nil AutoRedialer")
+	}
+	if ar.stop == nil {
+		panic("this AutoRedialer has already been shut down")
+	}
+	defer ar.shutdown()
+
+	ar.lock.RLock()
+	stop := ar.stop
+	ar.lock.RUnlock()
+
 	var timeout time.Duration
 	var dialAttempts uint32 = 0 // unsigned so it can wrap safely ...
 	timeouts := Timeouts()
 	var numTimeouts uint32 = uint32(len(timeouts))
 	for {
-		if ar.Dial() == nil {
+		if ar.dial() == nil {
 			return dialAttempts + 1
 		}
 		if dialAttempts < numTimeouts {
@@ -86,22 +117,27 @@
 		} else {
 			timeout = timeouts[numTimeouts-1]
 		}
-		timeout += ar.Jitter(timeout)
+		if ar.jitter != nil {
+			timeout += ar.jitter(timeout)
+		}
 		dialAttempts++
 		select {
-		case <-ar.Stop:
+		case <-stop:
 			return dialAttempts
-		case <-time.NewTimer(timeout).C:
+		case <-time.After(timeout):
 		}
 	}
 }
 
-// AutoRedialer takes a Dialer and retries its Dial() method until it
-// stops returning an error. It does exponential (optionally
-// jitter'ed) backoff.
-func AutoRedial(dialer Dialer) uint32 {
-	ar := &AutoRetrier{quitRedialing, dialer.Dial, dialer.Jitter}
-	return ar.Retry()
+// returns a stoppable AutoRedialer using the provided Dialer. If the Dialer
+// is also a Jitterer, the backoff will be jittered.
+func NewAutoRedialer(dialer Dialer) AutoRedialer {
+	ar := &autoRedialer{stop: make(chan bool), dial: dialer.Dial}
+	jitterer, ok := dialer.(Jitterer)
+	if ok {
+		ar.jitter = jitterer.Jitter
+	}
+	return ar
 }
 
 func init() {
@@ -111,5 +147,4 @@
 		timeouts[i] = time.Duration(n) * time.Second
 	}
 	SwapTimeouts(timeouts)
-	rand.Seed(time.Now().Unix()) // good enough for us (not crypto, yadda)
 }

=== modified file 'util/redialer_test.go'
--- util/redialer_test.go	2014-02-04 13:02:38 +0000
+++ util/redialer_test.go	2014-02-05 13:24:10 +0000
@@ -17,9 +17,9 @@
 package util
 
 import (
-	"errors"
 	"io/ioutil"
 	. "launchpad.net/gocheck"
+	"launchpad.net/ubuntu-push/bus"
 	testibus "launchpad.net/ubuntu-push/bus/testing"
 	"launchpad.net/ubuntu-push/logger"
 	"launchpad.net/ubuntu-push/testing/condition"
@@ -46,60 +46,73 @@
 	s.timeouts = nil
 }
 
+// Redial() tests
+
 func (s *RedialerSuite) TestWorks(c *C) {
 	endp := testibus.NewTestingEndpoint(condition.Fail2Work(3), nil)
-	// instead of bus.Dial(), we do AutoRedial(bus)
-	c.Check(AutoRedial(endp), Equals, uint32(4))
-}
-
-func (s *RedialerSuite) TestCanBeStopped(c *C) {
+	ar := NewAutoRedialer(endp)
+	c.Check(ar.(*autoRedialer).stop, NotNil)
+	c.Check(ar.Redial(), Equals, uint32(4))
+	// and on success, the stopper goes away
+	c.Check(ar.(*autoRedialer).stop, IsNil)
+}
+
+func (s *RedialerSuite) TestRetryNil(c *C) {
+	var ar *autoRedialer
+	c.Check(ar.Redial, Not(PanicMatches), ".* nil pointer dereference")
+}
+
+func (s *RedialerSuite) TestRetryTwice(c *C) {
+	endp := testibus.NewTestingEndpoint(condition.Work(true), nil)
+	ar := NewAutoRedialer(endp)
+	c.Check(ar.Redial(), Equals, uint32(1))
+	c.Check(ar.Redial, PanicMatches, ".*shut.?down.*")
+}
+
+type JitteringEndpoint struct {
+	bus.Endpoint
+	jittered int
+}
+
+func (j *JitteringEndpoint) Jitter(time.Duration) time.Duration {
+	j.jittered++
+	return 0
+}
+
+func (s *RedialerSuite) TestJitterWorks(c *C) {
+	endp := &JitteringEndpoint{
+		testibus.NewTestingEndpoint(condition.Fail2Work(3), nil),
+		0,
+	}
+	ar := NewAutoRedialer(endp)
+	c.Check(ar.Redial(), Equals, uint32(4))
+	c.Check(endp.jittered, Equals, 3)
+}
+
+// Stop() tests
+
+func (s *RedialerSuite) TestStopWorksOnNil(c *C) {
+	// as a convenience, Stop() should succeed on nil
+	// (a nil retrier certainly isn't retrying!)
+	var ar *autoRedialer
+	c.Check(ar, IsNil)
+	ar.Stop() // nothing happens
+}
+
+func (s *RedialerSuite) TestStopStops(c *C) {
 	endp := testibus.NewTestingEndpoint(condition.Work(false), nil)
-	ch := make(chan uint32)
-	go func() { ch <- AutoRedial(endp) }()
-	quitRedialing <- true
+	countCh := make(chan uint32)
+	ar := NewAutoRedialer(endp)
+	go func() { countCh <- ar.Redial() }()
+	ar.Stop()
 	select {
-	case n := <-ch:
+	case n := <-countCh:
 		c.Check(n, Equals, uint32(1))
-	case <-time.Tick(20 * time.Millisecond):
+	case <-time.After(20 * time.Millisecond):
 		c.Fatal("timed out waiting for redial")
 	}
-}
-
-func (s *RedialerSuite) TestAutoRetry(c *C) {
-	cond := condition.Fail2Work(5)
-	f := func() error {
-		if cond.OK() {
-			return nil
-		} else {
-			return errors.New("X")
-		}
-	}
-	jitter := func(time.Duration) time.Duration { return 0 }
-	ar := &AutoRetrier{nil, f, jitter}
-	c.Check(ar.Retry(), Equals, uint32(6))
-}
-
-func (s *RedialerSuite) TestJitter(c *C) {
-	num_tries := 20       // should do the math
-	spread := time.Second //
-	has_neg := false
-	has_pos := false
-	has_zero := true
-	for i := 0; i < num_tries; i++ {
-		n := Jitter(spread)
-		if n > 0 {
-			has_pos = true
-		} else if n < 0 {
-			has_neg = true
-		} else {
-			has_zero = true
-		}
-	}
-	c.Check(has_neg, Equals, true)
-	c.Check(has_pos, Equals, true)
-	c.Check(has_zero, Equals, true)
-
-	// a negative spread is caught in the reasonable place
-	c.Check(func() { Jitter(time.Duration(-1)) }, PanicMatches,
-		"spread must be non-negative")
+	// on Stop(), the stopper goes away too
+	c.Check(ar.(*autoRedialer).stop, IsNil)
+	// and the next Stop() doesn't panic nor block
+	ar.Stop()
 }