diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 41d463851..ea5b90e24 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -161,6 +161,7 @@ func RegisterNewSSHServer(fn newSSHServerFunc) { type watchSession struct { ch chan *ipn.Notify sessionID string + cancel func() // call to signal that the session must be terminated } // LocalBackend is the glue between the major pieces of the Tailscale @@ -2635,7 +2636,15 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa } } - mak.Set(&b.notifyWatchers, sessionID, &watchSession{ch, sessionID}) + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + session := &watchSession{ + ch: ch, + sessionID: sessionID, + cancel: cancel, + } + mak.Set(&b.notifyWatchers, sessionID, session) b.mu.Unlock() defer func() { @@ -2666,8 +2675,6 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa // request every 2 seconds. // TODO(bradfitz): plumb this further and only send a Notify on change. if mask&ipn.NotifyWatchEngineUpdates != 0 { - ctx, cancel := context.WithCancel(ctx) - defer cancel() go b.pollRequestEngineStatus(ctx) } @@ -2680,7 +2687,7 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa select { case <-ctx.Done(): return - case n, ok := <-ch: + case n := <-ch: // URLs flow into Notify.BrowseToURL via two means: // 1. From MapResponse.PopBrowserURL, which already says they're dup // suppressed if identical, and that's done by the controlclient, @@ -2696,7 +2703,7 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa lastURLPop = v } } - if !ok || !fn(n) { + if !fn(n) { return } } diff --git a/ipn/ipnlocal/serve.go b/ipn/ipnlocal/serve.go index 9ad05a196..67d521f09 100644 --- a/ipn/ipnlocal/serve.go +++ b/ipn/ipnlocal/serve.go @@ -331,7 +331,7 @@ func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig, etag string if !has(k) { for _, sess := range b.notifyWatchers { if sess.sessionID == k { - close(sess.ch) + sess.cancel() } } } diff --git a/ipn/ipnlocal/serve_test.go b/ipn/ipnlocal/serve_test.go index 626615484..e43de1765 100644 --- a/ipn/ipnlocal/serve_test.go +++ b/ipn/ipnlocal/serve_test.go @@ -251,6 +251,14 @@ func TestServeConfigForeground(t *testing.T) { t.Fatal(err) } + // Introduce a race between [LocalBackend] sending notifications + // and [LocalBackend.WatchNotifications] shutting down due to + // setting the serve config below. + const N = 1000 + for range N { + go b.send(ipn.Notify{}) + } + // Setting a new serve config should shut down WatchNotifications // whose session IDs are no longer found: session1 goes, session2 stays. err = b.SetServeConfig(&ipn.ServeConfig{