commit 79df971312162b7c79cfdf1725e6c479ee62ab8e
author Robert Findley <rfindley@google.com> Tue Apr 09 14:07:32 2024 -0400
committer Robert Findley <rfindley@google.com> Wed Apr 10 18:53:13 2024 +0000
tree 2c8fc25fd75a4ad0747f477e1c301e31258b7e9b
parent bcd607e0402e99fe759a9720a5614d3ae7c7663f

gopls/internal/server: avoid duplicate diagnoses and loads

With gopls@v0.15.0, zero config gopls made it much more likely that
sessions would have multiple Views. Additionally, with improved build
tag support, it made it more likely that these Views would share files.
As a result, we encountered (and fixed) this latent bug:

1. User changes file x.go, invalidating view A and B. A and B are
   scheduled for diagnosis.
2. User changes file y.go, invalidating only view B. Step (1) is
   cancelled and view B is scheduled for diagnosis.
3. View A never gets rediagnosed.

The fix was naive: just mark view A and B as dirty, and schedule a
goroutine to diagnose all dirty views after each step. As before, step
(2) would cancel the context from step (1).

But there's a problem: diagnoses were happening on the *Snapshot*
context, not the operation context. Therefore, if the goroutines of step
(1) and (2) both find the same snapshots, the diagnostics of step (1)
would *not* be cancelled, and would be performed in addition to the
diagnostics of (2). In other words, following a sequence of
invalidations, we could theoretically be collecting diagnostics N times
rather than 1 time.

In practice, this is not so much of a problem for smaller repositories.
Most of the time, changes are arriving at the speed of keystrokes, and
diagnostics are being scheduled faster than we can type. However, on
slower machines, or when there is more overall work being scheduled, or
when changes arrive simultaneously (such as with a 'save all' command or
branch switch), it is quite possible in practice to cause gopls to do
more work than necessary, including redundant loads. I'm not sure if
this is what conspires to cause the regressions described in
golang/go#66647, but it certainly is a real regression.

Fix this by threading the correct context into diagnoseSnapshot.
Additionally, add earlier context cancellation in a few cases where
redundant work was being performed despite a context cancellation.

For golang/go#66647

Change-Id: I67da1c186848286ca7b6221330a655d23820fd5d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577695
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>