Merge lp:~chipaca/ubuntu-push/lets-go-to-the-races into lp:ubuntu-push/automatic

[John Lenton]

Fix race in setting up nm state watch.

Fix race in setting up nm state watch.

[lp:~chipaca/ubuntu-push/lets-go-to-the-races]

136. By John Lenton on 2014-04-28

make format

137. By John Lenton on 2014-04-28

fixed the README a bit.

138. By John Lenton on 2014-04-28

moved the lock/unlock pair to where it makes sense

[Samuele Pedroni (pedronis)]

review: Approve

=== modified file 'README'
--- README	2014-04-15 14:20:09 +0000
+++ README	2014-04-28 08:26:01 +0000
@@ -6,8 +6,9 @@
 The code expects to be checked out as launchpad.net/ubuntu-push in a Go
 workspace, see "go help gopath".
 
-To setup Go dependencies, install the following libraries:
+To setup Go dependencies, install the following dependencies:
 
+               build-essential
                libsqlite3-dev
                qtbase5-private-dev
                qtdeclarative5-dev
@@ -18,8 +19,8 @@
 
    make bootstrap
 
-To run tests, install libgcrypt11-dev, libwhoopsie-dev, and
-libubuntuoneauth-2.0-dev and run:
+To run tests, install libglib2.0-dev, libgcrypt11-dev, libwhoopsie-dev,
+and run:
 
    make check
 

=== modified file 'bus/connectivity/connectivity.go'
--- bus/connectivity/connectivity.go	2014-04-04 11:08:28 +0000
+++ bus/connectivity/connectivity.go	2014-04-28 08:26:01 +0000
@@ -72,19 +72,20 @@
 		cs.connAttempts += ar.Redial()
 		nm := networkmanager.New(cs.endp, cs.log)
 
+		// set up the watch
+		stateCh, err = nm.WatchState()
+		if err != nil {
+			cs.log.Debugf("failed to set up the state watch: %s", err)
+			goto Continue
+		}
+
 		// Get the current state.
 		initial = nm.GetState()
 		if initial == networkmanager.Unknown {
 			cs.log.Debugf("Failed to get state.")
 			goto Continue
 		}
-
-		// set up the watch
-		stateCh, err = nm.WatchState()
-		if err != nil {
-			cs.log.Debugf("failed to set up the state watch: %s", err)
-			goto Continue
-		}
+		cs.log.Debugf("got initial state of %s", initial)
 
 		primary = nm.GetPrimaryConnection()
 		cs.log.Debugf("primary connection starts as %#v", primary)

=== modified file 'bus/connectivity/connectivity_test.go'
--- bus/connectivity/connectivity_test.go	2014-04-04 12:01:42 +0000
+++ bus/connectivity/connectivity_test.go	2014-04-28 08:26:01 +0000
@@ -17,8 +17,15 @@
 package connectivity
 
 import (
+	"net/http/httptest"
+	"sync"
+	"testing"
+	"time"
+
 	"launchpad.net/go-dbus/v1"
 	. "launchpad.net/gocheck"
+
+	"launchpad.net/ubuntu-push/bus"
 	"launchpad.net/ubuntu-push/bus/networkmanager"
 	testingbus "launchpad.net/ubuntu-push/bus/testing"
 	"launchpad.net/ubuntu-push/config"
@@ -26,9 +33,6 @@
 	helpers "launchpad.net/ubuntu-push/testing"
 	"launchpad.net/ubuntu-push/testing/condition"
 	"launchpad.net/ubuntu-push/util"
-	"net/http/httptest"
-	"testing"
-	"time"
 )
 
 // hook up gocheck
@@ -115,6 +119,76 @@
 	c.Check(<-cs.networkStateCh, Equals, networkmanager.ConnectedGlobal)
 }
 
+// a racyEndpoint is an endpoint that behaves differently depending on
+// how much time passes between getting the state and setting up the
+// watch
+type racyEndpoint struct {
+	stateGot bool
+	maxTime  time.Time
+	delta    time.Duration
+	lock     sync.RWMutex
+}
+
+func (rep *racyEndpoint) GetProperty(prop string) (interface{}, error) {
+	switch prop {
+	case "state":
+		rep.lock.Lock()
+		defer rep.lock.Unlock()
+		rep.stateGot = true
+		rep.maxTime = time.Now().Add(rep.delta)
+		return uint32(networkmanager.Connecting), nil
+	case "PrimaryConnection":
+		return dbus.ObjectPath("/something"), nil
+	default:
+		return nil, nil
+	}
+}
+
+func (rep *racyEndpoint) WatchSignal(member string, f func(...interface{}), d func()) error {
+	if member == "StateChanged" {
+		// we count never having gotten the state as happening "after" now.
+		rep.lock.RLock()
+		defer rep.lock.RUnlock()
+		ok := !rep.stateGot || time.Now().Before(rep.maxTime)
+		go func() {
+			if ok {
+				f(uint32(networkmanager.ConnectedGlobal))
+			}
+			d()
+		}()
+	}
+	return nil
+}
+
+func (*racyEndpoint) Close()                                           {}
+func (*racyEndpoint) Dial() error                                      { return nil }
+func (*racyEndpoint) String() string                                   { return "racyEndpoint" }
+func (*racyEndpoint) Call(string, []interface{}, ...interface{}) error { return nil }
+
+var _ bus.Endpoint = (*racyEndpoint)(nil)
+
+// takeNext takes a value from given channel with a 1s timeout
+func takeNext(ch <-chan networkmanager.State) networkmanager.State {
+	select {
+	case <-time.After(time.Second):
+		panic("channel stuck: too long waiting")
+	case v := <-ch:
+		return v
+	}
+}
+
+// test that if the nm state goes from connecting to connected very
+// shortly after calling GetState, we don't lose the event.
+func (s *ConnSuite) TestStartAvoidsRace(c *C) {
+	for delta := time.Second; delta > 1; delta /= 2 {
+		rep := &racyEndpoint{delta: delta}
+		cs := connectedState{config: ConnectivityConfig{}, log: s.log, endp: rep}
+		f := Commentf("when delta=%s", delta)
+		c.Assert(cs.start(), Equals, networkmanager.Connecting, f)
+		c.Assert(takeNext(cs.networkStateCh), Equals, networkmanager.ConnectedGlobal, f)
+	}
+}
+
 /*
    tests for connectedStateStep()
 */