From ed3ff1b6e41f7b4e3ef2e4670038ce11a3c23c22 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 18 Apr 2023 18:26:12 -0400 Subject: [PATCH] gopls/internal/lsp: finish debouncing on context cancellation While investigating golang/go#59475, I noticed that the gopls command-line is slowed down by the longer diagnostic delay, because shutdown awaits the diagnostics pass, which is not properly cancelled. Fix this by (1) cancelling the snapshot on shutdown, and (2) honoring context cancellation in debounce. This improvement revealed an underlying bug in the diagnose debouncing logic that made the rare flake from golang/go#57710 much more likely: we were using the view name and seqence ID as the debounce key, rather than a unique identifier for the view and/or a globally monotonic ID. Since this test changes and then reverts workspace folders, it was possible for the debouncer to skip the later diagnostics pass, thinking that it had already processed the same key with a higher sequence ID. Fix this redundantly by both using a unique ID for the view, and a globally monotonic sequence ID for the snapshot. Fixes golang/go#57710 Change-Id: I03ea550e561e6fdf6852dce9d844dbe69140cb8c Reviewed-on: https://go-review.googlesource.com/c/tools/+/486115 Run-TryBot: Robert Findley Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot gopls-CI: kokoro --- gopls/internal/lsp/cache/view.go | 1 + gopls/internal/lsp/debounce.go | 7 ++++++- gopls/internal/lsp/debounce_test.go | 18 +++++++++++++++++- gopls/internal/lsp/diagnostics.go | 8 ++++---- gopls/internal/lsp/regtest/env.go | 1 + gopls/internal/lsp/source/view.go | 3 +++ 6 files changed, 32 insertions(+), 6 deletions(-) diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index 0fa99e72d1c..160c72c6f19 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -577,6 +577,7 @@ func (v *View) shutdown() { v.snapshotMu.Lock() if v.snapshot != nil { + v.snapshot.cancel() v.releaseSnapshot() v.destroy(v.snapshot, "View.shutdown") v.snapshot = nil diff --git a/gopls/internal/lsp/debounce.go b/gopls/internal/lsp/debounce.go index 06f411471c6..bd59cf29acc 100644 --- a/gopls/internal/lsp/debounce.go +++ b/gopls/internal/lsp/debounce.go @@ -5,6 +5,7 @@ package lsp import ( + "context" "sync" "time" ) @@ -28,7 +29,10 @@ func newDebouncer() *debouncer { // debounce returns a channel that receives a boolean reporting whether, // by the time the delay channel receives a value, this call is (or will be) // the most recent call with the highest order number for its key. -func (d *debouncer) debounce(key string, order uint64, delay <-chan time.Time) <-chan bool { +// +// If ctx is done before the delay channel receives a value, the channel +// reports false. +func (d *debouncer) debounce(ctx context.Context, key string, order uint64, delay <-chan time.Time) <-chan bool { okc := make(chan bool, 1) d.mu.Lock() @@ -63,6 +67,7 @@ func (d *debouncer) debounce(key string, order uint64, delay <-chan time.Time) < } d.mu.Unlock() case <-done: + case <-ctx.Done(): } okc <- ok }() diff --git a/gopls/internal/lsp/debounce_test.go b/gopls/internal/lsp/debounce_test.go index b5597faf598..aac7e6c9d4a 100644 --- a/gopls/internal/lsp/debounce_test.go +++ b/gopls/internal/lsp/debounce_test.go @@ -5,6 +5,7 @@ package lsp import ( + "context" "testing" "time" ) @@ -56,6 +57,7 @@ func TestDebouncer(t *testing.T) { for _, test := range tests { test := test t.Run(test.label, func(t *testing.T) { + ctx := context.Background() d := newDebouncer() delays := make([]chan time.Time, len(test.events)) @@ -64,7 +66,7 @@ func TestDebouncer(t *testing.T) { // Register the events in deterministic order, synchronously. for i, e := range test.events { delays[i] = make(chan time.Time, 1) - okcs[i] = d.debounce(e.key, e.order, delays[i]) + okcs[i] = d.debounce(ctx, e.key, e.order, delays[i]) } // Now see which event fired. @@ -79,3 +81,17 @@ func TestDebouncer(t *testing.T) { }) } } + +func TestDebouncer_ContextCancellation(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + d := newDebouncer() + c := make(chan time.Time, 1) + + okc := d.debounce(ctx, "", 0, c) + cancel() + if ok := <-okc; ok { + t.Error("<-debounce(ctx, ...) returned true after cancellation") + } +} diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go index 17259ba7a92..20c797d85cb 100644 --- a/gopls/internal/lsp/diagnostics.go +++ b/gopls/internal/lsp/diagnostics.go @@ -150,6 +150,9 @@ func computeDiagnosticHash(diags ...*source.Diagnostic) string { return fmt.Sprintf("%x", h.Sum(nil)) } +// TODO(rfindley): is diagnoseDetached even necessary? We should always +// eventually diagnose after a change. I don't see the value in ensuring that +// the first diagnostics pass completes. func (s *Server) diagnoseDetached(snapshot source.Snapshot) { ctx := snapshot.BackgroundContext() ctx = xcontext.Detach(ctx) @@ -192,10 +195,7 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.U // // TODO(rfindley): it would be cleaner to simply put the diagnostic // debouncer on the view, and remove the "key" argument to debouncing. - // - // TODO(rfindley): debounce should accept a context, so that we don't hold onto - // the snapshot when the BackgroundContext is cancelled. - if ok := <-s.diagDebouncer.debounce(snapshot.View().Name(), snapshot.SequenceID(), time.After(delay)); ok { + if ok := <-s.diagDebouncer.debounce(ctx, snapshot.View().ID(), uint64(snapshot.GlobalID()), time.After(delay)); ok { s.diagnose(ctx, snapshot, analyzeOpenPackages) s.publishDiagnostics(ctx, true, snapshot) } diff --git a/gopls/internal/lsp/regtest/env.go b/gopls/internal/lsp/regtest/env.go index 91335bdab57..29cb28864d1 100644 --- a/gopls/internal/lsp/regtest/env.go +++ b/gopls/internal/lsp/regtest/env.go @@ -342,6 +342,7 @@ func (e *Env) Await(expectations ...Expectation) { // unmeetable. If it was met, OnceMet checks that the state meets all // expectations in mustMeets. func (e *Env) OnceMet(precondition Expectation, mustMeets ...Expectation) { + e.T.Helper() e.Await(OnceMet(precondition, mustMeets...)) } diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index baaca9d899a..affe556e831 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -308,6 +308,9 @@ func (m InvocationFlags) AllowNetwork() bool { // This is the level at which we maintain configuration like working directory // and build tags. type View interface { + // ID returns a globally unique identifier for this view. + ID() string + // Name returns the name this view was constructed with. Name() string