From c979f9254a9be60d4e53e9d55534fc5211b079a1 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 29 Jun 2021 18:42:34 -0400 Subject: [PATCH] internal/lsp/cache: invalidate packages in setMetadata When setting metadata, we must invalidate any packages that have been computed with old metadata. Also make setMetadata atomic, by locking around it in Load. This is just writing memory after a Load, so should be fast and infrequent. It is critical that our various maps related to metadata are coherent, so it makes sense to err on the side of coarser locking, IMO. Fix a race in TestUpgradeCodeLens. Change-Id: Iee8003b7e52b9f21681bdba08a009824f4ac6268 Reviewed-on: https://go-review.googlesource.com/c/tools/+/331809 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- .../regtest/codelens/codelens_test.go | 9 ++++++-- internal/lsp/cache/load.go | 21 ++++++++++++------- internal/lsp/cache/snapshot.go | 15 +++++++------ 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go index a2a1ae4558..d89b8e0bd8 100644 --- a/gopls/internal/regtest/codelens/codelens_test.go +++ b/gopls/internal/regtest/codelens/codelens_test.go @@ -166,11 +166,16 @@ require golang.org/x/hello v1.3.3 if vendoring { env.RunGoCommand("mod", "vendor") } + env.Await(env.DoneWithChangeWatchedFiles()) env.OpenFile("go.mod") env.ExecuteCodeLensCommand("go.mod", command.CheckUpgrades) d := &protocol.PublishDiagnosticsParams{} - env.Await(OnceMet(env.DiagnosticAtRegexpWithMessage("go.mod", `require`, "can be upgraded"), - ReadDiagnostics("go.mod", d))) + env.Await( + OnceMet( + env.DiagnosticAtRegexpWithMessage("go.mod", `require`, "can be upgraded"), + ReadDiagnostics("go.mod", d), + ), + ) env.ApplyQuickFixes("go.mod", d.Diagnostics) env.Await(env.DoneWithChangeWatchedFiles()) if got := env.Editor.BufferText("go.mod"); got != wantGoMod { diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 5a69875b90..1baa3e5ee2 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -194,7 +194,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf continue } // Set the metadata for this package. - m, err := s.setMetadata(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}) + s.mu.Lock() + m, err := s.setMetadataLocked(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}) + s.mu.Unlock() if err != nil { return err } @@ -388,10 +390,10 @@ func getWorkspaceDir(ctx context.Context, h *memoize.Handle, g *memoize.Generati return span.URIFromPath(v.(*workspaceDirData).dir), nil } -// setMetadata extracts metadata from pkg and records it in s. It +// setMetadataLocked extracts metadata from pkg and records it in s. It // recurses through pkg.Imports to ensure that metadata exists for all // dependencies. -func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *packages.Package, cfg *packages.Config, seen map[packageID]struct{}) (*metadata, error) { +func (s *snapshot) setMetadataLocked(ctx context.Context, pkgPath packagePath, pkg *packages.Package, cfg *packages.Config, seen map[packageID]struct{}) (*metadata, error) { id := packageID(pkg.ID) if _, ok := seen[id]; ok { return nil, errors.Errorf("import cycle detected: %q", id) @@ -429,7 +431,7 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa m.goFiles = append(m.goFiles, uri) uris[uri] = struct{}{} } - s.updateIDForURIs(id, uris) + s.updateIDForURIsLocked(id, uris) // TODO(rstambler): is this still necessary? copied := map[packageID]struct{}{ @@ -452,16 +454,14 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa m.missingDeps[importPkgPath] = struct{}{} continue } - if s.noValidMetadataForID(importID) { - if _, err := s.setMetadata(ctx, importPkgPath, importPkg, cfg, copied); err != nil { + if s.noValidMetadataForIDLocked(importID) { + if _, err := s.setMetadataLocked(ctx, importPkgPath, importPkg, cfg, copied); err != nil { event.Error(ctx, "error in dependency", err) } } } // Add the metadata to the cache. - s.mu.Lock() - defer s.mu.Unlock() // If we've already set the metadata for this snapshot, reuse it. if original, ok := s.metadata[m.id]; ok && original.valid { @@ -473,6 +473,11 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa metadata: m, valid: true, } + // Invalidate any packages we may have associated with this metadata. + for _, mode := range []source.ParseMode{source.ParseHeader, source.ParseExported, source.ParseFull} { + key := packageKey{mode, m.id} + delete(s.packages, key) + } } // Set the workspace packages. If any of the package's files belong to the diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 207be3a437..21ca384de4 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1114,19 +1114,22 @@ func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool { // noValidMetadataForID reports whether there is no valid metadata for the // given ID. func (s *snapshot) noValidMetadataForID(id packageID) bool { - m := s.getMetadata(id) + s.mu.Lock() + defer s.mu.Unlock() + return s.noValidMetadataForIDLocked(id) +} + +func (s *snapshot) noValidMetadataForIDLocked(id packageID) bool { + m := s.metadata[id] return m == nil || !m.valid } -// updateIDForURIs adds the given ID to the set of known IDs for the given URI. +// updateIDForURIsLocked adds the given ID to the set of known IDs for the given URI. // Any existing invalid IDs are removed from the set of known IDs. IDs that are // not "command-line-arguments" are preferred, so if a new ID comes in for a // URI that previously only had "command-line-arguments", the new ID will // replace the "command-line-arguments" ID. -func (s *snapshot) updateIDForURIs(id packageID, uris map[span.URI]struct{}) { - s.mu.Lock() - defer s.mu.Unlock() - +func (s *snapshot) updateIDForURIsLocked(id packageID, uris map[span.URI]struct{}) { for uri := range uris { // Collect the new set of IDs, preserving any valid existing IDs. newIDs := []packageID{id}