Merge lp:~jonas-drange/ubuntu-push/poller-checks-send-recv into lp:ubuntu-push/automatic

[Jonas G. Drange]

Fix lp:1469398 by using the connectivity state. Fix case where a failed powerd wakeup request would deadlock step().

Fix lp:1469398 by using the connectivity state. Fix case where a failed powerd wakeup request would deadlock step().

[Samuele Pedroni (pedronis)]

see comment below

[Samuele Pedroni (pedronis)]

another one

review: Approve

[Samuele Pedroni (pedronis)]

review: Abstain

[Samuele Pedroni (pedronis)]

as discussed the new

p.requestedWakeupErrCh <- err

can probably be

filteredWakeUpCh <- true

without need new channels

[Jonas G. Drange (jonas-drange)]

You were right, Samuele. That did indeed work.

[Ubuntu One Auto Pilot (otto-pilot)]

Voting does not meet specified criteria. Required: Approve >= 1, Disapprove == 0, Needs Fixing == 0, Needs Information == 0, Resubmit == 0, Pending == 0. Got: .

[John Lenton (chipaca)]

LGTM!

review: Approve

=== modified file 'client/client.go'
--- client/client.go	2015-04-01 17:43:20 +0000
+++ client/client.go	2015-10-07 12:15:39 +0000
@@ -442,6 +442,11 @@
 	return nil
 }
 
+func (client *PushClient) handeConnNotification(conn bool) {
+	client.session.HasConnectivity(conn)
+	client.poller.HasConnectivity(conn)
+}
+
 // doLoop connects events with their handlers
 func (client *PushClient) doLoop(connhandler func(bool), bcasthandler func(*session.BroadcastNotification) error, ucasthandler func(session.AddressedNotification) error, unregisterhandler func(*click.AppId), accountshandler func()) {
 	for {
@@ -475,7 +480,7 @@
 
 // Loop calls doLoop with the "real" handlers
 func (client *PushClient) Loop() {
-	client.doLoop(client.session.HasConnectivity,
+	client.doLoop(client.handeConnNotification,
 		client.handleBroadcastNotification,
 		client.handleUnicastNotification,
 		client.handleUnregister,

=== modified file 'client/client_test.go'
--- client/client_test.go	2015-04-01 16:02:31 +0000
+++ client/client_test.go	2015-10-07 12:15:39 +0000
@@ -1032,6 +1032,7 @@
 ******************************************************************/
 
 type loopSession struct{ hasConn bool }
+type loopPoller struct{}
 
 func (s *loopSession) ResetCookie() {}
 func (s *loopSession) State() session.ClientSessionState {
@@ -1045,6 +1046,11 @@
 func (s *loopSession) KeepConnection() error        { return nil }
 func (s *loopSession) StopKeepConnection()          {}
 
+func (p *loopPoller) HasConnectivity(hasConn bool) {}
+func (p *loopPoller) IsConnected() bool            { return false }
+func (p *loopPoller) Start() error                 { return nil }
+func (p *loopPoller) Run() error                   { return nil }
+
 func (cs *clientSuite) TestLoop(c *C) {
 	cli := NewPushClient(cs.configPath, cs.leveldbPath)
 	cli.connCh = make(chan bool)
@@ -1070,6 +1076,7 @@
 	c.Assert(cli.session, NotNil)
 	cli.session.StopKeepConnection()
 	cli.session = &loopSession{}
+	cli.poller = &loopPoller{}
 
 	go cli.Loop()
 

=== modified file 'poller/poller.go'
--- poller/poller.go	2015-09-28 08:24:51 +0000
+++ poller/poller.go	2015-10-07 12:15:39 +0000
@@ -25,7 +25,6 @@
 	"time"
 
 	"launchpad.net/ubuntu-push/bus"
-	"launchpad.net/ubuntu-push/bus/networkmanager"
 	"launchpad.net/ubuntu-push/bus/polld"
 	"launchpad.net/ubuntu-push/bus/powerd"
 	"launchpad.net/ubuntu-push/client/session"
@@ -56,6 +55,7 @@
 	IsConnected() bool
 	Start() error
 	Run() error
+	HasConnectivity(bool)
 }
 
 type PollerSetup struct {
@@ -69,9 +69,9 @@
 	log                  logger.Logger
 	powerd               powerd.Powerd
 	polld                polld.Polld
-	nm                   networkmanager.NetworkManager
 	cookie               string
 	sessionState         stater
+	connCh               chan bool
 	requestWakeupCh      chan struct{}
 	requestedWakeupErrCh chan error
 	holdsWakeLockCh      chan bool
@@ -84,6 +84,7 @@
 		powerd:               nil,
 		polld:                nil,
 		sessionState:         setup.SessionStateGetter,
+		connCh:               make(chan bool),
 		requestWakeupCh:      make(chan struct{}),
 		requestedWakeupErrCh: make(chan error),
 		holdsWakeLockCh:      make(chan bool),
@@ -94,6 +95,10 @@
 	return p.sessionState.State() == session.Running
 }
 
+func (p *poller) HasConnectivity(hasConn bool) {
+	p.connCh <- hasConn
+}
+
 func (p *poller) Start() error {
 	if p.log == nil {
 		return ErrUnconfigured
@@ -104,10 +109,9 @@
 
 	powerdEndp := bus.SystemBus.Endpoint(powerd.BusAddress, p.log)
 	polldEndp := bus.SessionBus.Endpoint(polld.BusAddress, p.log)
-	nmEndp := bus.SystemBus.Endpoint(networkmanager.BusAddress, p.log)
 
 	var wg sync.WaitGroup
-	wg.Add(3)
+	wg.Add(2)
 	go func() {
 		n := util.NewAutoRedialer(powerdEndp).Redial()
 		p.log.Debugf("powerd dialed on try %d", n)
@@ -118,16 +122,10 @@
 		p.log.Debugf("polld dialed in on try %d", n)
 		wg.Done()
 	}()
-	go func() {
-		n := util.NewAutoRedialer(nmEndp).Redial()
-		p.log.Debugf("NetworkManager dialed on try %d", n)
-		wg.Done()
-	}()
 	wg.Wait()
 
 	p.powerd = powerd.New(powerdEndp, p.log)
 	p.polld = polld.New(polldEndp, p.log)
-	p.nm = networkmanager.New(nmEndp, p.log)
 
 	// busy sleep loop to workaround go's timer/sleep
 	// not accounting for time when the system is suspended
@@ -150,7 +148,7 @@
 	if p.log == nil {
 		return ErrUnconfigured
 	}
-	if p.powerd == nil || p.polld == nil || p.nm == nil {
+	if p.powerd == nil || p.polld == nil {
 		return ErrNotStarted
 	}
 	wakeupCh, err := p.powerd.WatchWakeups()
@@ -161,13 +159,8 @@
 	if err != nil {
 		return err
 	}
-	nmState := p.nm.GetState()
-	nmStateCh, _, err := p.nm.WatchState()
-	if err != nil {
-		return err
-	}
 	filteredWakeUpCh := make(chan bool)
-	go p.control(wakeupCh, filteredWakeUpCh, nmState, nmStateCh)
+	go p.control(wakeupCh, filteredWakeUpCh)
 	go p.run(filteredWakeUpCh, doneCh)
 	return nil
 }
@@ -185,10 +178,10 @@
 	return t, cookie, err
 }
 
-func (p *poller) control(wakeupCh <-chan bool, filteredWakeUpCh chan<- bool, nmState networkmanager.State, nmStateCh <-chan networkmanager.State) {
-	connected := nmState == networkmanager.ConnectedGlobal
+func (p *poller) control(wakeupCh <-chan bool, filteredWakeUpCh chan<- bool) {
+	// Assume a connection, and poll immediately.
+	connected := true
 	dontPoll := !connected
-	p.log.Debugf("nmState: %v, networkmanager.ConnectedGlobal: %v", nmState, networkmanager.ConnectedGlobal)
 	var t time.Time
 	cookie := ""
 	holdsWakeLock := false
@@ -225,11 +218,12 @@
 					filteredWakeUpCh <- true
 				}
 			}
-		case nmState = <-nmStateCh:
-			connected = nmState == networkmanager.ConnectedGlobal
+		case state := <-p.connCh:
+			connected = state
+			p.log.Debugf("control: connected:%v", state)
 		}
 		newDontPoll := !connected
-		p.log.Debugf("control: nmState:%v prevDontPoll:%v dontPoll:%v wakeupReq:%v holdsWakeLock:%v", nmState, dontPoll, newDontPoll, !t.IsZero(), holdsWakeLock)
+		p.log.Debugf("control: prevDontPoll:%v dontPoll:%v wakeupReq:%v holdsWakeLock:%v", dontPoll, newDontPoll, !t.IsZero(), holdsWakeLock)
 		if newDontPoll != dontPoll {
 			if dontPoll = newDontPoll; dontPoll {
 				if !t.IsZero() {
@@ -245,7 +239,12 @@
 			} else {
 				if t.IsZero() && !holdsWakeLock {
 					// reschedule soon
-					t, cookie, _ = p.doRequestWakeup(p.times.NetworkWait / 20)
+					var err error
+					t, cookie, err = p.doRequestWakeup(p.times.NetworkWait / 20)
+					if err != nil {
+						// Make sure we break a potential deadlock by trying again.
+						filteredWakeUpCh <- true
+					}
 				}
 			}
 		}
@@ -281,8 +280,6 @@
 	if lockCookie != "" {
 		if err := p.powerd.ClearWakelock(lockCookie); err != nil {
 			p.log.Errorf("ClearWakelock(%#v) got %v", lockCookie, err)
-		} else {
-			p.log.Debugf("cleared wakelock cookie %s.", lockCookie)
 		}
 		lockCookie = ""
 	}

=== modified file 'poller/poller_test.go'
--- poller/poller_test.go	2015-09-28 08:24:51 +0000
+++ poller/poller_test.go	2015-10-07 12:15:39 +0000
@@ -21,7 +21,6 @@
 
 	. "launchpad.net/gocheck"
 
-	"launchpad.net/ubuntu-push/bus/networkmanager"
 	"launchpad.net/ubuntu-push/client/session"
 	helpers "launchpad.net/ubuntu-push/testing"
 )
@@ -91,11 +90,6 @@
 	s.myd = &myD{}
 }
 
-const (
-	connectedGlobal    = networkmanager.ConnectedGlobal
-	disconnectedGlobal = networkmanager.Disconnected
-)
-
 func (s *PrSuite) TestStep(c *C) {
 	p := &poller{
 		times:                Times{},
@@ -106,6 +100,7 @@
 		requestWakeupCh:      make(chan struct{}),
 		requestedWakeupErrCh: make(chan error),
 		holdsWakeLockCh:      make(chan bool),
+		connCh:               make(chan bool),
 	}
 	s.myd.reqLockCookie = "wakelock cookie"
 	s.myd.stateState = session.Running
@@ -117,7 +112,7 @@
 	ch := make(chan string)
 	// now, run
 	filteredWakeUpCh := make(chan bool)
-	go p.control(wakeupCh, filteredWakeUpCh, connectedGlobal, nil)
+	go p.control(wakeupCh, filteredWakeUpCh)
 	go func() { ch <- p.step(filteredWakeUpCh, doneCh, "old cookie") }()
 	select {
 	case s := <-ch:
@@ -139,14 +134,15 @@
 		requestWakeupCh:      make(chan struct{}),
 		requestedWakeupErrCh: make(chan error),
 		holdsWakeLockCh:      make(chan bool),
+		connCh:               make(chan bool),
 	}
 	wakeUpCh := make(chan bool)
 	filteredWakeUpCh := make(chan bool)
 	s.myd.watchWakeCh = make(chan bool, 1)
-	nmStateCh := make(chan networkmanager.State)
-	go p.control(wakeUpCh, filteredWakeUpCh, connectedGlobal, nmStateCh)
+	go p.control(wakeUpCh, filteredWakeUpCh)
 
 	// works
+	p.HasConnectivity(true)
 	err := p.requestWakeup()
 	c.Assert(err, IsNil)
 	c.Check(<-s.myd.watchWakeCh, Equals, true)
@@ -160,17 +156,17 @@
 	wakeUpCh <- true
 	<-filteredWakeUpCh
 
-	nmStateCh <- disconnectedGlobal
+	p.HasConnectivity(false)
 	err = p.requestWakeup()
 	c.Assert(err, IsNil)
 	c.Check(s.myd.watchWakeCh, HasLen, 0)
 
 	// connected
-	nmStateCh <- connectedGlobal
+	p.HasConnectivity(true)
 	c.Check(<-s.myd.watchWakeCh, Equals, true)
 
 	// disconnected
-	nmStateCh <- disconnectedGlobal
+	p.HasConnectivity(false)
 	// pending wakeup was cleared
 	c.Check(<-s.myd.watchWakeCh, Equals, false)