From 7421ba91ecf0b24ef9c0e01aaf3d6a9de43816e9 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Wed, 15 Sep 2021 12:10:15 -0700 Subject: [PATCH] ipn/ipnlocal: only call UpdateEndpoints when the endpoints change Signed-off-by: Josh Bleecher Snyder --- ipn/ipnlocal/local.go | 21 +++++++++++++++++++-- ipn/ipnlocal/state_test.go | 26 ++++++++------------------ 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 9a523fdaa..eb3423679 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -620,11 +620,16 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) { es := b.parseWgStatusLocked(s) cc := b.cc b.engineStatus = es - b.endpoints = append([]tailcfg.Endpoint{}, s.LocalAddrs...) + needUpdateEndpoints := !endpointsEqual(s.LocalAddrs, b.endpoints) + if needUpdateEndpoints { + b.endpoints = append([]tailcfg.Endpoint{}, s.LocalAddrs...) + } b.mu.Unlock() if cc != nil { - cc.UpdateEndpoints(0, s.LocalAddrs) + if needUpdateEndpoints { + cc.UpdateEndpoints(0, s.LocalAddrs) + } b.stateMachine() } @@ -635,6 +640,18 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) { b.send(ipn.Notify{Engine: &es}) } +func endpointsEqual(x, y []tailcfg.Endpoint) bool { + if len(x) != len(y) { + return false + } + for i := range x { + if x[i] != y[i] { + return false + } + } + return true +} + func (b *LocalBackend) SetNotifyCallback(notify func(ipn.Notify)) { b.mu.Lock() defer b.mu.Unlock() diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index c5973588a..b4f5c011c 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -412,11 +412,7 @@ func TestStateMachine(t *testing.T) { b.StartLoginInteractive() { nn := notifies.drain(1) - // BUG: UpdateEndpoints shouldn't be called yet. - // We're still not logged in so there's nothing we can do - // with it. (And empirically, it's providing an empty list - // of endpoints.) - c.Assert([]string{"UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"unpause"}, qt.DeepEquals, cc.getCalls()) c.Assert(nn[0].BrowseToURL, qt.Not(qt.IsNil)) c.Assert(url1, qt.Equals, *nn[0].BrowseToURL) } @@ -441,8 +437,7 @@ func TestStateMachine(t *testing.T) { url2 := "http://localhost:1/2" cc.send(nil, url2, false, nil) { - // BUG: UpdateEndpoints again, this is getting silly. - c.Assert([]string{"UpdateEndpoints", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"unpause", "unpause"}, qt.DeepEquals, cc.getCalls()) // This time, backend should emit it to the UI right away, // because the UI is anxiously awaiting a new URL to visit. @@ -462,8 +457,6 @@ func TestStateMachine(t *testing.T) { cc.send(nil, "", true, &netmap.NetworkMap{}) { nn := notifies.drain(3) - // BUG: still too soon for UpdateEndpoints. - // // Arguably it makes sense to unpause now, since the machine // authorization status is part of the netmap. // @@ -472,7 +465,7 @@ func TestStateMachine(t *testing.T) { // wait until it gets into Starting. // TODO: (Currently this test doesn't detect that bug, but // it's visible in the logs) - c.Assert([]string{"unpause", "unpause", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"unpause", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls()) c.Assert(nn[0].LoginFinished, qt.Not(qt.IsNil)) c.Assert(nn[1].Prefs, qt.Not(qt.IsNil)) c.Assert(nn[2].State, qt.Not(qt.IsNil)) @@ -494,7 +487,7 @@ func TestStateMachine(t *testing.T) { }) { nn := notifies.drain(1) - c.Assert([]string{"unpause", "unpause", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"unpause", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls()) c.Assert(nn[0].State, qt.Not(qt.IsNil)) c.Assert(ipn.Starting, qt.Equals, *nn[0].State) } @@ -534,9 +527,8 @@ func TestStateMachine(t *testing.T) { }) { nn := notifies.drain(2) - // BUG: UpdateEndpoints isn't needed here. // BUG: Login isn't needed here. We never logged out. - c.Assert([]string{"Login", "unpause", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"Login", "unpause", "unpause"}, qt.DeepEquals, cc.getCalls()) // BUG: I would expect Prefs to change first, and state after. c.Assert(nn[0].State, qt.Not(qt.IsNil)) c.Assert(nn[1].Prefs, qt.Not(qt.IsNil)) @@ -677,9 +669,8 @@ func TestStateMachine(t *testing.T) { c.Assert(b.Start(ipn.Options{StateKey: ipn.GlobalDaemonStateKey}), qt.IsNil) { // BUG: We already called Shutdown(), no need to do it again. - // BUG: Way too soon for UpdateEndpoints. // BUG: don't unpause because we're not logged in. - c.Assert([]string{"Shutdown", "unpause", "New", "UpdateEndpoints", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"Shutdown", "unpause", "New", "unpause"}, qt.DeepEquals, cc.getCalls()) nn := notifies.drain(2) c.Assert(cc.getCalls(), qt.HasLen, 0) @@ -739,13 +730,12 @@ func TestStateMachine(t *testing.T) { { // NOTE: cc.Shutdown() is correct here, since we didn't call // b.Shutdown() explicitly ourselves. - // BUG: UpdateEndpoints should be called here since we're not WantRunning. // Note: unpause happens because ipn needs to get at least one netmap // on startup, otherwise UIs can't show the node list, login // name, etc when in state ipn.Stopped. // Arguably they shouldn't try. But they currently do. nn := notifies.drain(2) - c.Assert([]string{"Shutdown", "unpause", "New", "UpdateEndpoints", "Login", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"Shutdown", "unpause", "New", "Login", "unpause"}, qt.DeepEquals, cc.getCalls()) c.Assert(cc.getCalls(), qt.HasLen, 0) c.Assert(nn[0].Prefs, qt.Not(qt.IsNil)) c.Assert(nn[1].State, qt.Not(qt.IsNil)) @@ -863,7 +853,7 @@ func TestStateMachine(t *testing.T) { { // NOTE: cc.Shutdown() is correct here, since we didn't call // b.Shutdown() ourselves. - c.Assert([]string{"Shutdown", "unpause", "New", "UpdateEndpoints", "Login", "unpause"}, qt.DeepEquals, cc.getCalls()) + c.Assert([]string{"Shutdown", "unpause", "New", "Login", "unpause"}, qt.DeepEquals, cc.getCalls()) nn := notifies.drain(1) c.Assert(cc.getCalls(), qt.HasLen, 0)