From 065b96d36cf81bee7836ac900cce5f8ef4285dca Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 7 Jul 2020 15:18:31 -0400 Subject: [PATCH] internal/lsp/cache: fix race in RunProcessEnvFunc Changing build flags (-modfile) while work is happening in the background causes races. Explicitly detect relevant configuration changes and only modify the ProcessEnv then, when the resolver is inactive after the call to ClearForNewMod. This still leaves a very small window for a race: if refreshProcessEnv has already captured env but not yet started priming the cache, it may race with the modification. But I don't expect it to be a problem in practice. Fixes golang/go#39865. Change-Id: I31c79f39be55975fee14aa0e548b060c46cdd882 Reviewed-on: https://go-review.googlesource.com/c/tools/+/241317 Run-TryBot: Heschi Kreinick Reviewed-by: Rebecca Stambler --- internal/lsp/cache/session.go | 8 +++ internal/lsp/cache/view.go | 104 +++++++++++++++++----------------- 2 files changed, 60 insertions(+), 52 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 24c4714a57..ec7463e893 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -13,6 +13,7 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" + "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/xcontext" @@ -160,6 +161,13 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, return nil, nil, err } + // We have v.goEnv now. + v.processEnv = &imports.ProcessEnv{ + GocmdRunner: s.gocmdRunner, + WorkingDir: folder.Filename(), + Env: v.goEnv, + } + // Initialize the view without blocking. initCtx, initCancel := context.WithCancel(xcontext.Detach(ctx)) v.initCancel = initCancel diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 82821e7fcc..ba6b81cbc1 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -71,10 +71,12 @@ type View struct { // TODO(suzmue): the state cached in the process env is specific to each view, // however, there is state that can be shared between views that is not currently // cached, like the module cache. - processEnv *imports.ProcessEnv - cacheRefreshDuration time.Duration - cacheRefreshTimer *time.Timer - cachedModFileVersion source.FileIdentity + processEnv *imports.ProcessEnv + cleanupProcessEnv func() + cacheRefreshDuration time.Duration + cacheRefreshTimer *time.Timer + cachedModFileIdentifier string + cachedBuildFlags []string // keep track of files by uri and by basename, a single file may be mapped // to multiple uris, and the same basename may map to multiple files @@ -363,53 +365,56 @@ func (v *View) RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) v.importsMu.Lock() defer v.importsMu.Unlock() - // The resolver cached in the process env is reused, but some fields need - // to be repopulated for each use. - if v.processEnv == nil { - v.processEnv = &imports.ProcessEnv{} - } - + // Use temporary go.mod files, but always go to disk for the contents. + // Rebuilding the cache is expensive, and we don't want to do it for + // transient changes. var modFH, sumFH source.FileHandle - if v.tmpMod { - var err error - // Use temporary go.mod files, but always go to disk for the contents. - // Rebuilding the cache is expensive, and we don't want to do it for - // transient changes. + var modFileIdentifier string + var err error + if v.modURI != "" { modFH, err = v.session.cache.getFile(ctx, v.modURI) if err != nil { return err } - if v.sumURI != "" { - sumFH, err = v.session.cache.getFile(ctx, v.sumURI) - if err != nil { - return err - } - } + modFileIdentifier = modFH.Identity().Identifier } - - cleanup, err := v.populateProcessEnv(ctx, modFH, sumFH) - if err != nil { - return err - } - defer cleanup() - - // If the go.mod file has changed, clear the cache. - if v.modURI != "" { - modFH, err := v.session.cache.getFile(ctx, v.modURI) + if v.sumURI != "" { + sumFH, err = v.session.cache.getFile(ctx, v.sumURI) if err != nil { return err } - if modFH.Identity() != v.cachedModFileVersion { + } + // v.goEnv is immutable -- changes make a new view. Options can change. + // We can't compare build flags directly because we may add -modfile. + v.optionsMu.Lock() + localPrefix := v.options.LocalPrefix + currentBuildFlags := v.options.BuildFlags + changed := !reflect.DeepEqual(currentBuildFlags, v.cachedBuildFlags) || + v.options.VerboseOutput != (v.processEnv.Logf != nil) || + modFileIdentifier != v.cachedModFileIdentifier + v.optionsMu.Unlock() + + // If anything relevant to imports has changed, clear caches and + // update the processEnv. Clearing caches blocks on any background + // scans. + if changed { + // As a special case, skip cleanup the first time -- we haven't fully + // initialized the environment yet and calling GetResolver will do + // unnecessary work and potentially mess up the go.mod file. + if v.cleanupProcessEnv != nil { if resolver, err := v.processEnv.GetResolver(); err == nil { resolver.(*imports.ModuleResolver).ClearForNewMod() } - v.cachedModFileVersion = modFH.Identity() + v.cleanupProcessEnv() + } + v.cachedModFileIdentifier = modFH.Identity().Identifier + v.cachedBuildFlags = currentBuildFlags + v.cleanupProcessEnv, err = v.populateProcessEnv(ctx, modFH, sumFH) + if err != nil { + return err } } - v.optionsMu.Lock() - localPrefix := v.options.LocalPrefix - v.optionsMu.Unlock() // Run the user function. opts := &imports.Options{ // Defaults. @@ -464,24 +469,24 @@ func (v *View) refreshProcessEnv() { } // populateProcessEnv sets the dynamically configurable fields for the view's -// process environment. It operates on a snapshot because it needs to access -// file contents. Assumes that the caller is holding the s.view.importsMu. +// process environment. Assumes that the caller is holding the s.view.importsMu. func (v *View) populateProcessEnv(ctx context.Context, modFH, sumFH source.FileHandle) (cleanup func(), err error) { cleanup = func() {} + pe := v.processEnv v.optionsMu.Lock() - _, buildFlags := v.envLocked() - verboseOutput := v.options.VerboseOutput + pe.BuildFlags = append([]string(nil), v.options.BuildFlags...) + if v.options.VerboseOutput { + pe.Logf = func(format string, args ...interface{}) { + event.Log(ctx, fmt.Sprintf(format, args...)) + } + } else { + pe.Logf = nil + } v.optionsMu.Unlock() - pe := v.processEnv - pe.GocmdRunner = v.session.gocmdRunner - pe.BuildFlags = buildFlags - pe.Env = v.goEnv - pe.WorkingDir = v.folder.Filename() - // Add -modfile to the build flags, if we are using it. - if modFH != nil { + if v.tmpMod && modFH != nil { var tmpURI span.URI tmpURI, cleanup, err = tempModFile(modFH, sumFH) if err != nil { @@ -490,11 +495,6 @@ func (v *View) populateProcessEnv(ctx context.Context, modFH, sumFH source.FileH pe.BuildFlags = append(pe.BuildFlags, fmt.Sprintf("-modfile=%s", tmpURI.Filename())) } - if verboseOutput { - pe.Logf = func(format string, args ...interface{}) { - event.Log(ctx, fmt.Sprintf(format, args...)) - } - } return cleanup, nil }