From 4f238b926e10322727e514f9ec7635d21f9a7b8f Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 5 Sep 2019 17:34:00 -0400 Subject: [PATCH] internal/lsp: fix deadlock between f.mu and f.handleMu This change propagates the file handle through the type-checking process, ensuring that the same handle is used throughout. It also removes the ordering constraint that f.mu needs to be acquired before f.handleMu. To make this more correct, we should associate a cached package only with a FileHandle, but this relies on correct cache invalidation, so that will be addressed in future changes. Updates golang/go#34052 Change-Id: I6645046bfd15c882619a7f01f9b48c838de42a30 Reviewed-on: https://go-review.googlesource.com/c/tools/+/193718 Run-TryBot: Rebecca Stambler Reviewed-by: Ian Cottrell TryBot-Result: Gobot Gobot --- internal/lsp/cache/gofile.go | 27 +++++++++------------------ internal/lsp/cache/load.go | 18 +++++++++--------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index 083d459b30..62f6b472f0 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -63,14 +63,14 @@ func (f *goFile) GetToken(ctx context.Context) (*token.File, error) { func (f *goFile) GetAST(ctx context.Context, mode source.ParseMode) (*ast.File, error) { ctx = telemetry.File.With(ctx, f.URI()) + fh := f.Handle(ctx) - if f.isDirty(ctx) || f.wrongParseMode(ctx, mode) { - if err := f.view.loadParseTypecheck(ctx, f); err != nil { + if f.isDirty(ctx, fh) || f.wrongParseMode(ctx, fh, mode) { + if err := f.view.loadParseTypecheck(ctx, f, fh); err != nil { return nil, err } } // Check for a cached AST first, in case getting a trimmed version would actually cause a re-parse. - fh := f.Handle(ctx) cached, err := f.view.session.cache.cachedAST(fh, mode) if cached != nil || err != nil { return cached, err @@ -127,9 +127,10 @@ func (f *goFile) GetPackage(ctx context.Context) (source.Package, error) { func (f *goFile) GetCheckPackageHandles(ctx context.Context) ([]source.CheckPackageHandle, error) { ctx = telemetry.File.With(ctx, f.URI()) + fh := f.Handle(ctx) - if f.isDirty(ctx) || f.wrongParseMode(ctx, source.ParseFull) { - if err := f.view.loadParseTypecheck(ctx, f); err != nil { + if f.isDirty(ctx, fh) || f.wrongParseMode(ctx, fh, source.ParseFull) { + if err := f.view.loadParseTypecheck(ctx, f, fh); err != nil { return nil, err } } @@ -192,11 +193,10 @@ func bestCheckPackageHandle(uri span.URI, cphs []source.CheckPackageHandle) (sou return result, nil } -func (f *goFile) wrongParseMode(ctx context.Context, mode source.ParseMode) bool { +func (f *goFile) wrongParseMode(ctx context.Context, fh source.FileHandle, mode source.ParseMode) bool { f.mu.Lock() defer f.mu.Unlock() - fh := f.Handle(ctx) for _, cph := range f.pkgs { for _, ph := range cph.Files() { if fh.Identity() == ph.File().Identity() { @@ -219,7 +219,7 @@ func (f *goFile) Builtin() (*ast.File, bool) { // isDirty is true if the file needs to be type-checked. // It assumes that the file's view's mutex is held by the caller. -func (f *goFile) isDirty(ctx context.Context) bool { +func (f *goFile) isDirty(ctx context.Context, fh source.FileHandle) bool { f.mu.Lock() defer f.mu.Unlock() @@ -239,7 +239,6 @@ func (f *goFile) isDirty(ctx context.Context) bool { if len(f.missingImports) > 0 { return true } - fh := f.Handle(ctx) for _, cph := range f.pkgs { for _, file := range cph.Files() { // There is a type-checked package for the current file handle. @@ -252,14 +251,6 @@ func (f *goFile) isDirty(ctx context.Context) bool { } func (f *goFile) GetActiveReverseDeps(ctx context.Context) (files []source.GoFile) { - f.mu.Lock() - meta := f.meta - f.mu.Unlock() - - if meta == nil { - return nil - } - seen := make(map[packageID]struct{}) // visited packages results := make(map[*goFile]struct{}) @@ -269,7 +260,7 @@ func (f *goFile) GetActiveReverseDeps(ctx context.Context) (files []source.GoFil f.view.mcache.mu.Lock() defer f.view.mcache.mu.Unlock() - for _, m := range meta { + for _, m := range f.metadata() { f.view.reverseDeps(ctx, seen, results, m.id) for f := range results { if f == nil { diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 3fba121b2e..20ed2e2adb 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -18,11 +18,11 @@ import ( errors "golang.org/x/xerrors" ) -func (view *view) loadParseTypecheck(ctx context.Context, f *goFile) error { +func (view *view) loadParseTypecheck(ctx context.Context, f *goFile, fh source.FileHandle) error { ctx, done := trace.StartSpan(ctx, "cache.view.loadParseTypeCheck", telemetry.URI.Of(f.URI())) defer done() - meta, err := view.load(ctx, f) + meta, err := view.load(ctx, f, fh) if err != nil { return err } @@ -51,7 +51,7 @@ func (view *view) loadParseTypecheck(ctx context.Context, f *goFile) error { return nil } -func (view *view) load(ctx context.Context, f *goFile) ([]*metadata, error) { +func (view *view) load(ctx context.Context, f *goFile, fh source.FileHandle) ([]*metadata, error) { ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.URI.Of(f.URI())) defer done() @@ -72,7 +72,7 @@ func (view *view) load(ctx context.Context, f *goFile) ([]*metadata, error) { // If the AST for this file is trimmed, and we are explicitly type-checking it, // don't ignore function bodies. - if f.wrongParseMode(ctx, source.ParseFull) { + if f.wrongParseMode(ctx, fh, source.ParseFull) { // Remove the package and all of its reverse dependencies from the cache. for _, id := range toDelete { f.view.remove(ctx, id, map[packageID]struct{}{}) @@ -80,7 +80,7 @@ func (view *view) load(ctx context.Context, f *goFile) ([]*metadata, error) { } // Get the metadata for the file. - meta, err := view.checkMetadata(ctx, f) + meta, err := view.checkMetadata(ctx, f, fh) if err != nil { return nil, err } @@ -92,10 +92,10 @@ func (view *view) load(ctx context.Context, f *goFile) ([]*metadata, error) { // 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) ([]*metadata, error) { +func (v *view) checkMetadata(ctx context.Context, f *goFile, fh source.FileHandle) ([]*metadata, error) { // Check if we need to re-run go/packages before loading the package. f.mu.Lock() - runGopackages := v.shouldRunGopackages(ctx, f) + runGopackages := v.shouldRunGopackages(ctx, f, fh) metadata := f.metadata() f.mu.Unlock() @@ -178,7 +178,7 @@ func sameSet(x, y map[packagePath]struct{}) bool { // shouldRunGopackages reparses a file's package and import declarations to // determine if they have changed. // It assumes that the caller holds the lock on the f.mu lock. -func (v *view) shouldRunGopackages(ctx context.Context, f *goFile) (result bool) { +func (v *view) shouldRunGopackages(ctx context.Context, f *goFile, fh source.FileHandle) (result bool) { defer func() { // Clear metadata if we are intending to re-run go/packages. if result { @@ -196,7 +196,7 @@ func (v *view) shouldRunGopackages(ctx context.Context, f *goFile) (result bool) return true } // Get file content in case we don't already have it. - parsed, err := v.session.cache.ParseGoHandle(f.Handle(ctx), source.ParseHeader).Parse(ctx) + parsed, err := v.session.cache.ParseGoHandle(fh, source.ParseHeader).Parse(ctx) if err == context.Canceled { log.Error(ctx, "parsing file header", err, tag.Of("file", f.URI())) return false