Merge lp:~chipaca/ubuntu-push/no-more-session-autoredial into lp:ubuntu-push/automatic

[John Lenton]

Remove AutoRedial from ClientSession's interface.

[lp:~chipaca/ubuntu-push/no-more-session-autoredial]

378. By John Lenton on 2015-03-12

removed spurious test

379. By John Lenton on 2015-03-12

merged from lp

[Samuele Pedroni (pedronis)]

see comments

[John Lenton (chipaca)]

Agreed on both points, fixing.

[lp:~chipaca/ubuntu-push/no-more-session-autoredial]

380. By John Lenton on 2015-03-17

call close from stopCh handler; set state to shutdown before closing stopCh.

381. By John Lenton on 2015-03-17

repeat myself inside the stopCh handler, because it's not *exactly* doClose what I want, and there isn't yet a clear way of generalizing that function without making a mess down-pipe.

[Samuele Pedroni (pedronis)]

review: Approve

=== modified file 'client/client.go'
--- client/client.go	2015-03-03 10:46:04 +0000
+++ client/client.go	2015-03-17 16:47:16 +0000
@@ -115,7 +115,6 @@
 	systemImageEndp    bus.Endpoint
 	systemImageInfo    *systemimage.InfoResult
 	connCh             chan bool
-	hasConnectivity    bool
 	session            session.ClientSession
 	sessionConnectedCh chan uint32
 	pushService        PushService
@@ -126,7 +125,6 @@
 	poller             poller.Poller
 	accountsCh         <-chan accounts.Changed
 	// session-side channels
-	errCh           chan error
 	broadcastCh     chan *session.BroadcastNotification
 	notificationsCh chan session.AddressedNotification
 }
@@ -137,7 +135,6 @@
 	return &PushClient{
 		configPath:      configPath,
 		leveldbPath:     leveldbPath,
-		errCh:           make(chan error),
 		broadcastCh:     make(chan *session.BroadcastNotification),
 		notificationsCh: make(chan session.AddressedNotification),
 	}
@@ -212,7 +209,6 @@
 		AuthGetter:       client.getAuthorization,
 		AuthURL:          client.config.SessionURL,
 		AddresseeChecker: client,
-		ErrCh:            client.errCh,
 		BroadcastCh:      client.broadcastCh,
 		NotificationsCh:  client.notificationsCh,
 	}
@@ -317,6 +313,7 @@
 		return err
 	}
 	client.session = sess
+	sess.KeepConnection()
 	client.poller = poller.New(client.derivePollerSetup())
 	return nil
 }
@@ -388,24 +385,8 @@
 // handleConnState deals with connectivity events
 func (client *PushClient) handleConnState(hasConnectivity bool) {
 	client.log.Debugf("handleConnState: %v", hasConnectivity)
-	if client.hasConnectivity == hasConnectivity {
-		// nothing to do!
-		return
-	}
-	client.hasConnectivity = hasConnectivity
-	client.session.Close()
-	if hasConnectivity {
-		client.session.AutoRedial(client.sessionConnectedCh)
-	}
-}
-
-// handleErr deals with the session erroring out of its loop
-func (client *PushClient) handleErr(err error) {
-	// if we're not connected, we don't really care
-	client.log.Errorf("session exited: %s", err)
-	if client.hasConnectivity {
-		client.session.AutoRedial(client.sessionConnectedCh)
-	}
+	client.session.HasConnectivity(hasConnectivity)
+	client.log.Debugf("handled.")
 }
 
 // filterBroadcastNotification finds out if the notification is about an actual
@@ -487,7 +468,7 @@
 }
 
 // doLoop connects events with their handlers
-func (client *PushClient) doLoop(connhandler func(bool), bcasthandler func(*session.BroadcastNotification) error, ucasthandler func(session.AddressedNotification) error, errhandler func(error), unregisterhandler func(*click.AppId), accountshandler func()) {
+func (client *PushClient) doLoop(connhandler func(bool), bcasthandler func(*session.BroadcastNotification) error, ucasthandler func(session.AddressedNotification) error, unregisterhandler func(*click.AppId), accountshandler func()) {
 	for {
 		select {
 		case <-client.accountsCh:
@@ -498,8 +479,6 @@
 			bcasthandler(bcast)
 		case aucast := <-client.notificationsCh:
 			ucasthandler(aucast)
-		case err := <-client.errCh:
-			errhandler(err)
 		case count := <-client.sessionConnectedCh:
 			client.log.Debugf("session connected after %d attempts", count)
 		case app := <-client.unregisterCh:
@@ -524,7 +503,6 @@
 	client.doLoop(client.handleConnState,
 		client.handleBroadcastNotification,
 		client.handleUnicastNotification,
-		client.handleErr,
 		client.handleUnregister,
 		client.handleAccountsChange,
 	)

=== modified file 'client/client_test.go'
--- client/client_test.go	2015-03-03 10:46:04 +0000
+++ client/client_test.go	2015-03-17 16:47:16 +0000
@@ -43,7 +43,6 @@
 	clickhelp "launchpad.net/ubuntu-push/click/testing"
 	"launchpad.net/ubuntu-push/client/service"
 	"launchpad.net/ubuntu-push/client/session"
-	"launchpad.net/ubuntu-push/client/session/seenstate"
 	"launchpad.net/ubuntu-push/config"
 	"launchpad.net/ubuntu-push/identifier"
 	idtesting "launchpad.net/ubuntu-push/identifier/testing"
@@ -427,7 +426,6 @@
 		AuthGetter:       func(string) string { return "" },
 		AuthURL:          "xyzzy://",
 		AddresseeChecker: cli,
-		ErrCh:            make(chan error),
 		BroadcastCh:      make(chan *session.BroadcastNotification),
 		NotificationsCh:  make(chan session.AddressedNotification),
 	}
@@ -444,10 +442,8 @@
 	// compare authGetter by string
 	c.Check(fmt.Sprintf("%#v", conf.AuthGetter), Equals, fmt.Sprintf("%#v", cli.getAuthorization))
 	// channels are ok as long as non-nil
-	conf.ErrCh = nil
 	conf.BroadcastCh = nil
 	conf.NotificationsCh = nil
-	expected.ErrCh = nil
 	expected.BroadcastCh = nil
 	expected.NotificationsCh = nil
 	// and set it to nil
@@ -534,9 +530,11 @@
 type derivePollerSession struct{}
 
 func (s *derivePollerSession) Close()                            {}
-func (s *derivePollerSession) AutoRedial(ch chan uint32)         {}
 func (s *derivePollerSession) ClearCookie()                      {}
 func (s *derivePollerSession) State() session.ClientSessionState { return session.Unknown }
+func (s *derivePollerSession) HasConnectivity(bool) error        { return nil }
+func (s *derivePollerSession) KeepConnection() error             { return nil }
+func (s *derivePollerSession) StopKeepConnection()               {}
 
 func (cs *clientSuite) TestDerivePollerSetup(c *C) {
 	cs.writeTestConfig(map[string]interface{}{})
@@ -722,22 +720,6 @@
 }
 
 /*****************************************************************
-    handleErr tests
-******************************************************************/
-
-func (cs *clientSuite) TestHandleErr(c *C) {
-	cli := NewPushClient(cs.configPath, cs.leveldbPath)
-	cli.log = cs.log
-	cli.systemImageInfo = siInfoRes
-	c.Assert(cli.initSessionAndPoller(), IsNil)
-	cs.log.ResetCapture()
-	cli.hasConnectivity = true
-	defer cli.session.Close()
-	cli.handleErr(errors.New("bananas"))
-	c.Check(cs.log.Captured(), Matches, ".*session exited.*bananas\n")
-}
-
-/*****************************************************************
     seenStateFactory tests
 ******************************************************************/
 
@@ -758,66 +740,6 @@
 }
 
 /*****************************************************************
-    handleConnState tests
-******************************************************************/
-
-type handleConnStateSession struct {
-	connected bool
-}
-
-func (s *handleConnStateSession) AutoRedial(ch chan uint32)         { s.connected = true }
-func (s *handleConnStateSession) Close()                            { s.connected = false }
-func (s *handleConnStateSession) ClearCookie()                      {}
-func (s *handleConnStateSession) State() session.ClientSessionState { return session.Unknown }
-
-func (cs *clientSuite) TestHandleConnStateD2C(c *C) {
-	cli := NewPushClient(cs.configPath, cs.leveldbPath)
-	cli.log = cs.log
-	sess := &handleConnStateSession{connected: false}
-	cli.session = sess
-
-	c.Assert(cli.hasConnectivity, Equals, false)
-	cli.handleConnState(true)
-	c.Check(cli.hasConnectivity, Equals, true)
-	c.Check(sess.connected, Equals, true)
-}
-
-func (cs *clientSuite) TestHandleConnStateSame(c *C) {
-	cli := NewPushClient(cs.configPath, cs.leveldbPath)
-	cli.log = cs.log
-	// here we want to check that we don't do anything
-	c.Assert(cli.session, IsNil)
-	c.Assert(cli.hasConnectivity, Equals, false)
-	cli.handleConnState(false)
-	c.Check(cli.session, IsNil)
-
-	cli.hasConnectivity = true
-	cli.handleConnState(true)
-	c.Check(cli.session, IsNil)
-}
-
-func (cs *clientSuite) TestHandleConnStateC2D(c *C) {
-	cli := NewPushClient(cs.configPath, cs.leveldbPath)
-	cli.log = cs.log
-	sess := &handleConnStateSession{connected: true}
-	cli.session = sess
-	cli.hasConnectivity = true
-
-	cli.handleConnState(false)
-	c.Check(sess.connected, Equals, false)
-}
-
-func (cs *clientSuite) TestHandleConnStateC2DPending(c *C) {
-	cli := NewPushClient(cs.configPath, cs.leveldbPath)
-	cli.log = cs.log
-	cli.session, _ = session.NewSession(cli.config.Addr, cli.deriveSessionConfig(nil), cli.deviceId, seenstate.NewSeenState, cs.log)
-	cli.hasConnectivity = true
-
-	cli.handleConnState(false)
-	c.Check(cli.session.State(), Equals, session.Disconnected)
-}
-
-/*****************************************************************
    filterBroadcastNotification tests
 ******************************************************************/
 
@@ -1035,7 +957,6 @@
 var nopConn = func(bool) {}
 var nopBcast = func(*session.BroadcastNotification) error { return nil }
 var nopUcast = func(session.AddressedNotification) error { return nil }
-var nopError = func(error) {}
 var nopUnregister = func(*click.AppId) {}
 var nopAcct = func() {}
 
@@ -1048,7 +969,7 @@
 	c.Assert(cli.initSessionAndPoller(), IsNil)
 
 	ch := make(chan bool, 1)
-	go cli.doLoop(func(bool) { ch <- true }, nopBcast, nopUcast, nopError, nopUnregister, nopAcct)
+	go cli.doLoop(func(bool) { ch <- true }, nopBcast, nopUcast, nopUnregister, nopAcct)
 	c.Check(takeNextBool(ch), Equals, true)
 }
 
@@ -1061,7 +982,7 @@
 	cli.broadcastCh <- &session.BroadcastNotification{}
 
 	ch := make(chan bool, 1)
-	go cli.doLoop(nopConn, func(_ *session.BroadcastNotification) error { ch <- true; return nil }, nopUcast, nopError, nopUnregister, nopAcct)
+	go cli.doLoop(nopConn, func(_ *session.BroadcastNotification) error { ch <- true; return nil }, nopUcast, nopUnregister, nopAcct)
 	c.Check(takeNextBool(ch), Equals, true)
 }
 
@@ -1074,20 +995,7 @@
 	cli.notificationsCh <- session.AddressedNotification{}
 
 	ch := make(chan bool, 1)
-	go cli.doLoop(nopConn, nopBcast, func(session.AddressedNotification) error { ch <- true; return nil }, nopError, nopUnregister, nopAcct)
-	c.Check(takeNextBool(ch), Equals, true)
-}
-
-func (cs *clientSuite) TestDoLoopErr(c *C) {
-	cli := NewPushClient(cs.configPath, cs.leveldbPath)
-	cli.log = cs.log
-	cli.systemImageInfo = siInfoRes
-	c.Assert(cli.initSessionAndPoller(), IsNil)
-	cli.errCh = make(chan error, 1)
-	cli.errCh <- nil
-
-	ch := make(chan bool, 1)
-	go cli.doLoop(nopConn, nopBcast, nopUcast, func(error) { ch <- true }, nopUnregister, nopAcct)
+	go cli.doLoop(nopConn, nopBcast, func(session.AddressedNotification) error { ch <- true; return nil }, nopUnregister, nopAcct)
 	c.Check(takeNextBool(ch), Equals, true)
 }
 
@@ -1100,7 +1008,7 @@
 	cli.unregisterCh <- app1
 
 	ch := make(chan bool, 1)
-	go cli.doLoop(nopConn, nopBcast, nopUcast, nopError, func(app *click.AppId) { c.Check(app.Original(), Equals, appId1); ch <- true }, nopAcct)
+	go cli.doLoop(nopConn, nopBcast, nopUcast, func(app *click.AppId) { c.Check(app.Original(), Equals, appId1); ch <- true }, nopAcct)
 	c.Check(takeNextBool(ch), Equals, true)
 }
 
@@ -1114,7 +1022,7 @@
 	cli.accountsCh = acctCh
 
 	ch := make(chan bool, 1)
-	go cli.doLoop(nopConn, nopBcast, nopUcast, nopError, nopUnregister, func() { ch <- true })
+	go cli.doLoop(nopConn, nopBcast, nopUcast, nopUnregister, func() { ch <- true })
 	c.Check(takeNextBool(ch), Equals, true)
 }
 
@@ -1149,6 +1057,21 @@
     Loop() tests
 ******************************************************************/
 
+type loopSession struct{ hasConn bool }
+
+func (s *loopSession) Close()       {}
+func (s *loopSession) ClearCookie() {}
+func (s *loopSession) State() session.ClientSessionState {
+	if s.hasConn {
+		return session.Connected
+	} else {
+		return session.Disconnected
+	}
+}
+func (s *loopSession) HasConnectivity(hasConn bool) error { s.hasConn = hasConn; return nil }
+func (s *loopSession) KeepConnection() error              { return nil }
+func (s *loopSession) StopKeepConnection()                {}
+
 func (cs *clientSuite) TestLoop(c *C) {
 	cli := NewPushClient(cs.configPath, cs.leveldbPath)
 	cli.connCh = make(chan bool)
@@ -1164,7 +1087,6 @@
 	c.Assert(cli.initSessionAndPoller(), IsNil)
 
 	cli.broadcastCh = make(chan *session.BroadcastNotification)
-	cli.errCh = make(chan error)
 
 	// we use tick() to make sure things have been through the
 	// event loop at least once before looking at things;
@@ -1179,26 +1101,25 @@
 	tick()
 	c.Check(cs.log.Captured(), Matches, "(?msi).*Session connected after 42 attempts$")
 
+	c.Assert(cli.session, NotNil)
+	cli.session.StopKeepConnection()
+	cli.session = &loopSession{}
+
 	// loop() should have connected:
 	//  * connCh to the connectivity checker
-	c.Check(cli.hasConnectivity, Equals, false)
+	c.Check(cli.session.State(), Equals, session.Disconnected)
 	cli.connCh <- true
 	tick()
-	c.Check(cli.hasConnectivity, Equals, true)
+	c.Check(cli.session.State(), Equals, session.Connected)
 	cli.connCh <- false
 	tick()
-	c.Check(cli.hasConnectivity, Equals, false)
+	c.Check(cli.session.State(), Equals, session.Disconnected)
 
 	//  * session.BroadcastCh to the notifications handler
 	c.Check(d.bcastCount, Equals, 0)
 	cli.broadcastCh <- positiveBroadcastNotification
 	tick()
 	c.Check(d.bcastCount, Equals, 1)
-
-	//  * session.ErrCh to the error handler
-	cli.errCh <- nil
-	tick()
-	c.Check(cs.log.Captured(), Matches, "(?ms).*session exited.*")
 }
 
 /*****************************************************************

=== modified file 'client/session/session.go'
--- client/session/session.go	2015-03-12 12:05:40 +0000
+++ client/session/session.go	2015-03-17 16:47:16 +0000
@@ -70,10 +70,12 @@
 
 const (
 	Error ClientSessionState = iota
+	Pristine
 	Disconnected
 	Connected
 	Started
 	Running
+	Shutdown
 	Unknown
 )
 
@@ -83,10 +85,12 @@
 	}
 	return [Unknown]string{
 		"Error",
+		"Pristine",
 		"Disconnected",
 		"Connected",
 		"Started",
 		"Running",
+		"Shutdown",
 	}[s]
 }
 
@@ -118,7 +122,6 @@
 	AuthGetter             func(string) string
 	AuthURL                string
 	AddresseeChecker       AddresseeChecking
-	ErrCh                  chan error
 	BroadcastCh            chan *BroadcastNotification
 	NotificationsCh        chan AddressedNotification
 }
@@ -126,9 +129,11 @@
 // ClientSession holds a client<->server session and its configuration.
 type ClientSession interface {
 	Close()
-	AutoRedial(doneCh chan uint32)
 	ClearCookie()
 	State() ClientSessionState
+	HasConnectivity(bool) error
+	KeepConnection() error
+	StopKeepConnection()
 }
 
 type clientSession struct {
@@ -169,6 +174,20 @@
 	redialJitter    func(time.Duration) time.Duration
 	redialDelays    []time.Duration
 	redialDelaysIdx int
+	// connection events come in over here
+	connCh chan bool
+	// last seen connection event is here
+	lastConn bool
+	// connection events are handled by this
+	connHandler func(bool)
+	// autoredial goes over here (xxx spurious goroutine involved)
+	doneCh chan uint32
+	// main loop errors out through here (possibly another spurious goroutine)
+	errCh chan error
+	// main loop errors are handled by this
+	errHandler func(error)
+	// look, a stopper!
+	stopCh chan struct{}
 }
 
 func redialDelay(sess *clientSession) time.Duration {
@@ -207,10 +226,10 @@
 		Protocolator:        protocol.NewProtocol0,
 		SeenState:           seenState,
 		TLS:                 &tls.Config{},
-		state:               Disconnected,
+		state:               Pristine,
 		timeSince:           time.Since,
 		shouldDelayP:        &shouldDelay,
-		redialDelay:         redialDelay,
+		redialDelay:         redialDelay, // NOTE there are tests that use calling sess.redialDelay as an indication of calling autoRedial!
 		redialDelays:        util.Timeouts(),
 	}
 	sess.redialJitter = sess.Jitter
@@ -222,6 +241,15 @@
 		}
 		sess.TLS.RootCAs = cp
 	}
+	sess.doneCh = make(chan uint32, 1)
+	sess.stopCh = make(chan struct{})
+	sess.connCh = make(chan bool, 1)
+	sess.errCh = make(chan error, 1)
+
+	// to be overridden by tests
+	sess.connHandler = sess.handleConn
+	sess.errHandler = sess.handleErr
+
 	return sess, nil
 }
 
@@ -383,7 +411,7 @@
 	}
 }
 
-func (sess *clientSession) AutoRedial(doneCh chan uint32) {
+func (sess *clientSession) autoRedial() {
 	sess.stopRedial()
 	if time.Since(sess.lastAutoRedial) < 2*time.Second {
 		sess.setShouldDelay()
@@ -405,7 +433,7 @@
 			return
 		}
 		sess.Log.Debugf("session autoredialier launching Redial goroutine")
-		doneCh <- retrier.Redial()
+		sess.doneCh <- retrier.Redial()
 	}()
 }
 
@@ -657,7 +685,7 @@
 	if err == nil {
 		err = starter()
 		if err == nil {
-			go func() { sess.ErrCh <- looper() }()
+			go func() { sess.errCh <- looper() }()
 		}
 	}
 	return err
@@ -684,6 +712,77 @@
 	return sess.run(sess.doClose, sess.addAuthorization, sess.getHosts, sess.connect, sess.start, sess.loop)
 }
 
+func (sess *clientSession) doKeepConnection() {
+Loop:
+	for {
+		select {
+		case hasConn := <-sess.connCh:
+			sess.connHandler(hasConn)
+		case <-sess.stopCh:
+			sess.Log.Infof("session shutting down.")
+			sess.connLock.Lock()
+			defer sess.connLock.Unlock()
+			sess.stopRedial()
+			if sess.Connection != nil {
+				sess.Connection.Close()
+				sess.Connection = nil
+			}
+			break Loop
+		case n := <-sess.doneCh:
+			sess.Log.Debugf("connected after %d attempts.", n)
+		case err := <-sess.errCh:
+			sess.errHandler(err)
+		}
+	}
+}
+
+func (sess *clientSession) handleConn(hasConn bool) {
+	sess.lastConn = hasConn
+
+	// Note this does not depend on the current state!  That's because Dial
+	// starts with doClose, which gets you to Disconnected even if you're
+	// connected, and you can call Close when Disconnected without it
+	// losing its stuff.
+	if hasConn {
+		sess.autoRedial()
+	} else {
+		sess.Close()
+	}
+}
+
+func (sess *clientSession) handleErr(err error) {
+	sess.Log.Errorf("session error'ed out with %v", err)
+	sess.stateLock.Lock()
+	if sess.state == Disconnected && sess.lastConn {
+		sess.autoRedial()
+	}
+	sess.stateLock.Unlock()
+}
+
+func (sess *clientSession) KeepConnection() error {
+	sess.stateLock.Lock()
+	defer sess.stateLock.Unlock()
+	if sess.state != Pristine {
+		return errors.New("don't call KeepConnection() on a non-pristine session.")
+	}
+	sess.state = Disconnected
+
+	go sess.doKeepConnection()
+
+	return nil
+}
+
+func (sess *clientSession) StopKeepConnection() {
+	sess.setState(Shutdown)
+	close(sess.stopCh)
+}
+
+func (sess *clientSession) HasConnectivity(hasConn bool) error {
+	sess.connCh <- hasConn
+	// XXX throw errors if called from weird state?
+	return nil
+}
+
 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	2015-02-13 11:20:41 +0000
+++ client/session/session_test.go	2015-03-17 16:47:16 +0000
@@ -190,6 +190,24 @@
 	cs.lvls = func() (seenstate.SeenState, error) { return seenstate.NewSqliteSeenState(":memory:") }
 }
 
+func (cs *clientSessionSuite) TestStateString(c *C) {
+	for _, i := range []struct {
+		v ClientSessionState
+		s string
+	}{
+		{Error, "Error"},
+		{Pristine, "Pristine"},
+		{Disconnected, "Disconnected"},
+		{Connected, "Connected"},
+		{Started, "Started"},
+		{Running, "Running"},
+		{Shutdown, "Shutdown"},
+		{Unknown, fmt.Sprintf("??? (%d)", Unknown)},
+	} {
+		c.Check(i.v.String(), Equals, i.s)
+	}
+}
+
 /****************************************************************
   parseServerAddrSpec() tests
 ****************************************************************/
@@ -214,7 +232,6 @@
 
 func dummyConf() ClientSessionConfig {
 	return ClientSessionConfig{
-		ErrCh:           make(chan error, 1),
 		BroadcastCh:     make(chan *BroadcastNotification, 5),
 		NotificationsCh: make(chan AddressedNotification, 5),
 	}
@@ -231,7 +248,9 @@
 	c.Check(sess.redialDelays, DeepEquals, util.Timeouts())
 	// but no root CAs set
 	c.Check(sess.TLS.RootCAs, IsNil)
-	c.Check(sess.State(), Equals, Disconnected)
+	c.Check(sess.State(), Equals, Pristine)
+	c.Check(sess.stopCh, NotNil)
+	c.Check(sess.connCh, NotNil)
 }
 
 func (cs *clientSessionSuite) TestNewSessionHostEndpointWorks(c *C) {
@@ -569,9 +588,9 @@
 	c.Check(ar.stopped, Equals, true)
 }
 
-/****************************************************************
-  AutoRedial() tests
-****************************************************************/
+// /****************************************************************
+//   AutoRedial() tests
+// ****************************************************************/
 
 func (cs *clientSessionSuite) TestAutoRedialWorks(c *C) {
 	// checks that AutoRedial sets up a retrier and tries redialing it
@@ -580,7 +599,8 @@
 	ar := new(derp)
 	sess.retrier = ar
 	c.Check(ar.stopped, Equals, false)
-	sess.AutoRedial(nil)
+	sess.autoRedial()
+	defer sess.stopRedial()
 	c.Check(ar.stopped, Equals, true)
 }
 
@@ -588,20 +608,21 @@
 	// checks that AutoRedial stops the previous retrier
 	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
 	c.Assert(err, IsNil)
-	ch := make(chan uint32)
+	sess.doneCh = make(chan uint32)
 	c.Check(sess.retrier, IsNil)
-	sess.AutoRedial(ch)
+	sess.autoRedial()
 	c.Assert(sess.retrier, NotNil)
 	sess.retrier.Stop()
-	c.Check(<-ch, Not(Equals), 0)
+	c.Check(<-sess.doneCh, Not(Equals), 0)
 }
 
 func (cs *clientSessionSuite) TestAutoRedialCallsRedialDelay(c *C) {
+	// NOTE there are tests that use calling redialDelay as an indication of calling autoRedial!
 	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
 	c.Assert(err, IsNil)
 	flag := false
 	sess.redialDelay = func(sess *clientSession) time.Duration { flag = true; return 0 }
-	sess.AutoRedial(nil)
+	sess.autoRedial()
 	c.Check(flag, Equals, true)
 }
 
@@ -609,11 +630,11 @@
 	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
 	c.Assert(err, IsNil)
 	sess.redialDelay = func(sess *clientSession) time.Duration { return 0 }
-	sess.AutoRedial(nil)
+	sess.autoRedial()
 	c.Check(sess.ShouldDelay(), Equals, false)
 	sess.stopRedial()
 	sess.clearShouldDelay()
-	sess.AutoRedial(nil)
+	sess.autoRedial()
 	c.Check(sess.ShouldDelay(), Equals, true)
 }
 
@@ -625,7 +646,6 @@
 	sess   *clientSession
 	upCh   chan interface{}
 	downCh chan interface{}
-	errCh  chan error
 }
 
 var _ = Suite(&msgSuite{})
@@ -637,7 +657,6 @@
 	s.sess, err = NewSession("", conf, "wah", seenstate.NewSeenState, helpers.NewTestLogger(c, "debug"))
 	c.Assert(err, IsNil)
 	s.sess.Connection = &testConn{Name: "TestHandle*"}
-	s.errCh = conf.ErrCh
 	s.upCh = make(chan interface{}, 5)
 	s.downCh = make(chan interface{}, 5)
 	s.sess.proto = &testProtocol{up: s.upCh, down: s.downCh}
@@ -696,10 +715,10 @@
 			json.RawMessage(`{"img1/m1":[102,"tubular"]}`),
 		},
 	}
-	go func() { s.errCh <- s.sess.handleBroadcast(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleBroadcast(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- nil // ack ok
-	c.Check(<-s.errCh, Equals, nil)
+	c.Check(<-s.sess.errCh, Equals, nil)
 	c.Assert(len(s.sess.BroadcastCh), Equals, 1)
 	c.Check(<-s.sess.BroadcastCh, DeepEquals, &BroadcastNotification{
 		TopLevel: 2,
@@ -728,11 +747,11 @@
 		TopLevel: 2,
 		Payloads: []json.RawMessage{json.RawMessage(`{"b":1}`)},
 	}
-	go func() { s.errCh <- s.sess.handleBroadcast(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleBroadcast(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	failure := errors.New("ACK ACK ACK")
 	s.upCh <- failure
-	c.Assert(<-s.errCh, Equals, failure)
+	c.Assert(<-s.sess.errCh, Equals, failure)
 	c.Check(s.sess.State(), Equals, Error)
 }
 
@@ -746,10 +765,10 @@
 		TopLevel: 2,
 		Payloads: []json.RawMessage{json.RawMessage(`{"b":1}`)},
 	}
-	go func() { s.errCh <- s.sess.handleBroadcast(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleBroadcast(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- nil // ack ok
-	c.Check(<-s.errCh, IsNil)
+	c.Check(<-s.sess.errCh, IsNil)
 	c.Check(len(s.sess.BroadcastCh), Equals, 0)
 }
 
@@ -764,10 +783,10 @@
 		TopLevel: 2,
 		Payloads: []json.RawMessage{json.RawMessage(`{"b":1}`)},
 	}
-	go func() { s.errCh <- s.sess.handleBroadcast(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleBroadcast(msg) }()
 	s.upCh <- nil // ack ok
 	// start returns with error
-	c.Check(<-s.errCh, Not(Equals), nil)
+	c.Check(<-s.sess.errCh, Not(Equals), nil)
 	c.Check(s.sess.State(), Equals, Error)
 	// no message sent out
 	c.Check(len(s.sess.BroadcastCh), Equals, 0)
@@ -780,10 +799,10 @@
 	s.sess.setShouldDelay()
 
 	msg := &serverMsg{Type: "broadcast"}
-	go func() { s.errCh <- s.sess.handleBroadcast(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleBroadcast(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- nil // ack ok
-	c.Check(<-s.errCh, IsNil)
+	c.Check(<-s.sess.errCh, IsNil)
 
 	c.Check(s.sess.ShouldDelay(), Equals, false)
 }
@@ -792,10 +811,10 @@
 	s.sess.setShouldDelay()
 
 	msg := &serverMsg{Type: "broadcast"}
-	go func() { s.errCh <- s.sess.handleBroadcast(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleBroadcast(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- errors.New("bcast")
-	c.Check(<-s.errCh, NotNil)
+	c.Check(<-s.sess.errCh, NotNil)
 
 	c.Check(s.sess.ShouldDelay(), Equals, true)
 }
@@ -845,10 +864,10 @@
 	msg.NotificationsMsg = protocol.NotificationsMsg{
 		Notifications: []protocol.Notification{n1, n2},
 	}
-	go func() { s.errCh <- s.sess.handleNotifications(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleNotifications(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- nil // ack ok
-	c.Check(<-s.errCh, Equals, nil)
+	c.Check(<-s.sess.errCh, Equals, nil)
 	c.Check(s.sess.ShouldDelay(), Equals, false)
 	c.Assert(s.sess.NotificationsCh, HasLen, 2)
 	app1, err := click.ParseAppId("com.example.app1_app1")
@@ -891,10 +910,10 @@
 	msg.NotificationsMsg = protocol.NotificationsMsg{
 		Notifications: []protocol.Notification{n1, n2},
 	}
-	go func() { s.errCh <- s.sess.handleNotifications(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleNotifications(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- nil // ack ok
-	c.Check(<-s.errCh, Equals, nil)
+	c.Check(<-s.sess.errCh, Equals, nil)
 	c.Check(s.sess.ShouldDelay(), Equals, false)
 	c.Assert(s.sess.NotificationsCh, HasLen, 1)
 	app2, err := click.ParseAppId("com.example.app2_app2")
@@ -926,10 +945,10 @@
 	msg.NotificationsMsg = protocol.NotificationsMsg{
 		Notifications: []protocol.Notification{n1, n2},
 	}
-	go func() { s.errCh <- s.sess.handleNotifications(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleNotifications(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- nil // ack ok
-	c.Check(<-s.errCh, Equals, nil)
+	c.Check(<-s.sess.errCh, Equals, nil)
 	c.Assert(s.sess.NotificationsCh, HasLen, 2)
 	app1, err := click.ParseAppId("com.example.app1_app1")
 	c.Assert(err, IsNil)
@@ -946,10 +965,10 @@
 	c.Check(ac.ops, HasLen, 3)
 
 	// second time they get ignored
-	go func() { s.errCh <- s.sess.handleNotifications(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleNotifications(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	s.upCh <- nil // ack ok
-	c.Check(<-s.errCh, Equals, nil)
+	c.Check(<-s.sess.errCh, Equals, nil)
 	c.Assert(s.sess.NotificationsCh, HasLen, 0)
 	c.Check(ac.ops, HasLen, 4)
 }
@@ -966,11 +985,11 @@
 	msg.NotificationsMsg = protocol.NotificationsMsg{
 		Notifications: []protocol.Notification{n1},
 	}
-	go func() { s.errCh <- s.sess.handleNotifications(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleNotifications(msg) }()
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	failure := errors.New("ACK ACK ACK")
 	s.upCh <- failure
-	c.Assert(<-s.errCh, Equals, failure)
+	c.Assert(<-s.sess.errCh, Equals, failure)
 	c.Check(s.sess.State(), Equals, Error)
 	// didn't get to clear
 	c.Check(s.sess.ShouldDelay(), Equals, true)
@@ -989,10 +1008,10 @@
 	msg.NotificationsMsg = protocol.NotificationsMsg{
 		Notifications: []protocol.Notification{n1},
 	}
-	go func() { s.errCh <- s.sess.handleNotifications(msg) }()
+	go func() { s.sess.errCh <- s.sess.handleNotifications(msg) }()
 	s.upCh <- nil // ack ok
 	// start returns with error
-	c.Check(<-s.errCh, Not(Equals), nil)
+	c.Check(<-s.sess.errCh, Not(Equals), nil)
 	c.Check(s.sess.State(), Equals, Error)
 	// no message sent out
 	c.Check(len(s.sess.NotificationsCh), Equals, 0)
@@ -1013,8 +1032,8 @@
 	msg.ConnBrokenMsg = protocol.ConnBrokenMsg{
 		Reason: "REASON",
 	}
-	go func() { s.errCh <- s.sess.handleConnBroken(msg) }()
-	c.Check(<-s.errCh, ErrorMatches, "server broke connection: REASON")
+	go func() { s.sess.errCh <- s.sess.handleConnBroken(msg) }()
+	c.Check(<-s.sess.errCh, ErrorMatches, "server broke connection: REASON")
 	c.Check(s.sess.State(), Equals, Error)
 }
 
@@ -1025,8 +1044,8 @@
 		Reason: protocol.BrokenHostMismatch,
 	}
 	s.sess.deliveryHosts = []string{"foo:443", "bar:443"}
-	go func() { s.errCh <- s.sess.handleConnBroken(msg) }()
-	c.Check(<-s.errCh, ErrorMatches, "server broke connection: host-mismatch")
+	go func() { s.sess.errCh <- s.sess.handleConnBroken(msg) }()
+	c.Check(<-s.sess.errCh, ErrorMatches, "server broke connection: host-mismatch")
 	c.Check(s.sess.State(), Equals, Error)
 	// hosts were reset
 	c.Check(s.sess.deliveryHosts, IsNil)
@@ -1044,14 +1063,14 @@
 	(*msgSuite)(s).SetUpTest(c)
 	s.sess.Connection.(*testConn).Name = "TestLoop*"
 	go func() {
-		s.errCh <- s.sess.loop()
+		s.sess.errCh <- s.sess.loop()
 	}()
 }
 
 func (s *loopSuite) TestLoopReadError(c *C) {
 	c.Check(s.sess.State(), Equals, Running)
 	s.upCh <- errors.New("Read")
-	err := <-s.errCh
+	err := <-s.sess.errCh
 	c.Check(err, ErrorMatches, "Read")
 	c.Check(s.sess.State(), Equals, Error)
 }
@@ -1063,7 +1082,7 @@
 	c.Check(takeNext(s.downCh), Equals, protocol.PingPongMsg{Type: "pong"})
 	failure := errors.New("pong")
 	s.upCh <- failure
-	c.Check(<-s.errCh, Equals, failure)
+	c.Check(<-s.sess.errCh, Equals, failure)
 }
 
 func (s *loopSuite) TestLoopLoopsDaLoop(c *C) {
@@ -1076,7 +1095,7 @@
 	}
 	failure := errors.New("pong")
 	s.upCh <- failure
-	c.Check(<-s.errCh, Equals, failure)
+	c.Check(<-s.sess.errCh, Equals, failure)
 }
 
 func (s *loopSuite) TestLoopBroadcast(c *C) {
@@ -1093,7 +1112,7 @@
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	failure := errors.New("ack")
 	s.upCh <- failure
-	c.Check(<-s.errCh, Equals, failure)
+	c.Check(<-s.sess.errCh, Equals, failure)
 }
 
 func (s *loopSuite) TestLoopNotifications(c *C) {
@@ -1113,7 +1132,7 @@
 	c.Check(takeNext(s.downCh), Equals, protocol.AckMsg{"ack"})
 	failure := errors.New("ack")
 	s.upCh <- failure
-	c.Check(<-s.errCh, Equals, failure)
+	c.Check(<-s.sess.errCh, Equals, failure)
 }
 
 func (s *loopSuite) TestLoopSetParams(c *C) {
@@ -1126,7 +1145,7 @@
 	s.upCh <- setParams
 	failure := errors.New("fail")
 	s.upCh <- failure
-	c.Assert(<-s.errCh, Equals, failure)
+	c.Assert(<-s.sess.errCh, Equals, failure)
 	c.Check(s.sess.getCookie(), Equals, "COOKIE")
 }
 
@@ -1138,7 +1157,7 @@
 	}
 	c.Check(takeNext(s.downCh), Equals, "deadline 1ms")
 	s.upCh <- broken
-	c.Check(<-s.errCh, NotNil)
+	c.Check(<-s.sess.errCh, NotNil)
 }
 
 func (s *loopSuite) TestLoopConnWarn(c *C) {
@@ -1159,7 +1178,7 @@
 	s.upCh <- warn
 	s.upCh <- connwarn
 	s.upCh <- failure
-	c.Check(<-s.errCh, Equals, failure)
+	c.Check(<-s.sess.errCh, Equals, failure)
 	c.Check(log.Captured(),
 		Matches, `(?ms).* warning: XXX$.*`)
 	c.Check(log.Captured(),
@@ -1426,12 +1445,12 @@
 		func() error { sess.BroadcastCh <- notf; return <-failureCh })
 	c.Check(err, Equals, nil)
 	// if run doesn't error it sets up the channels
-	c.Assert(sess.ErrCh, NotNil)
+	c.Assert(sess.errCh, NotNil)
 	c.Assert(sess.BroadcastCh, NotNil)
 	c.Check(<-sess.BroadcastCh, Equals, notf)
 	failure := errors.New("TestRunRunsEvenIfLoopFails")
 	failureCh <- failure
-	c.Check(<-sess.ErrCh, Equals, failure)
+	c.Check(<-sess.errCh, Equals, failure)
 	// so now you know it was running in a goroutine :)
 }
 
@@ -1632,7 +1651,7 @@
 	c.Check(takeNext(downCh), Equals, protocol.PingPongMsg{Type: "pong"})
 	failure := errors.New("pongs")
 	upCh <- failure
-	c.Check(<-sess.ErrCh, Equals, failure)
+	c.Check(<-sess.errCh, Equals, failure)
 }
 
 func (cs *clientSessionSuite) TestDialWorksDirect(c *C) {
@@ -1703,3 +1722,191 @@
 	sess.ClearCookie()
 	c.Check(sess.getCookie(), Equals, "")
 }
+
+/****************************************************************
+  KeepConnection() (and related) tests
+****************************************************************/
+
+func (cs *clientSessionSuite) TestKeepConnectionDoesNothingIfNotConnected(c *C) {
+	// how do you test "does nothing?"
+	sess, err := NewSession("foo:443", dummyConf(), "", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	c.Assert(sess, NotNil)
+	c.Assert(sess.State(), Equals, Pristine)
+	c.Assert(sess.KeepConnection(), IsNil)
+	// stopCh is meant to be used just for closing it, but abusing
+	// it for testing seems the right thing to do: this ensures
+	// the thing is ticking along before we check the state of
+	// stuff.
+	sess.stopCh <- struct{}{}
+	c.Check(sess.State(), Equals, Disconnected)
+}
+
+func (cs *clientSessionSuite) TestYouCantCallKeepConnectionTwice(c *C) {
+	sess, err := NewSession("foo:443", dummyConf(), "", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	c.Assert(sess, NotNil)
+	c.Assert(sess.State(), Equals, Pristine)
+	c.Assert(sess.KeepConnection(), IsNil)
+	defer sess.StopKeepConnection()
+	c.Check(sess.KeepConnection(), NotNil)
+}
+
+func (cs *clientSessionSuite) TestStopKeepConnectionShutsdown(c *C) {
+	sess, err := NewSession("foo:443", dummyConf(), "", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	c.Assert(sess, NotNil)
+	sess.StopKeepConnection()
+	c.Check(sess.State(), Equals, Shutdown)
+}
+
+func (cs *clientSessionSuite) TestHasConnectivityTriggersConnectivityHandler(c *C) {
+	sess, err := NewSession("foo:443", dummyConf(), "", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	c.Assert(sess, NotNil)
+	testCh := make(chan bool)
+	sess.connHandler = func(p bool) { testCh <- p }
+	go sess.doKeepConnection()
+	defer sess.StopKeepConnection()
+	sess.HasConnectivity(true)
+	c.Check(<-testCh, Equals, true)
+	sess.HasConnectivity(false)
+	c.Check(<-testCh, Equals, false)
+}
+
+func (cs *clientSessionSuite) TestDoneChIsEmptiedAndLogged(c *C) {
+	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	sess.doneCh = make(chan uint32) // unbuffered
+
+	sess.KeepConnection()
+	defer sess.StopKeepConnection()
+
+	sess.doneCh <- 23
+
+	c.Check(cs.log.Captured(),
+		Matches, `(?ms).* connected after 23 attempts\.`)
+}
+
+func (cs *clientSessionSuite) TestErrChIsEmptiedAndLoggedAndAutoRedial(c *C) {
+	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	ch := make(chan struct{}, 1)
+	sess.errCh = make(chan error) // unbuffered
+	sess.redialDelay = func(sess *clientSession) time.Duration { ch <- struct{}{}; return 0 }
+	sess.lastConn = true // -> autoRedial, if the session is in Disconnected
+
+	sess.KeepConnection()
+	defer sess.StopKeepConnection()
+
+	sess.errCh <- errors.New("potato")
+	c.Assert(sess.State(), Equals, Disconnected)
+	select {
+	case <-ch:
+		// all ok
+	case <-time.After(100 * time.Millisecond):
+		c.Fatalf("redialDelay not called (-> autoRedial not called)?")
+	}
+
+	c.Check(cs.log.Captured(),
+		Matches, `(?ms).* session error.*potato`)
+}
+
+func (cs *clientSessionSuite) TestErrChIsEmptiedAndLoggedNoAutoRedial(c *C) {
+	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	ch := make(chan struct{}, 1)
+	sess.errCh = make(chan error) // unbuffered
+	sess.redialDelay = func(sess *clientSession) time.Duration { ch <- struct{}{}; return 0 }
+	sess.connHandler = func(bool) {}
+	sess.lastConn = false // so, no autoredial
+
+	sess.KeepConnection()
+	defer sess.StopKeepConnection()
+
+	sess.errCh <- errors.New("potato")
+	c.Assert(sess.State(), Equals, Disconnected)
+	select {
+	case <-ch:
+		c.Fatalf("redialDelay called (-> autoRedial called) when disconnected?")
+	case <-time.After(100 * time.Millisecond):
+		// all ok
+	}
+
+	c.Check(cs.log.Captured(),
+		Matches, `(?ms).* session error.*potato`)
+}
+
+func (cs *clientSessionSuite) TestHandleConnConnFromConnected(c *C) {
+	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	ch := make(chan struct{}, 1)
+	sess.redialDelay = func(sess *clientSession) time.Duration { ch <- struct{}{}; return 0 }
+	sess.state = Connected
+	sess.lastConn = true
+	sess.handleConn(true)
+	c.Check(sess.lastConn, Equals, true)
+
+	select {
+	case <-ch:
+		// all ok
+	case <-time.After(100 * time.Millisecond):
+		c.Fatalf("redialDelay not called (-> autoRedial not called)?")
+	}
+}
+
+func (cs *clientSessionSuite) TestHandleConnConnFromDisconnected(c *C) {
+	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	ch := make(chan struct{}, 1)
+	sess.redialDelay = func(sess *clientSession) time.Duration { ch <- struct{}{}; return 0 }
+	sess.state = Disconnected
+	sess.lastConn = false
+	sess.handleConn(true)
+	c.Check(sess.lastConn, Equals, true)
+
+	select {
+	case <-ch:
+		// all ok
+	case <-time.After(100 * time.Millisecond):
+		c.Fatalf("redialDelay not called (-> autoRedial not called)?")
+	}
+}
+
+func (cs *clientSessionSuite) TestHandleConnNotConnFromDisconnected(c *C) {
+	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	ch := make(chan struct{}, 1)
+	sess.redialDelay = func(sess *clientSession) time.Duration { ch <- struct{}{}; return 0 }
+	sess.state = Disconnected
+	sess.lastConn = false
+	sess.handleConn(false)
+	c.Check(sess.lastConn, Equals, false)
+
+	select {
+	case <-ch:
+		c.Fatalf("redialDelay called (-> autoRedial called)?")
+	case <-time.After(100 * time.Millisecond):
+		// all ok
+	}
+	c.Check(cs.log.Captured(), Matches, `(?ms).*-> Disconnected`)
+}
+
+func (cs *clientSessionSuite) TestHandleConnNotConnFromConnected(c *C) {
+	sess, err := NewSession("", dummyConf(), "wah", cs.lvls, cs.log)
+	c.Assert(err, IsNil)
+	ch := make(chan struct{}, 1)
+	sess.redialDelay = func(sess *clientSession) time.Duration { ch <- struct{}{}; return 0 }
+	sess.state = Connected
+	sess.lastConn = true
+	sess.handleConn(false)
+	c.Check(sess.lastConn, Equals, false)
+
+	select {
+	case <-ch:
+		c.Fatalf("redialDelay called (-> autoRedial called)?")
+	case <-time.After(100 * time.Millisecond):
+		// all ok
+	}
+	c.Check(cs.log.Captured(), Matches, `(?ms).*-> Disconnected`)
+}