internal/lsp: fix race condition in type-checking

There has been a race condition that occasionally appears in test runs
on TryBots. Multiple threads perform type-checking, so they may race on
setting the fields of the *goFiles. Add a mutex to synchronize this.

Change-Id: If52c9d792c6504fc89044964998b06de7dfbd19c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183978
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

internal/lsp/cache/check.go

@@ -192,8 +192,8 @@ func (imp *importer) typeCheck(id packageID) (*pkg, error) {
 	return pkg, nil
 }
 
-func (imp *importer) cachePackage(ctx context.Context, p *pkg, meta *metadata, mode source.ParseMode) {
-	for _, file := range p.files {
+func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata, mode source.ParseMode) {
+	for _, file := range pkg.files {
 		f, err := imp.view.getFile(file.uri)
 		if err != nil {
 			imp.view.session.log.Errorf(ctx, "no file: %v", err)
@@ -204,35 +204,9 @@ func (imp *importer) cachePackage(ctx context.Context, p *pkg, meta *metadata, m
 			imp.view.session.log.Errorf(ctx, "%v is not a Go file", file.uri)
 			continue
 		}
-		// Set the package even if we failed to parse the file.
-		if gof.pkgs == nil {
-			gof.pkgs = make(map[packageID]*pkg)
+		if err := imp.cachePerFile(gof, file, pkg); err != nil {
+			imp.view.session.log.Errorf(ctx, "failed to cache file %s: %v", gof.URI(), err)
 		}
-		gof.pkgs[p.id] = p
-
-		// Get the AST for the file.
-		gof.ast = file
-		if gof.ast == nil {
-			imp.view.session.log.Errorf(ctx, "no AST information for %s", file.uri)
-			continue
-		}
-		if gof.ast.file == nil {
-			imp.view.session.log.Errorf(ctx, "no AST for %s: %v", file.uri, err)
-			continue
-		}
-		// Get the *token.File directly from the AST.
-		pos := gof.ast.file.Pos()
-		if !pos.IsValid() {
-			imp.view.session.log.Errorf(ctx, "AST for %s has an invalid position", file.uri)
-			continue
-		}
-		tok := imp.view.session.cache.FileSet().File(pos)
-		if tok == nil {
-			imp.view.session.log.Errorf(ctx, "no *token.File for %s", file.uri)
-			continue
-		}
-		gof.token = tok
-		gof.imports = gof.ast.file.Imports
 	}
 
 	// Set imports of package to correspond to cached packages.
@@ -243,10 +217,42 @@ func (imp *importer) cachePackage(ctx context.Context, p *pkg, meta *metadata, m
 		if err != nil {
 			continue
 		}
-		p.imports[importPkg.pkgPath] = importPkg
+		pkg.imports[importPkg.pkgPath] = importPkg
 	}
 }
 
+func (imp *importer) cachePerFile(gof *goFile, file *astFile, p *pkg) error {
+	gof.mu.Lock()
+	defer gof.mu.Unlock()
+
+	// Set the package even if we failed to parse the file.
+	if gof.pkgs == nil {
+		gof.pkgs = make(map[packageID]*pkg)
+	}
+	gof.pkgs[p.id] = p
+
+	// Get the AST for the file.
+	gof.ast = file
+	if gof.ast == nil {
+		return fmt.Errorf("no AST information for %s", file.uri)
+	}
+	if gof.ast.file == nil {
+		return fmt.Errorf("no AST for %s", file.uri)
+	}
+	// Get the *token.File directly from the AST.
+	pos := gof.ast.file.Pos()
+	if !pos.IsValid() {
+		return fmt.Errorf("AST for %s has an invalid position", file.uri)
+	}
+	tok := imp.view.session.cache.FileSet().File(pos)
+	if tok == nil {
+		return fmt.Errorf("no *token.File for %s", file.uri)
+	}
+	gof.token = tok
+	gof.imports = gof.ast.file.Imports
+	return nil
+}
+
 func (c *cache) appendPkgError(pkg *pkg, err error) {
 	if err == nil {
 		return

internal/lsp/cache/gofile.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"go/ast"
 	"go/token"
+	"sync"
 
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
@@ -17,7 +18,9 @@ import (
 type goFile struct {
 	fileBase
 
-	ast *astFile
+	// mu protects all mutable state of the Go file,
+	// which can be modified during type-checking.
+	mu sync.Mutex
 
 	// missingImports is the set of unresolved imports for this package.
 	// It contains any packages with `go list` errors.
@@ -28,9 +31,11 @@ type goFile struct {
 	// that we know about all of their packages.
 	justOpened bool
 
-	pkgs    map[packageID]*pkg
-	meta    map[packageID]*metadata
 	imports []*ast.ImportSpec
+
+	ast  *astFile
+	pkgs map[packageID]*pkg
+	meta map[packageID]*metadata
 }
 
 type astFile struct {
@@ -130,13 +135,16 @@ func (f *goFile) GetPackage(ctx context.Context) source.Package {
 }
 
 func unexpectedAST(ctx context.Context, f *goFile) bool {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
 	// If the AST comes back nil, something has gone wrong.
 	if f.ast == nil {
 		f.View().Session().Logger().Errorf(ctx, "expected full AST for %s, returned nil", f.URI())
 		return true
 	}
 	// If the AST comes back trimmed, something has gone wrong.
-	if f.astIsTrimmed() {
+	if f.ast.isTrimmed {
 		f.View().Session().Logger().Errorf(ctx, "expected full AST for %s, returned trimmed", f.URI())
 		return true
 	}
@@ -146,6 +154,9 @@ func unexpectedAST(ctx context.Context, f *goFile) bool {
 // isDirty is true if the file needs to be type-checked.
 // It assumes that the file's view's mutex is held by the caller.
 func (f *goFile) isDirty() bool {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
 	// If the the file has just been opened,
 	// it may be part of more packages than we are aware of.
 	//
@@ -166,6 +177,9 @@ func (f *goFile) isDirty() bool {
 }
 
 func (f *goFile) astIsTrimmed() bool {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
 	return f.ast != nil && f.ast.isTrimmed
 }
 

internal/lsp/cache/load.go

@@ -20,59 +20,43 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er
 	// If the AST for this file is trimmed, and we are explicitly type-checking it,
 	// don't ignore function bodies.
 	if f.astIsTrimmed() {
+		v.pcache.mu.Lock()
 		f.invalidateAST()
+		v.pcache.mu.Unlock()
 	}
-	// Save the metadata's current missing imports, if any.
-	originalMissingImports := f.missingImports
 
-	// Check if we need to run go/packages.Load for this file's package.
-	if errs, err := v.checkMetadata(ctx, f); err != nil {
+	// Get the metadata for the file.
+	meta, errs, err := v.checkMetadata(ctx, f)
+	if err != nil {
 		return errs, err
 	}
-
-	// If `go list` failed to get data for the file in question (this should never happen).
-	if len(f.meta) == 0 {
-		return nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename())
-	}
-
-	// If we have already seen these missing imports before, and we still have type information,
-	// there is no need to continue.
-	if sameSet(originalMissingImports, f.missingImports) && len(f.pkgs) > 0 {
+	if meta == nil {
 		return nil, nil
 	}
-
-	for id, meta := range f.meta {
-		if _, ok := f.pkgs[id]; ok {
-			continue
-		}
+	for id, m := range meta {
 		imp := &importer{
 			view:          v,
 			seen:          make(map[packageID]struct{}),
 			ctx:           ctx,
-			fset:          f.FileSet(),
-			topLevelPkgID: meta.id,
+			fset:          v.session.cache.FileSet(),
+			topLevelPkgID: id,
 		}
 		// Start prefetching direct imports.
-		for importID := range meta.children {
+		for importID := range m.children {
 			go imp.getPkg(importID)
 		}
 		// Type-check package.
-		pkg, err := imp.getPkg(meta.id)
+		pkg, err := imp.getPkg(imp.topLevelPkgID)
 		if err != nil {
 			return nil, err
 		}
 		if pkg == nil || pkg.IsIllTyped() {
-			return nil, fmt.Errorf("loadParseTypecheck: %s is ill typed", meta.pkgPath)
-		}
-		// If we still have not found the package for the file, something is wrong.
-		if f.pkgs[id] == nil {
-			v.Session().Logger().Errorf(ctx, "failed to type-check package %s", meta.pkgPath)
+			return nil, fmt.Errorf("loadParseTypecheck: %s is ill typed", m.pkgPath)
 		}
 	}
 	if len(f.pkgs) == 0 {
-		return nil, fmt.Errorf("loadParseTypeCheck: no packages found for %v", f.filename())
+		return nil, fmt.Errorf("no packages found for %s", f.URI())
 	}
-
 	return nil, nil
 }
 
@@ -90,9 +74,15 @@ func sameSet(x, y map[packagePath]struct{}) bool {
 
 // checkMetadata determines if we should run go/packages.Load for this file.
 // If yes, update the metadata for the file and its package.
-func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error, error) {
+func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*metadata, []packages.Error, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	// Save the metadata's current missing imports, if any.
+	originalMissingImports := f.missingImports
+
 	if !v.parseImports(ctx, f) {
-		return nil, nil
+		return f.meta, nil, nil
 	}
 
 	// Reset the file's metadata and type information if we are re-running `go list`.
@@ -109,7 +99,7 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error,
 			err = fmt.Errorf("go/packages.Load: no packages found for %s", f.filename())
 		}
 		// Return this error as a diagnostic to the user.
-		return []packages.Error{
+		return nil, []packages.Error{
 			{
 				Msg:  err.Error(),
 				Kind: packages.UnknownError,
@@ -125,7 +115,7 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error,
 		// If the package comes back with errors from `go list`,
 		// don't bother type-checking it.
 		if len(pkg.Errors) > 0 {
-			return pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath)
+			return nil, pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath)
 		}
 		for importPath, importPkg := range pkg.Imports {
 			if len(importPkg.Errors) > 0 {
@@ -138,7 +128,19 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error,
 		// Build the import graph for this package.
 		v.link(ctx, packagePath(pkg.PkgPath), pkg, nil)
 	}
-	return nil, nil
+
+	// If `go list` failed to get data for the file in question (this should never happen).
+	if len(f.meta) == 0 {
+		return nil, nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename())
+	}
+
+	// If we have already seen these missing imports before, and we still have type information,
+	// there is no need to continue.
+	if sameSet(originalMissingImports, f.missingImports) && len(f.pkgs) != 0 {
+		return nil, nil, nil
+	}
+
+	return f.meta, nil, nil
 }
 
 // reparseImports reparses a file's package and import declarations to
@@ -158,6 +160,7 @@ func (v *view) parseImports(ctx context.Context, f *goFile) bool {
 	if len(f.imports) != len(parsed.Imports) {
 		return true
 	}
+
 	for i, importSpec := range f.imports {
 		if importSpec.Path.Value != parsed.Imports[i].Path.Value {
 			return true
@@ -173,7 +176,9 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack
 	// If a file was added or deleted we need to invalidate the package cache
 	// so relevant packages get parsed and type-checked again.
 	if ok && !filenamesIdentical(m.files, pkg.CompiledGoFiles) {
-		v.invalidatePackage(id)
+		v.pcache.mu.Lock()
+		v.remove(id, make(map[packageID]struct{}))
+		v.pcache.mu.Unlock()
 	}
 
 	// If we haven't seen this package before.
@@ -234,18 +239,15 @@ func filenamesIdentical(oldFiles, newFiles []string) bool {
 	if len(oldFiles) != len(newFiles) {
 		return false
 	}
-
 	oldByName := make(map[string]struct{}, len(oldFiles))
 	for _, filename := range oldFiles {
 		oldByName[filename] = struct{}{}
 	}
-
 	for _, newFilename := range newFiles {
 		if _, found := oldByName[newFilename]; !found {
 			return false
 		}
 		delete(oldByName, newFilename)
 	}
-
 	return len(oldByName) == 0
 }

internal/lsp/cache/session.go

@@ -203,7 +203,9 @@ func (s *session) DidOpen(ctx context.Context, uri span.URI) {
 				return
 			}
 			// Mark file as open.
+			gof.mu.Lock()
 			gof.justOpened = true
+			gof.mu.Unlock()
 		}
 	}
 }

internal/lsp/cache/view.go

@@ -228,6 +228,12 @@ func (f *goFile) invalidateContent() {
 	f.handleMu.Lock()
 	defer f.handleMu.Unlock()
 
+	f.view.mcache.mu.Lock()
+	defer f.view.mcache.mu.Unlock()
+
+	f.view.pcache.mu.Lock()
+	defer f.view.pcache.mu.Unlock()
+
 	f.invalidateAST()
 	f.handle = nil
 }
@@ -235,29 +241,20 @@ func (f *goFile) invalidateContent() {
 // invalidateAST invalidates the AST of a Go file,
 // including any position and type information that depends on it.
 func (f *goFile) invalidateAST() {
-	f.view.pcache.mu.Lock()
-	defer f.view.pcache.mu.Unlock()
-
+	f.mu.Lock()
 	f.ast = nil
 	f.token = nil
+	pkgs := f.pkgs
+	f.mu.Unlock()
 
 	// Remove the package and all of its reverse dependencies from the cache.
-	for id, pkg := range f.pkgs {
+	for id, pkg := range pkgs {
 		if pkg != nil {
 			f.view.remove(id, map[packageID]struct{}{})
 		}
 	}
 }
 
-// invalidatePackage removes the specified package and dependents from the
-// package cache.
-func (v *view) invalidatePackage(id packageID) {
-	v.pcache.mu.Lock()
-	defer v.pcache.mu.Unlock()
-
-	v.remove(id, make(map[packageID]struct{}))
-}
-
 // remove invalidates a package and its reverse dependencies in the view's
 // package cache. It is assumed that the caller has locked both the mutexes
 // of both the mcache and the pcache.
@@ -278,7 +275,9 @@ func (v *view) remove(id packageID, seen map[packageID]struct{}) {
 	for _, filename := range m.files {
 		if f, _ := v.findFile(span.FileURI(filename)); f != nil {
 			if gof, ok := f.(*goFile); ok {
+				gof.mu.Lock()
 				delete(gof.pkgs, id)
+				gof.mu.Unlock()
 			}
 		}
 	}
@@ -341,7 +340,11 @@ func (v *view) getFile(uri span.URI) (viewFile, error) {
 			},
 		}
 		v.session.filesWatchMap.Watch(uri, func() {
-			f.(*goFile).invalidateContent()
+			gof, ok := f.(*goFile)
+			if !ok {
+				return
+			}
+			gof.invalidateContent()
 		})
 	}
 	v.mapFile(uri, f)