p2p/nat: fix concurrent access to autodisc Interface

Concurrent calls to Interface methods on autodisc could return a "not
discovered" error if the discovery did not finish before the call.
autodisc.wait expected the done channel to carry the found Interface
but it was closed instead.

The fix is to use sync.Once for now, which is easier to get right.
And there is a test. Finally.

This will have to change again when we introduce re-discovery.

p2p/nat/nat.go

@@ -172,8 +172,9 @@ func PMP(gateway net.IP) Interface {
 // This type is useful because discovery can take a while but we
 // want return an Interface value from UPnP, PMP and Auto immediately.
 type autodisc struct {
-	what string
-	done <-chan Interface
+	what string // type of interface being autodiscovered
+	once sync.Once
+	doit func() Interface
 
 	mu    sync.Mutex
 	found Interface
@@ -181,9 +182,10 @@ type autodisc struct {
 
 func startautodisc(what string, doit func() Interface) Interface {
 	// TODO: monitor network configuration and rerun doit when it changes.
-	done := make(chan Interface)
-	ad := &autodisc{what: what, done: done}
-	go func() { done <- doit(); close(done) }()
+	ad := &autodisc{what: what, doit: doit}
+	// Start the auto discovery as early as possible so it is already
+	// in progress when the rest of the stack calls the methods.
+	go ad.wait()
 	return ad
 }
 
@@ -218,19 +220,15 @@ func (n *autodisc) String() string {
 	}
 }
 
+// wait blocks until auto-discovery has been performed.
 func (n *autodisc) wait() error {
-	n.mu.Lock()
-	found := n.found
-	n.mu.Unlock()
-	if found != nil {
-		// already discovered
-		return nil
-	}
-	if found = <-n.done; found == nil {
-		return errors.New("no UPnP or NAT-PMP router discovered")
+	n.once.Do(func() {
+		n.mu.Lock()
+		n.found = n.doit()
+		n.mu.Unlock()
+	})
+	if n.found == nil {
+		return fmt.Errorf("no %s router discovered", n.what)
 	}
-	n.mu.Lock()
-	n.found = found
-	n.mu.Unlock()
 	return nil
 }

p2p/nat/nat_test.go

@@ -0,0 +1,48 @@
+package nat
+
+import (
+	"bytes"
+	"net"
+	"testing"
+	"time"
+)
+
+// This test checks that autodisc doesn't hang and returns
+// consistent results when multiple goroutines call its methods
+// concurrently.
+func TestAutoDiscRace(t *testing.T) {
+	ad := startautodisc("thing", func() Interface {
+		time.Sleep(500 * time.Millisecond)
+		return extIP{33, 44, 55, 66}
+	})
+
+	// Spawn a few concurrent calls to ad.ExternalIP.
+	type rval struct {
+		ip  net.IP
+		err error
+	}
+	results := make(chan rval, 50)
+	for i := 0; i < cap(results); i++ {
+		go func() {
+			ip, err := ad.ExternalIP()
+			results <- rval{ip, err}
+		}()
+	}
+
+	// Check that they all return the correct result within the deadline.
+	deadline := time.After(550 * time.Millisecond)
+	for i := 0; i < cap(results); i++ {
+		select {
+		case <-deadline:
+			t.Fatal("deadline exceeded")
+		case rval := <-results:
+			if rval.err != nil {
+				t.Errorf("result %d: unexpected error: %v", i, rval.err)
+			}
+			wantIP := net.IP{33, 44, 55, 66}
+			if !bytes.Equal(rval.ip, wantIP) {
+				t.Errorf("result %d: got IP %v, want %v", i, rval.ip, wantIP)
+			}
+		}
+	}
+}