qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-18 17:24:40 -07:00
Code Releases Activity

internal/lsp: fix deadlock between f.mu and f.handleMu

Browse Source 
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 <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This commit is contained in:
Rebecca Stambler 2019-09-05 17:34:00 -04:00
parent 1d492ad178
commit 4f238b926e
2 changed files with 18 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
internal/lsp/cache/gofile.go vendored
View File

@ -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) { func (f *goFile) GetAST(ctx context.Context, mode source.ParseMode) (*ast.File, error) {
ctx = telemetry.File.With(ctx, f.URI()) ctx = telemetry.File.With(ctx, f.URI())
fh := f.Handle(ctx)
if f.isDirty(ctx) || f.wrongParseMode(ctx, mode) { if f.isDirty(ctx, fh) || f.wrongParseMode(ctx, fh, mode) {
if err := f.view.loadParseTypecheck(ctx, f); err != nil { if err := f.view.loadParseTypecheck(ctx, f, fh); err != nil {
return nil, err return nil, err
} }
} }
// Check for a cached AST first, in case getting a trimmed version would actually cause a re-parse. // 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) cached, err := f.view.session.cache.cachedAST(fh, mode)
if cached != nil || err != nil { if cached != nil || err != nil {
return cached, err 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) { func (f *goFile) GetCheckPackageHandles(ctx context.Context) ([]source.CheckPackageHandle, error) {
ctx = telemetry.File.With(ctx, f.URI()) ctx = telemetry.File.With(ctx, f.URI())
fh := f.Handle(ctx)
if f.isDirty(ctx) || f.wrongParseMode(ctx, source.ParseFull) { if f.isDirty(ctx, fh) || f.wrongParseMode(ctx, fh, source.ParseFull) {
if err := f.view.loadParseTypecheck(ctx, f); err != nil { if err := f.view.loadParseTypecheck(ctx, f, fh); err != nil {
return nil, err return nil, err
} }
} }
@ -192,11 +193,10 @@ func bestCheckPackageHandle(uri span.URI, cphs []source.CheckPackageHandle) (sou
return result, nil 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() f.mu.Lock()
defer f.mu.Unlock() defer f.mu.Unlock()
fh := f.Handle(ctx)
for _, cph := range f.pkgs { for _, cph := range f.pkgs {
for _, ph := range cph.Files() { for _, ph := range cph.Files() {
if fh.Identity() == ph.File().Identity() { 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. // 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. // 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() f.mu.Lock()
defer f.mu.Unlock() defer f.mu.Unlock()
@ -239,7 +239,6 @@ func (f *goFile) isDirty(ctx context.Context) bool {
if len(f.missingImports) > 0 { if len(f.missingImports) > 0 {
return true return true
} }
fh := f.Handle(ctx)
for _, cph := range f.pkgs { for _, cph := range f.pkgs {
for _, file := range cph.Files() { for _, file := range cph.Files() {
// There is a type-checked package for the current file handle. // 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) { 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 seen := make(map[packageID]struct{}) // visited packages
results := make(map[*goFile]struct{}) results := make(map[*goFile]struct{})
@ -269,7 +260,7 @@ func (f *goFile) GetActiveReverseDeps(ctx context.Context) (files []source.GoFil
f.view.mcache.mu.Lock() f.view.mcache.mu.Lock()
defer f.view.mcache.mu.Unlock() defer f.view.mcache.mu.Unlock()
for _, m := range meta { for _, m := range f.metadata() {
f.view.reverseDeps(ctx, seen, results, m.id) f.view.reverseDeps(ctx, seen, results, m.id)
for f := range results { for f := range results {
if f == nil { if f == nil {

18
internal/lsp/cache/load.go vendored
View File

@ -18,11 +18,11 @@ import (
errors "golang.org/x/xerrors" 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())) ctx, done := trace.StartSpan(ctx, "cache.view.loadParseTypeCheck", telemetry.URI.Of(f.URI()))
defer done() defer done()
meta, err := view.load(ctx, f) meta, err := view.load(ctx, f, fh)
if err != nil { if err != nil {
return err return err
} }
@ -51,7 +51,7 @@ func (view *view) loadParseTypecheck(ctx context.Context, f *goFile) error {
return nil 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())) ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.URI.Of(f.URI()))
defer done() 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, // If the AST for this file is trimmed, and we are explicitly type-checking it,
// don't ignore function bodies. // 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. // Remove the package and all of its reverse dependencies from the cache.
for _, id := range toDelete { for _, id := range toDelete {
f.view.remove(ctx, id, map[packageID]struct{}{}) 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. // Get the metadata for the file.
meta, err := view.checkMetadata(ctx, f) meta, err := view.checkMetadata(ctx, f, fh)
if err != nil { if err != nil {
return nil, err 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. // checkMetadata determines if we should run go/packages.Load for this file.
// If yes, update the metadata for the file and its package. // 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. // Check if we need to re-run go/packages before loading the package.
f.mu.Lock() f.mu.Lock()
runGopackages := v.shouldRunGopackages(ctx, f) runGopackages := v.shouldRunGopackages(ctx, f, fh)
metadata := f.metadata() metadata := f.metadata()
f.mu.Unlock() 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 // shouldRunGopackages reparses a file's package and import declarations to
// determine if they have changed. // determine if they have changed.
// It assumes that the caller holds the lock on the f.mu lock. // 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() { defer func() {
// Clear metadata if we are intending to re-run go/packages. // Clear metadata if we are intending to re-run go/packages.
if result { if result {
@ -196,7 +196,7 @@ func (v *view) shouldRunGopackages(ctx context.Context, f *goFile) (result bool)
return true return true
} }
// Get file content in case we don't already have it. // 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 { if err == context.Canceled {
log.Error(ctx, "parsing file header", err, tag.Of("file", f.URI())) log.Error(ctx, "parsing file header", err, tag.Of("file", f.URI()))
return false return false