diff options
author | Rohan Challa <rohan@golang.org> | 2020-03-23 08:35:36 -0400 |
---|---|---|
committer | Rebecca Stambler <rstambler@golang.org> | 2020-04-08 03:22:09 +0000 |
commit | 46bd65c8538fb57593a8365bea5663a3239aee37 (patch) | |
tree | 0d5909033cc53f34ced1615b0d22002193bbe4d6 | |
parent | 4d14fc9c00cedea0631fe7d87ee41b1a25031402 (diff) | |
download | golang-x-tools-46bd65c8538fb57593a8365bea5663a3239aee37.tar.gz |
internal/lsp/cache: add concurrency error check for go cmds
This change attempts to fix a concurrency error that would cause
textDocument/CodeLens, textDocument/Formatting, textDocument/DocumentLink,
and textDocument/Hover from failing on go.mod files.
The issue was that the go command would return a potential concurrency
error since the ModHandle and the ModTidyHandle are both using the
temporary go.mod file.
Updates golang/go#37824
Change-Id: I6cd63c1f75817c7308e033aec473966536a2a3bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224917
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
-rw-r--r-- | go/internal/packagesdriver/sizes.go | 11 | ||||
-rw-r--r-- | go/packages/golist.go | 11 | ||||
-rw-r--r-- | go/packages/packages.go | 13 | ||||
-rw-r--r-- | go/packages/packagestest/modules.go | 5 | ||||
-rw-r--r-- | internal/gocommand/invoke.go | 69 | ||||
-rw-r--r-- | internal/gocommand/invoke_test.go | 3 | ||||
-rw-r--r-- | internal/imports/fix.go | 4 | ||||
-rw-r--r-- | internal/imports/fix_test.go | 2 | ||||
-rw-r--r-- | internal/imports/imports.go | 5 | ||||
-rw-r--r-- | internal/imports/imports_test.go | 6 | ||||
-rw-r--r-- | internal/imports/mod_test.go | 10 | ||||
-rw-r--r-- | internal/lsp/cache/load.go | 2 | ||||
-rw-r--r-- | internal/lsp/cache/mod.go | 15 | ||||
-rw-r--r-- | internal/lsp/cache/session.go | 2 | ||||
-rw-r--r-- | internal/lsp/cache/snapshot.go | 6 | ||||
-rw-r--r-- | internal/lsp/cache/view.go | 58 | ||||
-rw-r--r-- | internal/lsp/command.go | 13 | ||||
-rw-r--r-- | internal/lsp/debug/info.go | 6 | ||||
-rw-r--r-- | internal/lsp/fake/workspace.go | 3 | ||||
-rw-r--r-- | internal/packagesinternal/packages.go | 10 |
20 files changed, 157 insertions, 97 deletions
diff --git a/go/internal/packagesdriver/sizes.go b/go/internal/packagesdriver/sizes.go index 5ee692d38..dc6177c12 100644 --- a/go/internal/packagesdriver/sizes.go +++ b/go/internal/packagesdriver/sizes.go @@ -19,8 +19,7 @@ import ( var debug = false -// GetSizes returns the sizes used by the underlying driver with the given parameters. -func GetSizes(ctx context.Context, buildFlags, env []string, dir string, usesExportData bool) (types.Sizes, error) { +func GetSizes(ctx context.Context, buildFlags, env []string, gocmdRunner *gocommand.Runner, dir string) (types.Sizes, error) { // TODO(matloob): Clean this up. This code is mostly a copy of packages.findExternalDriver. const toolPrefix = "GOPACKAGESDRIVER=" tool := "" @@ -40,7 +39,7 @@ func GetSizes(ctx context.Context, buildFlags, env []string, dir string, usesExp } if tool == "off" { - return GetSizesGolist(ctx, buildFlags, env, dir, usesExportData) + return GetSizesGolist(ctx, buildFlags, env, gocmdRunner, dir) } req, err := json.Marshal(struct { @@ -76,7 +75,7 @@ func GetSizes(ctx context.Context, buildFlags, env []string, dir string, usesExp return response.Sizes, nil } -func GetSizesGolist(ctx context.Context, buildFlags, env []string, dir string, usesExportData bool) (types.Sizes, error) { +func GetSizesGolist(ctx context.Context, buildFlags, env []string, gocmdRunner *gocommand.Runner, dir string) (types.Sizes, error) { inv := gocommand.Invocation{ Verb: "list", Args: []string{"-f", "{{context.GOARCH}} {{context.Compiler}}", "--", "unsafe"}, @@ -84,7 +83,7 @@ func GetSizesGolist(ctx context.Context, buildFlags, env []string, dir string, u BuildFlags: buildFlags, WorkingDir: dir, } - stdout, stderr, friendlyErr, rawErr := inv.RunRaw(ctx) + stdout, stderr, friendlyErr, rawErr := gocmdRunner.RunRaw(ctx, inv) var goarch, compiler string if rawErr != nil { if strings.Contains(rawErr.Error(), "cannot find main module") { @@ -96,7 +95,7 @@ func GetSizesGolist(ctx context.Context, buildFlags, env []string, dir string, u Env: env, WorkingDir: dir, } - envout, enverr := inv.Run(ctx) + envout, enverr := gocmdRunner.Run(ctx, inv) if enverr != nil { return nil, enverr } diff --git a/go/packages/golist.go b/go/packages/golist.go index 099207d78..88ca6691d 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -143,7 +143,7 @@ func goListDriver(cfg *Config, patterns ...string) (*driverResponse, error) { sizeswg.Add(1) go func() { var sizes types.Sizes - sizes, sizeserr = packagesdriver.GetSizesGolist(ctx, cfg.BuildFlags, cfg.Env, cfg.Dir, usesExportData(cfg)) + sizes, sizeserr = packagesdriver.GetSizesGolist(ctx, cfg.BuildFlags, cfg.Env, cfg.gocmdRunner, cfg.Dir) // types.SizesFor always returns nil or a *types.StdSizes. response.dr.Sizes, _ = sizes.(*types.StdSizes) sizeswg.Done() @@ -717,7 +717,7 @@ func golistargs(cfg *Config, words []string) []string { func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, error) { cfg := state.cfg - inv := &gocommand.Invocation{ + inv := gocommand.Invocation{ Verb: verb, Args: args, BuildFlags: cfg.BuildFlags, @@ -725,8 +725,11 @@ func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, Logf: cfg.Logf, WorkingDir: cfg.Dir, } - - stdout, stderr, _, err := inv.RunRaw(cfg.Context) + gocmdRunner := cfg.gocmdRunner + if gocmdRunner == nil { + gocmdRunner = &gocommand.Runner{} + } + stdout, stderr, _, err := gocmdRunner.RunRaw(cfg.Context, inv) if err != nil { // Check for 'go' executable not being found. if ee, ok := err.(*exec.Error); ok && ee.Err == exec.ErrNotFound { diff --git a/go/packages/packages.go b/go/packages/packages.go index 1ac6558c1..03fd999c0 100644 --- a/go/packages/packages.go +++ b/go/packages/packages.go @@ -23,6 +23,7 @@ import ( "sync" "golang.org/x/tools/go/gcexportdata" + "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/packagesinternal" ) @@ -127,6 +128,9 @@ type Config struct { // Env []string + // gocmdRunner guards go command calls from concurrency errors. + gocmdRunner *gocommand.Runner + // BuildFlags is a list of command-line flags to be passed through to // the build system's query tool. BuildFlags []string @@ -311,6 +315,12 @@ func init() { packagesinternal.GetModule = func(p interface{}) *packagesinternal.Module { return p.(*Package).module } + packagesinternal.GetGoCmdRunner = func(config interface{}) *gocommand.Runner { + return config.(*Config).gocmdRunner + } + packagesinternal.SetGoCmdRunner = func(config interface{}, runner *gocommand.Runner) { + config.(*Config).gocmdRunner = runner + } } // An Error describes a problem with a package's metadata, syntax, or types. @@ -473,6 +483,9 @@ func newLoader(cfg *Config) *loader { if ld.Config.Env == nil { ld.Config.Env = os.Environ() } + if ld.Config.gocmdRunner == nil { + ld.Config.gocmdRunner = &gocommand.Runner{} + } if ld.Context == nil { ld.Context = context.Background() } diff --git a/go/packages/packagestest/modules.go b/go/packages/packagestest/modules.go index 8bf830ad4..a6c46cf64 100644 --- a/go/packages/packagestest/modules.go +++ b/go/packages/packagestest/modules.go @@ -16,6 +16,7 @@ import ( "strings" "golang.org/x/tools/internal/gocommand" + "golang.org/x/tools/internal/packagesinternal" ) // Modules is the exporter that produces module layouts. @@ -166,6 +167,8 @@ func (modules) Finalize(exported *Exported) error { "GOPROXY="+proxyDirToURL(proxyDir), "GOSUMDB=off", ) + gocmdRunner := &gocommand.Runner{} + packagesinternal.SetGoCmdRunner(exported.Config, gocmdRunner) // Run go mod download to recreate the mod cache dir with all the extra // stuff in cache. All the files created by Export should be recreated. @@ -176,7 +179,7 @@ func (modules) Finalize(exported *Exported) error { BuildFlags: exported.Config.BuildFlags, WorkingDir: exported.Config.Dir, } - if _, err := inv.Run(context.Background()); err != nil { + if _, err := gocmdRunner.Run(context.Background(), inv); err != nil { return err } return nil diff --git a/internal/gocommand/invoke.go b/internal/gocommand/invoke.go index ac80f1072..5b1341270 100644 --- a/internal/gocommand/invoke.go +++ b/internal/gocommand/invoke.go @@ -12,10 +12,70 @@ import ( "io" "os" "os/exec" + "regexp" "strings" + "sync" "time" + + "golang.org/x/tools/internal/telemetry/event" ) +// An Runner will run go command invocations and serialize +// them if it sees a concurrency error. +type Runner struct { + // LoadMu guards packages.Load calls and associated state. + loadMu sync.Mutex + serializeLoads int +} + +// 1.13: go: updates to go.mod needed, but contents have changed +// 1.14: go: updating go.mod: existing contents have changed since last read +var modConcurrencyError = regexp.MustCompile(`go:.*go.mod.*contents have changed`) + +// Run calls Runner.RunRaw, serializing requests if they fight over +// go.mod changes. +func (runner *Runner) Run(ctx context.Context, inv Invocation) (*bytes.Buffer, error) { + stdout, _, friendly, _ := runner.RunRaw(ctx, inv) + return stdout, friendly +} + +// Run calls Innvocation.RunRaw, serializing requests if they fight over +// go.mod changes. +func (runner *Runner) RunRaw(ctx context.Context, inv Invocation) (*bytes.Buffer, *bytes.Buffer, error, error) { + // We want to run invocations concurrently as much as possible. However, + // if go.mod updates are needed, only one can make them and the others will + // fail. We need to retry in those cases, but we don't want to thrash so + // badly we never recover. To avoid that, once we've seen one concurrency + // error, start serializing everything until the backlog has cleared out. + runner.loadMu.Lock() + var locked bool // If true, we hold the mutex and have incremented. + if runner.serializeLoads == 0 { + runner.loadMu.Unlock() + } else { + locked = true + runner.serializeLoads++ + } + defer func() { + if locked { + runner.serializeLoads-- + runner.loadMu.Unlock() + } + }() + + for { + stdout, stderr, friendlyErr, err := inv.runRaw(ctx) + if friendlyErr == nil || !modConcurrencyError.MatchString(friendlyErr.Error()) { + return stdout, stderr, friendlyErr, err + } + event.Error(ctx, "Load concurrency error, will retry serially", err) + if !locked { + runner.loadMu.Lock() + runner.serializeLoads++ + locked = true + } + } +} + // An Invocation represents a call to the go command. type Invocation struct { Verb string @@ -26,16 +86,9 @@ type Invocation struct { Logf func(format string, args ...interface{}) } -// Run runs the invocation, returning its stdout and an error suitable for -// human consumption, including stderr. -func (i *Invocation) Run(ctx context.Context) (*bytes.Buffer, error) { - stdout, _, friendly, _ := i.RunRaw(ctx) - return stdout, friendly -} - // RunRaw is like RunPiped, but also returns the raw stderr and error for callers // that want to do low-level error handling/recovery. -func (i *Invocation) RunRaw(ctx context.Context) (stdout *bytes.Buffer, stderr *bytes.Buffer, friendlyError error, rawError error) { +func (i *Invocation) runRaw(ctx context.Context) (stdout *bytes.Buffer, stderr *bytes.Buffer, friendlyError error, rawError error) { stdout = &bytes.Buffer{} stderr = &bytes.Buffer{} rawError = i.RunPiped(ctx, stdout, stderr) diff --git a/internal/gocommand/invoke_test.go b/internal/gocommand/invoke_test.go index ff50ea7eb..aee108b0a 100644 --- a/internal/gocommand/invoke_test.go +++ b/internal/gocommand/invoke_test.go @@ -15,7 +15,8 @@ func TestGoVersion(t *testing.T) { inv := gocommand.Invocation{ Verb: "version", } - if _, err := inv.Run(context.Background()); err != nil { + gocmdRunner := &gocommand.Runner{} + if _, err := gocmdRunner.Run(context.Background(), inv); err != nil { t.Error(err) } } diff --git a/internal/imports/fix.go b/internal/imports/fix.go index 92a23439f..264d001ed 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go @@ -747,6 +747,8 @@ func getPackageExports(ctx context.Context, wrapped func(PackageExport), searchP type ProcessEnv struct { LocalPrefix string + GocmdRunner *gocommand.Runner + BuildFlags []string // If non-empty, these will be used instead of the @@ -830,7 +832,7 @@ func (e *ProcessEnv) invokeGo(ctx context.Context, verb string, args ...string) Logf: e.Logf, WorkingDir: e.WorkingDir, } - return inv.Run(ctx) + return e.GocmdRunner.Run(ctx, inv) } func addStdlibCandidates(pass *pass, refs references) { diff --git a/internal/imports/fix_test.go b/internal/imports/fix_test.go index 5f9695201..68522b44e 100644 --- a/internal/imports/fix_test.go +++ b/internal/imports/fix_test.go @@ -19,6 +19,7 @@ import ( "testing" "golang.org/x/tools/go/packages/packagestest" + "golang.org/x/tools/internal/gocommand" ) var testDebug = flag.Bool("debug", false, "enable debug output") @@ -1638,6 +1639,7 @@ func (c testConfig) test(t *testing.T, fn func(*goimportTest)) { GO111MODULE: env["GO111MODULE"], GOSUMDB: env["GOSUMDB"], WorkingDir: exported.Config.Dir, + GocmdRunner: &gocommand.Runner{}, }, exported: exported, } diff --git a/internal/imports/imports.go b/internal/imports/imports.go index b18daea29..f43d6b22e 100644 --- a/internal/imports/imports.go +++ b/internal/imports/imports.go @@ -27,6 +27,7 @@ import ( "strings" "golang.org/x/tools/go/ast/astutil" + "golang.org/x/tools/internal/gocommand" ) // Options is golang.org/x/tools/imports.Options with extra internal-only options. @@ -154,6 +155,10 @@ func initialize(filename string, src []byte, opt *Options) ([]byte, *Options, er GOSUMDB: os.Getenv("GOSUMDB"), } } + // Set the gocmdRunner if the user has not provided it. + if opt.Env.GocmdRunner == nil { + opt.Env.GocmdRunner = &gocommand.Runner{} + } if src == nil { b, err := ioutil.ReadFile(filename) if err != nil { diff --git a/internal/imports/imports_test.go b/internal/imports/imports_test.go index 1901e6a71..70f66abde 100644 --- a/internal/imports/imports_test.go +++ b/internal/imports/imports_test.go @@ -5,6 +5,7 @@ import ( "os" "testing" + "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/testenv" ) @@ -31,8 +32,9 @@ func TestNilOpts(t *testing.T) { name: "default", opt: &Options{ Env: &ProcessEnv{ - GOPATH: build.Default.GOPATH, - GOROOT: build.Default.GOROOT, + GOPATH: build.Default.GOPATH, + GOROOT: build.Default.GOROOT, + GocmdRunner: &gocommand.Runner{}, }, Comments: true, TabIndent: true, diff --git a/internal/imports/mod_test.go b/internal/imports/mod_test.go index d92211e54..352fd202b 100644 --- a/internal/imports/mod_test.go +++ b/internal/imports/mod_test.go @@ -19,6 +19,7 @@ import ( "testing" "golang.org/x/mod/module" + "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/gopathwalk" "golang.org/x/tools/internal/testenv" "golang.org/x/tools/txtar" @@ -676,6 +677,7 @@ func setup(t *testing.T, main, wd string) *modTest { GOPROXY: proxyDirToURL(proxyDir), GOSUMDB: "off", WorkingDir: filepath.Join(mainDir, wd), + GocmdRunner: &gocommand.Runner{}, } if *testDebug { env.Logf = log.Printf @@ -850,6 +852,7 @@ func TestInvalidModCache(t *testing.T) { GOPATH: filepath.Join(dir, "gopath"), GO111MODULE: "on", GOSUMDB: "off", + GocmdRunner: &gocommand.Runner{}, WorkingDir: dir, } resolver := newModuleResolver(env) @@ -918,9 +921,10 @@ import _ "rsc.io/quote" func BenchmarkScanModCache(b *testing.B) { env := &ProcessEnv{ - GOPATH: build.Default.GOPATH, - GOROOT: build.Default.GOROOT, - Logf: log.Printf, + GOPATH: build.Default.GOPATH, + GOROOT: build.Default.GOROOT, + GocmdRunner: &gocommand.Runner{}, + Logf: log.Printf, } exclude := []gopathwalk.RootType{gopathwalk.RootGOROOT} scanToSlice(env.GetResolver(), exclude) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index dab568cd9..880d6f8a8 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -84,7 +84,7 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { defer done() cfg := s.Config(ctx) - pkgs, err := s.view.loadPackages(cfg, query...) + pkgs, err := packages.Load(cfg, query...) // If the context was canceled, return early. Otherwise, we might be // type-checking an incomplete result. Check the context directly, diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index 1cccaa4c9..461963310 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -20,6 +20,7 @@ import ( "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/memoize" + "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/event" errors "golang.org/x/xerrors" @@ -220,7 +221,7 @@ func goModWhy(ctx context.Context, cfg *packages.Config, folder string, data *mo for _, req := range data.origParsedFile.Require { inv.Args = append(inv.Args, req.Mod.Path) } - stdout, err := inv.Run(ctx) + stdout, err := packagesinternal.GetGoCmdRunner(cfg).Run(ctx, inv) if err != nil { return err } @@ -247,7 +248,7 @@ func dependencyUpgrades(ctx context.Context, cfg *packages.Config, folder string Env: cfg.Env, WorkingDir: folder, } - stdout, err := inv.Run(ctx) + stdout, err := packagesinternal.GetGoCmdRunner(cfg).Run(ctx, inv) if err != nil { return err } @@ -288,6 +289,7 @@ func (s *snapshot) ModTidyHandle(ctx context.Context, realfh source.FileHandle) cfg := s.Config(ctx) options := s.View().Options() folder := s.View().Folder().Filename() + gocmdRunner := s.view.gocmdRunner wsPackages, err := s.WorkspacePackages(ctx) if ctx.Err() != nil { @@ -358,12 +360,9 @@ func (s *snapshot) ModTidyHandle(ctx context.Context, realfh source.FileHandle) Env: cfg.Env, WorkingDir: folder, } - if _, err := inv.Run(ctx); err != nil { - // Ignore concurrency errors here. - if !modConcurrencyError.MatchString(err.Error()) { - return &modData{ - err: err, - } + if _, err := gocmdRunner.Run(ctx, inv); err != nil { + return &modData{ + err: err, } } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 3917c92a8..40d8ee785 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -11,6 +11,7 @@ import ( "sync" "sync/atomic" + "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/lsp/debug" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" @@ -133,6 +134,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, modHandles: make(map[span.URI]*modHandle), }, ignoredURIs: make(map[span.URI]struct{}), + gocmdRunner: &gocommand.Runner{}, } v.snapshot.view = v diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 23a54ea56..013ba0dd5 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -19,6 +19,7 @@ import ( "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/event" errors "golang.org/x/xerrors" @@ -94,7 +95,7 @@ func (s *snapshot) Config(ctx context.Context) *packages.Config { verboseOutput := s.view.options.VerboseOutput s.view.optionsMu.Unlock() - return &packages.Config{ + cfg := &packages.Config{ Env: env, Dir: s.view.folder.Filename(), Context: ctx, @@ -117,6 +118,9 @@ func (s *snapshot) Config(ctx context.Context) *packages.Config { }, Tests: true, } + packagesinternal.SetGoCmdRunner(cfg, s.view.gocmdRunner) + + return cfg } func (s *snapshot) buildOverlay() map[string][]byte { diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 4d81b9e54..e707466b7 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -15,12 +15,10 @@ import ( "os" "path/filepath" "reflect" - "regexp" "strings" "sync" "time" - "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/debug" @@ -111,9 +109,8 @@ type view struct { // `go env` variables that need to be tracked. gopath, gocache string - // LoadMu guards packages.Load calls and associated state. - loadMu sync.Mutex - serializeLoads int + // gocmdRunner guards go command calls from concurrency errors. + gocmdRunner *gocommand.Runner } type builtinPackageHandle struct { @@ -279,7 +276,7 @@ func (v *view) WriteEnv(ctx context.Context, w io.Writer) error { Env: env, WorkingDir: v.Folder().Filename(), } - stdout, err := inv.Run(ctx) + stdout, err := v.gocmdRunner.Run(ctx, inv) if err != nil { return err } @@ -364,6 +361,7 @@ func (v *view) buildProcessEnv(ctx context.Context) (*imports.ProcessEnv, error) WorkingDir: v.folder.Filename(), BuildFlags: buildFlags, LocalPrefix: localPrefix, + GocmdRunner: v.gocmdRunner, } if verboseOutput { processEnv.Logf = func(format string, args ...interface{}) { @@ -734,7 +732,7 @@ func (v *view) getGoEnv(ctx context.Context, env []string) (string, error) { Env: env, WorkingDir: v.Folder().Filename(), } - stdout, err := inv.Run(ctx) + stdout, err := v.gocmdRunner.Run(ctx, inv) if err != nil { return "", err } @@ -767,50 +765,6 @@ func (v *view) getGoEnv(ctx context.Context, env []string) (string, error) { return "", nil } -// 1.13: go: updates to go.mod needed, but contents have changed -// 1.14: go: updating go.mod: existing contents have changed since last read -var modConcurrencyError = regexp.MustCompile(`go:.*go.mod.*contents have changed`) - -// LoadPackages calls packages.Load, serializing requests if they fight over -// go.mod changes. -func (v *view) loadPackages(cfg *packages.Config, patterns ...string) ([]*packages.Package, error) { - // We want to run go list calls concurrently as much as possible. However, - // if go.mod updates are needed, only one can make them and the others will - // fail. We need to retry in those cases, but we don't want to thrash so - // badly we never recover. To avoid that, once we've seen one concurrency - // error, start serializing everything until the backlog has cleared out. - // This could all be avoided on 1.14 by using multiple -modfiles. - - v.loadMu.Lock() - var locked bool // If true, we hold the mutex and have incremented. - if v.serializeLoads == 0 { - v.loadMu.Unlock() - } else { - locked = true - v.serializeLoads++ - } - defer func() { - if locked { - v.serializeLoads-- - v.loadMu.Unlock() - } - }() - - for { - pkgs, err := packages.Load(cfg, patterns...) - if err == nil || !modConcurrencyError.MatchString(err.Error()) { - return pkgs, err - } - - event.Error(cfg.Context, "Load concurrency error, will retry serially", err) - if !locked { - v.loadMu.Lock() - v.serializeLoads++ - locked = true - } - } -} - // This function will return the main go.mod file for this folder if it exists and whether the -modfile // flag exists for this version of go. func (v *view) modfileFlagExists(ctx context.Context, env []string) (bool, error) { @@ -824,7 +778,7 @@ func (v *view) modfileFlagExists(ctx context.Context, env []string) (bool, error Env: append(env, "GO111MODULE=off"), WorkingDir: v.Folder().Filename(), } - stdout, err := inv.Run(ctx) + stdout, err := v.gocmdRunner.Run(ctx, inv) if err != nil { return false, err } diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 6c5f51f40..280f47e78 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -11,6 +11,7 @@ import ( "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/xcontext" errors "golang.org/x/xerrors" ) @@ -32,14 +33,16 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom if !ok { return nil, err } + cfg := snapshot.Config(ctx) // Run go.mod tidy on the view. inv := gocommand.Invocation{ Verb: "mod", Args: []string{"tidy"}, - Env: snapshot.Config(ctx).Env, + Env: cfg.Env, WorkingDir: snapshot.View().Folder().Filename(), } - if _, err := inv.Run(ctx); err != nil { + gocmdRunner := packagesinternal.GetGoCmdRunner(cfg) + if _, err := gocmdRunner.Run(ctx, inv); err != nil { return nil, err } case "upgrade.dependency": @@ -52,14 +55,16 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom if !ok { return nil, err } + cfg := snapshot.Config(ctx) // Run "go get" on the dependency to upgrade it to the latest version. inv := gocommand.Invocation{ Verb: "get", Args: strings.Split(deps, " "), - Env: snapshot.Config(ctx).Env, + Env: cfg.Env, WorkingDir: snapshot.View().Folder().Filename(), } - if _, err := inv.Run(ctx); err != nil { + gocmdRunner := packagesinternal.GetGoCmdRunner(cfg) + if _, err := gocmdRunner.Run(ctx, inv); err != nil { return nil, err } } diff --git a/internal/lsp/debug/info.go b/internal/lsp/debug/info.go index 8baf1d932..b070c99f1 100644 --- a/internal/lsp/debug/info.go +++ b/internal/lsp/debug/info.go @@ -50,10 +50,10 @@ func PrintVersionInfo(ctx context.Context, w io.Writer, verbose bool, mode Print }) fmt.Fprint(w, "\n") section(w, mode, "Go info", func() { - i := &gocommand.Invocation{ + gocmdRunner := &gocommand.Runner{} + version, err := gocmdRunner.Run(ctx, gocommand.Invocation{ Verb: "version", - } - version, err := i.Run(ctx) + }) if err != nil { panic(err) } diff --git a/internal/lsp/fake/workspace.go b/internal/lsp/fake/workspace.go index 446216a59..1388e1a49 100644 --- a/internal/lsp/fake/workspace.go +++ b/internal/lsp/fake/workspace.go @@ -157,7 +157,8 @@ func (w *Workspace) RunGoCommand(ctx context.Context, verb string, args ...strin Args: args, WorkingDir: w.workdir, } - _, stderr, _, err := inv.RunRaw(ctx) + gocmdRunner := &gocommand.Runner{} + _, stderr, _, err := gocmdRunner.RunRaw(ctx, inv) if err != nil { return err } diff --git a/internal/packagesinternal/packages.go b/internal/packagesinternal/packages.go index b13ce33a3..a88750be2 100644 --- a/internal/packagesinternal/packages.go +++ b/internal/packagesinternal/packages.go @@ -1,7 +1,11 @@ // Package packagesinternal exposes internal-only fields from go/packages. package packagesinternal -import "time" +import ( + "time" + + "golang.org/x/tools/internal/gocommand" +) // Fields must match go list; type Module struct { @@ -25,3 +29,7 @@ type ModuleError struct { var GetForTest = func(p interface{}) string { return "" } var GetModule = func(p interface{}) *Module { return nil } + +var GetGoCmdRunner = func(config interface{}) *gocommand.Runner { return nil } + +var SetGoCmdRunner = func(config interface{}, runner *gocommand.Runner) {} |