gopls/internal/server: fix a (mostly) benign race in diagnostics

In golang/go#65312, we observed a test flake due to what I believe is a
mostly benign race in diagnostics:

1. diagnoseSnapshot requests session.Views()
2. diagnoseSnapshot computes diagnostics
3. diagnoseSnapshot calls updateDiagnostics for the views from (1)

This means that if a view V were created and diagnosed between (1) and
(3), the call to updateDiagnostics will prune the diagnostics for V,
because it doesn't think it's an active view.

This is fundamentally a flaw in the design of multi-view diagnostics: we
don't a priori know which views apply to a given file, so we have to do
this complicated join. Nevertheless, I think this race is avoidable by
requesting session.Views() inside the critical section of
updateDiagnostics. In this way, it's not possible to overwrite
diagnostics reported by a different View, without also observing that
View.

Fixes golang/go#65312

Change-Id: Ie4116fbfac2837f7d142c7865758ff6bac92fce4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563957
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/server/diagnostics.go

@@ -178,7 +178,6 @@ func (s *server) diagnoseSnapshot(snapshot *cache.Snapshot, changedURIs []protoc
 	ctx, done := event.Start(ctx, "Server.diagnoseSnapshot", snapshot.Labels()...)
 	defer done()
 
-	allViews := s.session.Views()
 	if delay > 0 {
 		// 2-phase diagnostics.
 		//
@@ -207,7 +206,7 @@ func (s *server) diagnoseSnapshot(snapshot *cache.Snapshot, changedURIs []protoc
 				}
 				return
 			}
-			s.updateDiagnostics(ctx, allViews, snapshot, diagnostics, false)
+			s.updateDiagnostics(ctx, snapshot, diagnostics, false)
 		}
 
 		if delay < minDelay {
@@ -230,7 +229,7 @@ func (s *server) diagnoseSnapshot(snapshot *cache.Snapshot, changedURIs []protoc
 		}
 		return
 	}
-	s.updateDiagnostics(ctx, allViews, snapshot, diagnostics, true)
+	s.updateDiagnostics(ctx, snapshot, diagnostics, true)
 }
 
 func (s *server) diagnoseChangedFiles(ctx context.Context, snapshot *cache.Snapshot, uris []protocol.DocumentURI) (diagMap, error) {
@@ -671,10 +670,7 @@ func (s *server) updateCriticalErrorStatus(ctx context.Context, snapshot *cache.
 
 // updateDiagnostics records the result of diagnosing a snapshot, and publishes
 // any diagnostics that need to be updated on the client.
-//
-// The allViews argument should be the current set of views present in the
-// session, for the purposes of trimming diagnostics produced by deleted views.
-func (s *server) updateDiagnostics(ctx context.Context, allViews []*cache.View, snapshot *cache.Snapshot, diagnostics diagMap, final bool) {
+func (s *server) updateDiagnostics(ctx context.Context, snapshot *cache.Snapshot, diagnostics diagMap, final bool) {
 	ctx, done := event.Start(ctx, "Server.publishDiagnostics")
 	defer done()
 
@@ -697,8 +693,13 @@ func (s *server) updateDiagnostics(ctx context.Context, allViews []*cache.View,
 		return
 	}
 
+	// golang/go#65312: since the set of diagnostics depends on the set of views,
+	// we get the views *after* locking diagnosticsMu. This ensures that
+	// updateDiagnostics does not incorrectly delete diagnostics that have been
+	// set for an existing view that was created between the call to
+	// s.session.Views() and updateDiagnostics.
 	viewMap := make(viewSet)
-	for _, v := range allViews {
+	for _, v := range s.session.Views() {
 		viewMap[v] = unit{}
 	}