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>
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index d0ee975..b56be5f 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -192,8 +192,8 @@
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 @@
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 @@
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
diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go
index 36ec12f..0351aa6 100644
--- a/internal/lsp/cache/gofile.go
+++ b/internal/lsp/cache/gofile.go
@@ -8,6 +8,7 @@
"context"
"go/ast"
"go/token"
+ "sync"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/span"
@@ -17,7 +18,9 @@
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 @@
// 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 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 @@
// 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) astIsTrimmed() bool {
+ f.mu.Lock()
+ defer f.mu.Unlock()
+
return f.ast != nil && f.ast.isTrimmed
}
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 1368d56..913ef4a 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -20,59 +20,43 @@
// 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 @@
// 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 @@
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 @@
// 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 @@
// 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 @@
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 @@
// 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 @@
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
}
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 9088053..16a928e 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -203,7 +203,9 @@
return
}
// Mark file as open.
+ gof.mu.Lock()
gof.justOpened = true
+ gof.mu.Unlock()
}
}
}
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 76fade1..ec848a0 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -228,6 +228,12 @@
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 @@
// 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 @@
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 @@
},
}
v.session.filesWatchMap.Watch(uri, func() {
- f.(*goFile).invalidateContent()
+ gof, ok := f.(*goFile)
+ if !ok {
+ return
+ }
+ gof.invalidateContent()
})
}
v.mapFile(uri, f)
