internal/lsp/cache: fix race condition in type-checking
We had previously been returning the metadata map in a few of the
loading functions. We don't actually need the map; we only need the
actual metadata. The race was caused by the return of the f.meta field
in a few functions, unprotected by the f.mu lock. This was likely a
result of the f.mu lock being added after the fact.
Fixes golang/go#33978
Change-Id: Ice5778d9d6dea23304237baf321b55d4fee6599c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193717
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go
index e34ec83..083d459 100644
--- a/internal/lsp/cache/gofile.go
+++ b/internal/lsp/cache/gofile.go
@@ -40,6 +40,15 @@
meta map[packageID]*metadata
}
+// metadata assumes that the caller holds the f.mu lock.
+func (f *goFile) metadata() []*metadata {
+ result := make([]*metadata, 0, len(f.meta))
+ for _, m := range f.meta {
+ result = append(result, m)
+ }
+ return result
+}
+
func (f *goFile) GetToken(ctx context.Context) (*token.File, error) {
file, err := f.GetAST(ctx, source.ParseFull)
if file == nil {
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index df4f938..9c4b242 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -46,7 +46,7 @@
return nil
}
-func (view *view) load(ctx context.Context, f *goFile) (map[packageID]*metadata, error) {
+func (view *view) load(ctx context.Context, f *goFile) ([]*metadata, error) {
view.mu.Lock()
defer view.mu.Unlock()
@@ -84,9 +84,16 @@
// 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) (map[packageID]*metadata, error) {
- if !v.shouldRunGopackages(ctx, f) {
- return f.meta, nil
+func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]*metadata, error) {
+ // Check if we need to re-run go/packages before loading the package.
+ f.mu.Lock()
+ runGopackages := v.shouldRunGopackages(ctx, f)
+ metadata := f.metadata()
+ f.mu.Unlock()
+
+ // The package metadata is correct as-is, so just return it.
+ if !runGopackages {
+ return metadata, nil
}
// Check if the context has been canceled before calling packages.Load.
@@ -127,7 +134,7 @@
return m, nil
}
-func validateMetadata(ctx context.Context, missingImports map[packagePath]struct{}, f *goFile) (map[packageID]*metadata, error) {
+func validateMetadata(ctx context.Context, missingImports map[packagePath]struct{}, f *goFile) ([]*metadata, error) {
f.mu.Lock()
defer f.mu.Unlock()
@@ -141,9 +148,11 @@
if sameSet(missingImports, f.missingImports) && len(f.pkgs) != 0 {
return nil, nil
}
+
// Otherwise, update the missing imports map.
f.missingImports = missingImports
- return f.meta, nil
+
+ return f.metadata(), nil
}
func sameSet(x, y map[packagePath]struct{}) bool {
@@ -158,10 +167,10 @@
return true
}
-// reparseImports reparses a file's package and import declarations to
+// shouldRunGopackages reparses a file's package and import declarations to
// determine if they have changed.
+// It assumes that the caller holds the lock on the f.mu lock.
func (v *view) shouldRunGopackages(ctx context.Context, f *goFile) (result bool) {
- f.mu.Lock()
defer func() {
// Clear metadata if we are intending to re-run go/packages.
if result {
@@ -173,7 +182,6 @@
delete(f.pkgs, k)
}
}
- f.mu.Unlock()
}()
if len(f.meta) == 0 || len(f.missingImports) > 0 {
