internal/lsp: fix race condition in metadata handling
The metadata was being added to the cache before it was fully computed.
Change-Id: I6931476a715f0383f7739fa4e950dcaa6cbec4fe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204562
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/load.go b/internal/lsp/cache/load.go
index 855f598..3408d3f 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -45,7 +45,6 @@
if err == context.Canceled {
return nil, errors.Errorf("no metadata for %s: %v", uri.Filename(), err)
}
-
log.Print(ctx, "go/packages.Load", tag.Of("packages", len(pkgs)))
if len(pkgs) == 0 {
if err == nil {
@@ -149,7 +148,7 @@
log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles))
// Set the metadata for this package.
- if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg); err != nil {
+ if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}); err != nil {
return nil, nil, err
}
m := s.getMetadata(packageID(pkg.ID))
@@ -167,16 +166,21 @@
return results, prevMissingImports, nil
}
-func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *packages.Package, cfg *packages.Config) error {
+func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *packages.Package, cfg *packages.Config, seen map[packageID]struct{}) error {
+ id := packageID(pkg.ID)
+ if _, ok := seen[id]; ok {
+ return errors.Errorf("import cycle detected: %q", id)
+ }
// Recreate the metadata rather than reusing it to avoid locking.
m := &metadata{
- id: packageID(pkg.ID),
+ id: id,
pkgPath: pkgPath,
name: pkg.Name,
typesSizes: pkg.TypesSizes,
errors: pkg.Errors,
config: cfg,
}
+ seen[id] = struct{}{}
for _, filename := range pkg.CompiledGoFiles {
uri := span.FileURI(filename)
m.files = append(m.files, uri)
@@ -184,16 +188,14 @@
s.addID(uri, m.id)
}
- // Add the metadata to the cache.
- s.setMetadata(m)
-
+ copied := make(map[packageID]struct{})
+ for k, v := range seen {
+ copied[k] = v
+ }
for importPath, importPkg := range pkg.Imports {
importPkgPath := packagePath(importPath)
importID := packageID(importPkg.ID)
- if importPkgPath == pkgPath {
- return errors.Errorf("cycle detected in %s", importPath)
- }
m.deps = append(m.deps, importID)
// Don't remember any imports with significant errors.
@@ -206,10 +208,14 @@
}
dep := s.getMetadata(importID)
if dep == nil {
- if err := s.updateImports(ctx, importPkgPath, importPkg, cfg); err != nil {
+ if err := s.updateImports(ctx, importPkgPath, importPkg, cfg, copied); err != nil {
log.Error(ctx, "error in dependency", err)
}
}
}
+
+ // Add the metadata to the cache.
+ s.setMetadata(m)
+
return nil
}
diff --git a/internal/lsp/cmd/test/check.go b/internal/lsp/cmd/test/check.go
index 5fafa1f..ad260d2 100644
--- a/internal/lsp/cmd/test/check.go
+++ b/internal/lsp/cmd/test/check.go
@@ -61,7 +61,7 @@
}
_, found := got[expect]
if !found {
- t.Errorf("missing diagnostic %q", expect)
+ t.Errorf("missing diagnostic %q, %v", expect, got)
} else {
delete(got, expect)
}
diff --git a/internal/lsp/testdata/nodisk/nodisk.overlay.go b/internal/lsp/testdata/nodisk/nodisk.overlay.go
index f9194be..0831f0e 100644
--- a/internal/lsp/testdata/nodisk/nodisk.overlay.go
+++ b/internal/lsp/testdata/nodisk/nodisk.overlay.go
@@ -6,4 +6,4 @@
func _() {
foo.Foo() //@complete("F", Foo, IntFoo, StructFoo)
-}
+}
\ No newline at end of file
