ipn/ipnlocal: remove UnlockEarly from doSetHostinfoFilterServices

Pull the lock-bearing code into a closure, and use a clone rather than a
shallow copy of the hostinfo record.

Updates #11649

Change-Id: I4f1d42c42ce45e493b204baae0d50b1cbf82b102
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>

1 files changed, 22 insertions, 24 deletions

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index a1d2df24c..26f0155a1 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -4896,36 +4896,34 @@ func (b *LocalBackend) peerAPIServicesLocked() (ret []tailcfg.Service) {
 // TODO(danderson): we shouldn't be mangling hostinfo here after
 // painstakingly constructing it in twelvety other places.
 func (b *LocalBackend) doSetHostinfoFilterServices() {
-	unlock := b.lockAndGetUnlock()
-	defer unlock()
+	// Check the control client, hostinfo, and services under the mutex.
+	// On return, either both the client and hostinfo are nil, or both are non-nil.
+	// When non-nil, the Hostinfo is a clone of the value carried by b, safe to modify.
+	cc, hi, peerAPIServices := func() (controlclient.Client, *tailcfg.Hostinfo, []tailcfg.Service) {
+		b.mu.Lock()
+		defer b.mu.Unlock()
 
-	cc := b.cc
-	if cc == nil {
-		// Control client isn't up yet.
-		return
-	}
-	if b.hostinfo == nil {
-		b.logf("[unexpected] doSetHostinfoFilterServices with nil hostinfo")
+		if b.cc == nil {
+			return nil, nil, nil // control client isn't up yet
+		} else if b.hostinfo == nil {
+			b.logf("[unexpected] doSetHostinfoFilterServices with nil hostinfo")
+			return nil, nil, nil
+		}
+		svc := b.peerAPIServicesLocked()
+		if b.egg {
+			svc = append(svc, tailcfg.Service{Proto: "egg", Port: 1})
+		}
+		// Make a clone of hostinfo so we can mutate the service field, below.
+		return b.cc, b.hostinfo.Clone(), svc
+	}()
+	if cc == nil || hi == nil {
 		return
 	}
-	peerAPIServices := b.peerAPIServicesLocked()
-	if b.egg {
-		peerAPIServices = append(peerAPIServices, tailcfg.Service{Proto: "egg", Port: 1})
-	}
-
-	// TODO(maisem,bradfitz): store hostinfo as a view, not as a mutable struct.
-	hi := *b.hostinfo // shallow copy
-	unlock.UnlockEarly()
 
-	// Make a shallow copy of hostinfo so we can mutate
-	// at the Service field.
 	if !b.shouldUploadServices() {
 		hi.Services = []tailcfg.Service{}
 	}
-	// Don't mutate hi.Service's underlying array. Append to
-	// the slice with no free capacity.
-	c := len(hi.Services)
-	hi.Services = append(hi.Services[:c:c], peerAPIServices...)
+	hi.Services = append(hi.Services, peerAPIServices...)
 	hi.PushDeviceToken = b.pushDeviceToken.Load()
 
 	// Compare the expected ports from peerAPIServices to the actual ports in hi.Services.
@@ -4935,7 +4933,7 @@ func (b *LocalBackend) doSetHostinfoFilterServices() {
 		b.logf("Hostinfo peerAPI ports changed: expected %v, got %v", expectedPorts, actualPorts)
 	}
 
-	cc.SetHostinfo(&hi)
+	cc.SetHostinfo(hi)
 }
 
 type portPair struct {