wgengine/magicsock: dial derp without holding send lockcrawshaw/derpdial

Dialing derp was blocking conn.Send, which bubbled up into
wireguard-go, which expects Send to behave like writing to
a typical socket with no blocking. The result fired the
watchdog timer in wgengine.

Instead, create the buffer channel for the derp server and
return it immediately, dialing on another goroutine.

Fixes #137

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>

1 files changed, 41 insertions, 18 deletions

diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index fd0b091d1..3713c5273 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -111,7 +111,7 @@ var derpMagicIP = net.ParseIP(DerpMagicIP).To4()
 
 // activeDerp contains fields for an active DERP connection.
 type activeDerp struct {
-	c         *derphttp.Client
+	c         *derphttp.Client // Conn.derpMu must be held
 	cancel    context.CancelFunc
 	writeCh   chan<- derpWriteRequest
 	lastWrite *time.Time
@@ -412,7 +412,9 @@ func (c *Conn) setNearestDERP(derpNum int) (wantDERP bool) {
 	c.myDerp = derpNum
 	c.logf("home DERP server is now %v, %v", derpNum, c.derps.ServerByID(derpNum))
 	for i, ad := range c.activeDerp {
-		go ad.c.NotePreferred(i == c.myDerp)
+		if ad.c != nil {
+			go ad.c.NotePreferred(i == c.myDerp)
+		}
 	}
 	if derpNum != 0 && derpNum != c.myDerp {
 		// On change, start connecting to it:
@@ -707,32 +709,51 @@ func (c *Conn) derpWriteChanOfAddr(addr *net.UDPAddr) chan<- derpWriteRequest {
 			return nil
 		}
 
-		// TODO(bradfitz): don't hold derpMu here. It's slow. Release first and use singleflight to dial+re-lock to add.
-		dc, err := derphttp.NewClient(c.privateKey, "https://"+derpSrv.HostHTTPS+"/derp", c.logf)
-		if err != nil {
-			c.logf("derphttp.NewClient: port %d, host %q invalid? err: %v", addr.Port, derpSrv.HostHTTPS, err)
-			return nil
-		}
-		dc.NotePreferred(c.myDerp == addr.Port)
-		dc.DNSCache = dnscache.Get()
-		dc.TLSConfig = c.derpTLSConfig
-
 		ctx, cancel := context.WithCancel(context.Background())
 		ch := make(chan derpWriteRequest, bufferedDerpWritesBeforeDrop)
-
-		ad.c = dc
 		ad.writeCh = ch
 		ad.cancel = cancel
 		ad.lastWrite = new(time.Time)
 		c.activeDerp[addr.Port] = ad
-
-		go c.runDerpReader(ctx, addr, dc)
-		go c.runDerpWriter(ctx, addr, dc, ch)
+		go c.dialDerp(ctx, addr, &ad, ch, derpSrv)
 	}
 	*ad.lastWrite = time.Now()
 	return ad.writeCh
 }
 
+func (c *Conn) dialDerp(ctx context.Context, addr *net.UDPAddr, ad *activeDerp, ch chan derpWriteRequest, derpSrv *derpmap.Server) {
+	dc, err := derphttp.NewClient(c.privateKey, "https://"+derpSrv.HostHTTPS+"/derp", c.logf)
+	if err != nil {
+		c.logf("derphttp.NewClient: port %d, host %q invalid? err: %v", addr.Port, derpSrv.HostHTTPS, err)
+
+		c.derpMu.Lock()
+		if ctx.Err() == nil {
+			ad.cancel()
+			delete(c.activeDerp, addr.Port)
+		}
+		c.derpMu.Unlock()
+
+		return
+	}
+	dc.NotePreferred(c.myDerp == addr.Port)
+	dc.DNSCache = dnscache.Get()
+	dc.TLSConfig = c.derpTLSConfig
+
+	c.derpMu.Lock()
+	if ctx.Err() != nil {
+		// Some other dialDerp beat us to the punch.
+		c.derpMu.Unlock()
+		dc.Close()
+		return
+	}
+	ad.c = dc
+	c.activeDerp[addr.Port] = *ad
+	c.derpMu.Unlock()
+
+	go c.runDerpReader(ctx, addr, dc)
+	go c.runDerpWriter(ctx, addr, dc, ch)
+}
+
 // derpReadResult is the type sent by runDerpClient to ReceiveIPv4
 // when a DERP packet is available.
 //
@@ -1035,7 +1056,9 @@ func (c *Conn) closeAllDerpLocked() {
 func (c *Conn) closeDerpLocked(node int) {
 	if ad, ok := c.activeDerp[node]; ok {
 		c.logf("closing connection to derp%v", node)
-		go ad.c.Close()
+		if ad.c != nil {
+			go ad.c.Close()
+		}
 		ad.cancel()
 		delete(c.activeDerp, node)
 	}