internal/lsp/cache: fix race condition in snapshot's initializedErr

Use the snapshot's mutex to guard the initialize error. See the race
here: https://storage.googleapis.com/go-build-log/bb2fc21c/linux-amd64-race_8a495dc7.log.
Change-Id: I81628b19430fee318cd506ed86d12c6c007e71d1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/305789
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/snapshot.go

@@ -1115,8 +1115,11 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalErr
 	// TODO(rstambler): Should we be more careful about returning the
 	// initialization error? Is it possible for the initialization error to be
 	// corrected without a successful reinitialization?
-	if s.initializedErr != nil {
-		return s.initializedErr
+	s.mu.Lock()
+	initializedErr := s.initializedErr
+	s.mu.Unlock()
+	if initializedErr != nil {
+		return initializedErr
 	}
 
 	if ctx.Err() != nil {

internal/lsp/cache/view.go

@@ -527,78 +527,85 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) {
 		return
 	}
 	s.initializeOnce.Do(func() {
-		defer func() {
-			s.initializeOnce = nil
-			if firstAttempt {
-				close(s.view.initialWorkspaceLoad)
-			}
-		}()
-
-		// If we have multiple modules, we need to load them by paths.
-		var scopes []interface{}
-		var modDiagnostics []*source.Diagnostic
-		addError := func(uri span.URI, err error) {
-			modDiagnostics = append(modDiagnostics, &source.Diagnostic{
-				URI:      uri,
-				Severity: protocol.SeverityError,
-				Source:   source.ListError,
-				Message:  err.Error(),
-			})
-		}
-		if len(s.workspace.getActiveModFiles()) > 0 {
-			for modURI := range s.workspace.getActiveModFiles() {
-				fh, err := s.GetFile(ctx, modURI)
-				if err != nil {
-					addError(modURI, err)
-					continue
-				}
-				parsed, err := s.ParseMod(ctx, fh)
-				if err != nil {
-					addError(modURI, err)
-					continue
-				}
-				if parsed.File == nil || parsed.File.Module == nil {
-					addError(modURI, fmt.Errorf("no module path for %s", modURI))
-					continue
-				}
-				path := parsed.File.Module.Mod.Path
-				scopes = append(scopes, moduleLoadScope(path))
-			}
-		} else {
-			scopes = append(scopes, viewLoadScope("LOAD_VIEW"))
-		}
-		var err error
-		if len(scopes) > 0 {
-			err = s.load(ctx, firstAttempt, append(scopes, packagePath("builtin"))...)
-		}
-		if ctx.Err() != nil {
-			return
-		}
-		if err != nil {
-			event.Error(ctx, "initial workspace load failed", err)
-			extractedDiags, _ := s.extractGoCommandErrors(ctx, err.Error())
-			s.initializedErr = &source.CriticalError{
-				MainError: err,
-				DiagList:  append(modDiagnostics, extractedDiags...),
-			}
-		} else if len(modDiagnostics) == 1 {
-			s.initializedErr = &source.CriticalError{
-				MainError: fmt.Errorf(modDiagnostics[0].Message),
-				DiagList:  modDiagnostics,
-			}
-		} else if len(modDiagnostics) > 1 {
-			s.initializedErr = &source.CriticalError{
-				MainError: fmt.Errorf("error loading module names"),
-				DiagList:  modDiagnostics,
-			}
-		} else {
-			// Clear out the initialization error, in case it had been set
-			// previously.
-			s.initializedErr = nil
-		}
+		s.loadWorkspace(ctx, firstAttempt)
 	})
 }
 
+func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) {
+	defer func() {
+		s.initializeOnce = nil
+		if firstAttempt {
+			close(s.view.initialWorkspaceLoad)
+		}
+	}()
+
+	// If we have multiple modules, we need to load them by paths.
+	var scopes []interface{}
+	var modDiagnostics []*source.Diagnostic
+	addError := func(uri span.URI, err error) {
+		modDiagnostics = append(modDiagnostics, &source.Diagnostic{
+			URI:      uri,
+			Severity: protocol.SeverityError,
+			Source:   source.ListError,
+			Message:  err.Error(),
+		})
+	}
+	if len(s.workspace.getActiveModFiles()) > 0 {
+		for modURI := range s.workspace.getActiveModFiles() {
+			fh, err := s.GetFile(ctx, modURI)
+			if err != nil {
+				addError(modURI, err)
+				continue
+			}
+			parsed, err := s.ParseMod(ctx, fh)
+			if err != nil {
+				addError(modURI, err)
+				continue
+			}
+			if parsed.File == nil || parsed.File.Module == nil {
+				addError(modURI, fmt.Errorf("no module path for %s", modURI))
+				continue
+			}
+			path := parsed.File.Module.Mod.Path
+			scopes = append(scopes, moduleLoadScope(path))
+		}
+	} else {
+		scopes = append(scopes, viewLoadScope("LOAD_VIEW"))
+	}
+	var err error
+	if len(scopes) > 0 {
+		err = s.load(ctx, firstAttempt, append(scopes, packagePath("builtin"))...)
+	}
+	if ctx.Err() != nil {
+		return
+	}
+
+	var criticalErr *source.CriticalError
+	if err != nil {
+		event.Error(ctx, "initial workspace load failed", err)
+		extractedDiags, _ := s.extractGoCommandErrors(ctx, err.Error())
+		criticalErr = &source.CriticalError{
+			MainError: err,
+			DiagList:  append(modDiagnostics, extractedDiags...),
+		}
+	} else if len(modDiagnostics) == 1 {
+		criticalErr = &source.CriticalError{
+			MainError: fmt.Errorf(modDiagnostics[0].Message),
+			DiagList:  modDiagnostics,
+		}
+	} else if len(modDiagnostics) > 1 {
+		criticalErr = &source.CriticalError{
+			MainError: fmt.Errorf("error loading module names"),
+			DiagList:  modDiagnostics,
+		}
+	}
+
+	// Lock the snapshot when setting the initialized error.
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.initializedErr = criticalErr
+}
+
 // invalidateContent invalidates the content of a Go file,
 // including any position and type information that depends on it.
 func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]*fileChange, forceReloadMetadata bool) (*snapshot, func()) {