internal/lsp: fix deadlock in type-checking

There was a situation where we were trying to re-acquire a lock that was
already held. This change solves this issue.

Change-Id: I97cf6bad7e7c219a267e3ca5d174a2573f70ebe2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184217
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

internal/lsp/cache/check.go

@@ -119,7 +119,7 @@ func (imp *importer) typeCheck(id packageID) (*pkg, error) {
 	)
 	for _, filename := range meta.files {
 		uri := span.FileURI(filename)
-		f, err := imp.view.getFile(uri)
+		f, err := imp.view.getFile(imp.ctx, uri)
 		if err != nil {
 			continue
 		}
@@ -187,25 +187,25 @@ func (imp *importer) typeCheck(id packageID) (*pkg, error) {
 	check.Files(pkg.GetSyntax())
 
 	// Add every file in this package to our cache.
-	imp.cachePackage(imp.ctx, pkg, meta, mode)
+	if err := imp.cachePackage(imp.ctx, pkg, meta, mode); err != nil {
+		return nil, err
+	}
 
 	return pkg, nil
 }
 
-func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata, mode source.ParseMode) {
+func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata, mode source.ParseMode) error {
 	for _, file := range pkg.files {
-		f, err := imp.view.getFile(file.uri)
+		f, err := imp.view.getFile(ctx, file.uri)
 		if err != nil {
-			imp.view.session.log.Errorf(ctx, "no file: %v", err)
-			continue
+			return fmt.Errorf("no such file %s: %v", file.uri, err)
 		}
 		gof, ok := f.(*goFile)
 		if !ok {
-			imp.view.session.log.Errorf(ctx, "%v is not a Go file", file.uri)
-			continue
+			return fmt.Errorf("non Go file %s", file.uri)
 		}
 		if err := imp.cachePerFile(gof, file, pkg); err != nil {
-			imp.view.session.log.Errorf(ctx, "failed to cache file %s: %v", gof.URI(), err)
+			return fmt.Errorf("failed to cache file %s: %v", gof.URI(), err)
 		}
 	}
 
@@ -219,6 +219,8 @@ func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata,
 		}
 		pkg.imports[importPkg.pkgPath] = importPkg
 	}
+
+	return nil
 }
 
 func (imp *importer) cachePerFile(gof *goFile, file *astFile, p *pkg) error {

internal/lsp/cache/gofile.go

@@ -226,7 +226,7 @@ func (v *view) reverseDeps(ctx context.Context, seen map[packageID]struct{}, res
 	}
 	for _, filename := range m.files {
 		uri := span.FileURI(filename)
-		if f, err := v.getFile(uri); err == nil && v.session.IsOpen(uri) {
+		if f, err := v.getFile(ctx, uri); err == nil && v.session.IsOpen(uri) {
 			results[f.(*goFile)] = struct{}{}
 		}
 	}

internal/lsp/cache/load.go

@@ -21,7 +21,7 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er
 	// don't ignore function bodies.
 	if f.astIsTrimmed() {
 		v.pcache.mu.Lock()
-		f.invalidateAST()
+		f.invalidateAST(ctx)
 		v.pcache.mu.Unlock()
 	}
 
@@ -75,25 +75,20 @@ func sameSet(x, y map[packagePath]struct{}) bool {
 // 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, []packages.Error, error) {
-	f.mu.Lock()
-	defer f.mu.Unlock()
-
-	if !v.parseImports(ctx, f) {
+	filename, ok := v.runGopackages(ctx, f)
+	if !ok {
 		return f.meta, nil, nil
 	}
 
-	// Reset the file's metadata and type information if we are re-running `go list`.
-	for k := range f.meta {
-		delete(f.meta, k)
-	}
-	for k := range f.pkgs {
-		delete(f.pkgs, k)
+	// Check if the context has been canceled before calling packages.Load.
+	if ctx.Err() != nil {
+		return nil, nil, ctx.Err()
 	}
 
-	pkgs, err := packages.Load(v.buildConfig(), fmt.Sprintf("file=%s", f.filename()))
+	pkgs, err := packages.Load(v.buildConfig(), fmt.Sprintf("file=%s", filename))
 	if len(pkgs) == 0 {
 		if err == nil {
-			err = fmt.Errorf("go/packages.Load: no packages found for %s", f.filename())
+			err = fmt.Errorf("go/packages.Load: no packages found for %s", filename)
 		}
 		// Return this error as a diagnostic to the user.
 		return nil, []packages.Error{
@@ -103,7 +98,6 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*met
 			},
 		}, err
 	}
-
 	// Track missing imports as we look at the package's errors.
 	missingImports := make(map[packagePath]struct{})
 	for _, pkg := range pkgs {
@@ -119,52 +113,87 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*met
 			}
 		}
 		// Build the import graph for this package.
-		v.link(ctx, packagePath(pkg.PkgPath), pkg, nil)
+		if err := v.link(ctx, packagePath(pkg.PkgPath), pkg, nil); err != nil {
+			return nil, nil, err
+		}
 	}
+	m, err := validateMetadata(ctx, missingImports, f)
+	if err != nil {
+		return nil, nil, err
+	}
+	return m, nil, nil
+}
+
+func validateMetadata(ctx context.Context, missingImports map[packagePath]struct{}, f *goFile) (map[packageID]*metadata, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
 
 	// If `go list` failed to get data for the file in question (this should never happen).
 	if len(f.meta) == 0 {
-		return nil, nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename())
+		return nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename())
 	}
 
 	// If we have already seen these missing imports before, and we have type information,
 	// there is no need to continue.
 	if sameSet(missingImports, f.missingImports) && len(f.pkgs) != 0 {
-		return nil, nil, nil
+		return nil, nil
 	}
 	// Otherwise, update the missing imports map.
 	f.missingImports = missingImports
-
-	return f.meta, nil, nil
+	return f.meta, nil
 }
 
 // reparseImports reparses a file's package and import declarations to
 // determine if they have changed.
-func (v *view) parseImports(ctx context.Context, f *goFile) bool {
+func (v *view) runGopackages(ctx context.Context, f *goFile) (filename string, result bool) {
+	f.mu.Lock()
+	defer func() {
+		// Clear metadata if we are intending to re-run go/packages.
+		if result {
+			// Reset the file's metadata and type information if we are re-running `go list`.
+			for k := range f.meta {
+				delete(f.meta, k)
+			}
+			for k := range f.pkgs {
+				delete(f.pkgs, k)
+			}
+		}
+
+		defer f.mu.Unlock()
+	}()
+
 	if len(f.meta) == 0 || len(f.missingImports) > 0 {
-		return true
+		return f.filename(), true
 	}
 	// Get file content in case we don't already have it.
 	parsed, _ := v.session.cache.ParseGoHandle(f.Handle(ctx), source.ParseHeader).Parse(ctx)
 	if parsed == nil {
-		return true
+		return f.filename(), true
 	}
-	// TODO: Add support for re-running `go list` when the package name changes.
-
-	// If the package's imports have changed, re-run `go list`.
-	if len(f.imports) != len(parsed.Imports) {
-		return true
-	}
-
-	for i, importSpec := range f.imports {
-		if importSpec.Path.Value != parsed.Imports[i].Path.Value {
-			return true
+	// Check if the package's name has changed, by checking if this is a filename
+	// we already know about, and if so, check if its package name has changed.
+	for _, m := range f.meta {
+		for _, filename := range m.files {
+			if filename == f.URI().Filename() {
+				if m.name != parsed.Name.Name {
+					return f.filename(), true
+				}
+			}
 		}
 	}
-	return false
+	// If the package's imports have changed, re-run `go list`.
+	if len(f.imports) != len(parsed.Imports) {
+		return f.filename(), true
+	}
+	for i, importSpec := range f.imports {
+		if importSpec.Path.Value != parsed.Imports[i].Path.Value {
+			return f.filename(), true
+		}
+	}
+	return f.filename(), false
 }
 
-func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata) *metadata {
+func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata) error {
 	id := packageID(pkg.ID)
 	m, ok := v.mcache.packages[id]
 
@@ -172,7 +201,7 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack
 	// so relevant packages get parsed and type-checked again.
 	if ok && !filenamesIdentical(m.files, pkg.CompiledGoFiles) {
 		v.pcache.mu.Lock()
-		v.remove(id, make(map[packageID]struct{}))
+		v.remove(ctx, id, make(map[packageID]struct{}))
 		v.pcache.mu.Unlock()
 	}
 
@@ -192,16 +221,18 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack
 	m.name = pkg.Name
 	m.files = pkg.CompiledGoFiles
 	for _, filename := range m.files {
-		if f, _ := v.getFile(span.FileURI(filename)); f != nil {
-			if gof, ok := f.(*goFile); ok {
-				if gof.meta == nil {
-					gof.meta = make(map[packageID]*metadata)
-				}
-				gof.meta[m.id] = m
-			} else {
-				v.Session().Logger().Errorf(ctx, "not a Go file: %s", f.URI())
-			}
+		f, err := v.getFile(ctx, span.FileURI(filename))
+		if err != nil {
+			return err
 		}
+		gof, ok := f.(*goFile)
+		if !ok {
+			return fmt.Errorf("not a Go file: %s", f.URI())
+		}
+		if gof.meta == nil {
+			gof.meta = make(map[packageID]*metadata)
+		}
+		gof.meta[m.id] = m
 	}
 	// Connect the import graph.
 	if parent != nil {
@@ -209,8 +240,15 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack
 		parent.children[id] = true
 	}
 	for importPath, importPkg := range pkg.Imports {
+		importPkgPath := packagePath(importPath)
+		if importPkgPath == pkgPath {
+			v.session.log.Errorf(ctx, "cycle detected in %s", importPath)
+			return nil
+		}
 		if _, ok := m.children[packageID(importPkg.ID)]; !ok {
-			v.link(ctx, packagePath(importPath), importPkg, m)
+			if err := v.link(ctx, importPkgPath, importPkg, m); err != nil {
+				return err
+			}
 		}
 	}
 	// Clear out any imports that have been removed.
@@ -226,7 +264,7 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack
 		delete(m.children, importID)
 		delete(child.parents, id)
 	}
-	return m
+	return nil
 }
 
 // filenamesIdentical reports whether two sets of file names are identical.

internal/lsp/cache/view.go

@@ -224,7 +224,7 @@ func (v *view) SetContent(ctx context.Context, uri span.URI, content []byte) err
 
 // invalidateContent invalidates the content of a Go file,
 // including any position and type information that depends on it.
-func (f *goFile) invalidateContent() {
+func (f *goFile) invalidateContent(ctx context.Context) {
 	f.handleMu.Lock()
 	defer f.handleMu.Unlock()
 
@@ -234,13 +234,13 @@ func (f *goFile) invalidateContent() {
 	f.view.pcache.mu.Lock()
 	defer f.view.pcache.mu.Unlock()
 
-	f.invalidateAST()
+	f.invalidateAST(ctx)
 	f.handle = nil
 }
 
 // invalidateAST invalidates the AST of a Go file,
 // including any position and type information that depends on it.
-func (f *goFile) invalidateAST() {
+func (f *goFile) invalidateAST(ctx context.Context) {
 	f.mu.Lock()
 	f.ast = nil
 	f.token = nil
@@ -250,7 +250,7 @@ func (f *goFile) invalidateAST() {
 	// Remove the package and all of its reverse dependencies from the cache.
 	for id, pkg := range pkgs {
 		if pkg != nil {
-			f.view.remove(id, map[packageID]struct{}{})
+			f.view.remove(ctx, id, map[packageID]struct{}{})
 		}
 	}
 }
@@ -258,7 +258,7 @@ func (f *goFile) invalidateAST() {
 // remove invalidates a package and its reverse dependencies in the view's
 // package cache. It is assumed that the caller has locked both the mutexes
 // of both the mcache and the pcache.
-func (v *view) remove(id packageID, seen map[packageID]struct{}) {
+func (v *view) remove(ctx context.Context, id packageID, seen map[packageID]struct{}) {
 	if _, ok := seen[id]; ok {
 		return
 	}
@@ -268,20 +268,27 @@ func (v *view) remove(id packageID, seen map[packageID]struct{}) {
 	}
 	seen[id] = struct{}{}
 	for parentID := range m.parents {
-		v.remove(parentID, seen)
+		v.remove(ctx, parentID, seen)
 	}
 	// All of the files in the package may also be holding a pointer to the
 	// invalidated package.
 	for _, filename := range m.files {
-		if f, _ := v.findFile(span.FileURI(filename)); f != nil {
-			if gof, ok := f.(*goFile); ok {
-				gof.mu.Lock()
-				delete(gof.pkgs, id)
-				gof.mu.Unlock()
-			}
+		f, err := v.findFile(span.FileURI(filename))
+		if err != nil {
+			v.session.log.Errorf(ctx, "cannot find file %s: %v", f.URI(), err)
+			continue
 		}
+		gof, ok := f.(*goFile)
+		if !ok {
+			v.session.log.Errorf(ctx, "non-Go file %v", f.URI())
+			continue
+		}
+		gof.mu.Lock()
+		delete(gof.pkgs, id)
+		gof.mu.Unlock()
 	}
 	delete(v.pcache.packages, id)
+	return
 }
 
 // FindFile returns the file if the given URI is already a part of the view.
@@ -301,11 +308,11 @@ func (v *view) GetFile(ctx context.Context, uri span.URI) (source.File, error) {
 	v.mu.Lock()
 	defer v.mu.Unlock()
 
-	return v.getFile(uri)
+	return v.getFile(ctx, uri)
 }
 
 // getFile is the unlocked internal implementation of GetFile.
-func (v *view) getFile(uri span.URI) (viewFile, error) {
+func (v *view) getFile(ctx context.Context, uri span.URI) (viewFile, error) {
 	if f, err := v.findFile(uri); err != nil {
 		return nil, err
 	} else if f != nil {
@@ -344,7 +351,7 @@ func (v *view) getFile(uri span.URI) (viewFile, error) {
 			if !ok {
 				return
 			}
-			gof.invalidateContent()
+			gof.invalidateContent(ctx)
 		})
 	}
 	v.mapFile(uri, f)

internal/lsp/source/format.go

@@ -25,7 +25,7 @@ func Format(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) {
 		return nil, fmt.Errorf("no AST for %s", f.URI())
 	}
 	pkg := f.GetPackage(ctx)
-	if hasParseErrors(pkg.GetErrors()) {
+	if hasListErrors(pkg.GetErrors()) || hasParseErrors(pkg.GetErrors()) {
 		return nil, fmt.Errorf("%s has parse errors, not formatting", f.URI())
 	}
 	path, exact := astutil.PathEnclosingInterval(file, rng.Start, rng.End)
@@ -45,6 +45,23 @@ func Format(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) {
 	return computeTextEdits(ctx, f, buf.String()), nil
 }
 
+// Imports formats a file using the goimports tool.
+func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) {
+	data, _, err := f.Handle(ctx).Read(ctx)
+	if err != nil {
+		return nil, err
+	}
+	pkg := f.GetPackage(ctx)
+	if hasListErrors(pkg.GetErrors()) {
+		return nil, fmt.Errorf("%s has list errors, not running goimports", f.URI())
+	}
+	formatted, err := imports.Process(f.URI().Filename(), data, nil)
+	if err != nil {
+		return nil, err
+	}
+	return computeTextEdits(ctx, f, string(formatted)), nil
+}
+
 func hasParseErrors(errors []packages.Error) bool {
 	for _, err := range errors {
 		if err.Kind == packages.ParseError {
@@ -54,21 +71,13 @@ func hasParseErrors(errors []packages.Error) bool {
 	return false
 }
 
-// Imports formats a file using the goimports tool.
-func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) {
-	data, _, err := f.Handle(ctx).Read(ctx)
-	if err != nil {
-		return nil, err
+func hasListErrors(errors []packages.Error) bool {
+	for _, err := range errors {
+		if err.Kind == packages.ListError {
+			return true
+		}
 	}
-	tok := f.GetToken(ctx)
-	if tok == nil {
-		return nil, fmt.Errorf("no token file for %s", f.URI())
-	}
-	formatted, err := imports.Process(tok.Name(), data, nil)
-	if err != nil {
-		return nil, err
-	}
-	return computeTextEdits(ctx, f, string(formatted)), nil
+	return false
 }
 
 func computeTextEdits(ctx context.Context, file File, formatted string) (edits []TextEdit) {