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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Cottrell <[email protected]>

Author: stamblerre

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.

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 {

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()