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 <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>

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

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
 	}()

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")
+	}
+}

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)
 		}

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...))
 }
 

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