From 60bb3025ec9a8bf33d1fe3ef47aadf127f2a9885 Mon Sep 17 00:00:00 2001
From: Rebecca Stambler <rstambler@golang.org>
Date: Wed, 14 Aug 2019 10:57:47 -0400
Subject: [PATCH] internal/lsp: fix race condition in caching

This change fixes a race condition in the metadata caching logic.
Also, some minor fixes to comments and invalidation logic (it's not
necessary to invalidate ASTs when a package is invalidated).

Change-Id: I927bf6fbc661a86ef0ba99b29a9ed979cd1eb95d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
---
 internal/lsp/cache/load.go  | 3 +++
 internal/lsp/cache/parse.go | 5 +++--
 internal/lsp/cache/view.go  | 7 -------
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 56ffde2050a..43018caa76d 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -229,10 +229,13 @@ func (v *view) link(ctx context.Context, g *importGraph) error {
 			log.Error(ctx, "not a Go file", nil, telemetry.File.Of(filename))
 			continue
 		}
+		// Cache the metadata for this file.
+		gof.mu.Lock()
 		if gof.meta == nil {
 			gof.meta = make(map[packageID]*metadata)
 		}
 		gof.meta[m.id] = m
+		gof.mu.Unlock()
 	}
 
 	// Preserve the import graph.
diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index ee830d3345c..3da4a6f238a 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -14,6 +14,7 @@ import (
 
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/lsp/telemetry"
+	"golang.org/x/tools/internal/lsp/telemetry/log"
 	"golang.org/x/tools/internal/lsp/telemetry/trace"
 	"golang.org/x/tools/internal/memoize"
 	errors "golang.org/x/xerrors"
@@ -105,7 +106,7 @@ func parseGo(ctx context.Context, c *cache, fh source.FileHandle, mode source.Pa
 
 	ioLimit <- struct{}{}
 	buf, _, err := fh.Read(ctx)
-	<-ioLimit // Make sure to release the token, even when an error is returned.
+	<-ioLimit
 	if err != nil {
 		return nil, err
 	}
@@ -122,7 +123,7 @@ func parseGo(ctx context.Context, c *cache, fh source.FileHandle, mode source.Pa
 		// Fix any badly parsed parts of the AST.
 		tok := c.fset.File(ast.Pos())
 		if err := fix(ctx, ast, tok, buf); err != nil {
-			// TODO: Do something with the error (need access to a logger in here).
+			log.Error(ctx, "failed to fix AST", err)
 		}
 	}
 	if ast == nil {
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index e4db451abe8..d4ff2670eec 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -409,13 +409,6 @@ func (v *view) remove(ctx context.Context, id packageID, seen map[packageID]stru
 				files:  hashParseKeys(cph.Files()),
 				config: hashConfig(cph.Config()),
 			})
-			// Also, delete all of the cached ParseGoHandles.
-			for _, ph := range cph.Files() {
-				v.session.cache.store.Delete(parseKey{
-					file: ph.File().Identity(),
-					mode: ph.Mode(),
-				})
-			}
 		}
 		delete(gof.pkgs, id)
 		gof.mu.Unlock()