From b2552ef554f216cb7c6b475538301c39fb1571e1 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 11 Apr 2022 17:17:47 -0400 Subject: [PATCH] internal/lsp: run go mod vendor exclusively to avoid file contention Running go mod vendor can result in vendor/modules.txt being transiently deleted while it is being updated. On Windows this introduces potential problems with file locking, if modules.txt is being read by another go process, such as an ongoing package load. Change the command to use RunGoCommandPiped, which executes serially within the gopls process. For golang/go#49646 Change-Id: If2d1fe5431122ba05014051a0e9c303cf7ee9cb2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/399714 Run-TryBot: Robert Findley Reviewed-by: Bryan Mills gopls-CI: kokoro TryBot-Result: Gopher Robot --- gopls/internal/regtest/misc/vendor_test.go | 14 ++++++++++---- internal/lsp/command.go | 12 ++++++++++-- internal/lsp/source/view.go | 4 ++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/gopls/internal/regtest/misc/vendor_test.go b/gopls/internal/regtest/misc/vendor_test.go index 0e615f2818..324a8006fa 100644 --- a/gopls/internal/regtest/misc/vendor_test.go +++ b/gopls/internal/regtest/misc/vendor_test.go @@ -5,7 +5,6 @@ package misc import ( - "runtime" "testing" . "golang.org/x/tools/internal/lsp/regtest" @@ -27,9 +26,16 @@ var Goodbye error func TestInconsistentVendoring(t *testing.T) { testenv.NeedsGo1Point(t, 14) - if runtime.GOOS == "windows" { - t.Skipf("skipping test due to flakiness on Windows: https://golang.org/issue/49646") - } + + // TODO(golang/go#49646): delete this comment once this test is stable. + // + // In golang/go#49646, this test is reported as flaky on Windows. We believe + // this is due to file contention from go mod vendor that should be resolved. + // If this test proves to still be flaky, skip it. + // + // if runtime.GOOS == "windows" { + // t.Skipf("skipping test due to flakiness on Windows: https://golang.org/issue/49646") + // } const pkgThatUsesVendoring = ` -- go.mod -- diff --git a/internal/lsp/command.go b/internal/lsp/command.go index ae19a7da05..072ee08e80 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -256,11 +256,19 @@ func (c *commandHandler) Vendor(ctx context.Context, args command.URIArg) error progress: "Running go mod vendor", forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { - _, err := deps.snapshot.RunGoCommandDirect(ctx, source.Normal|source.AllowNetwork, &gocommand.Invocation{ + // Use RunGoCommandPiped here so that we don't compete with any other go + // command invocations. go mod vendor deletes modules.txt before recreating + // it, and therefore can run into file locking issues on Windows if that + // file is in use by another process, such as go list. + // + // If golang/go#44119 is resolved, go mod vendor will instead modify + // modules.txt in-place. In that case we could theoretically allow this + // command to run concurrently. + err := deps.snapshot.RunGoCommandPiped(ctx, source.Normal|source.AllowNetwork, &gocommand.Invocation{ Verb: "mod", Args: []string{"vendor"}, WorkingDir: filepath.Dir(args.URI.SpanURI().Filename()), - }) + }, &bytes.Buffer{}, &bytes.Buffer{}) return err }) } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index d263c1a9bf..f698a07e3e 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -98,6 +98,10 @@ type Snapshot interface { // RunGoCommandPiped runs the given `go` command, writing its output // to stdout and stderr. Verb, Args, and WorkingDir must be specified. + // + // RunGoCommandPiped runs the command serially using gocommand.RunPiped, + // enforcing that this command executes exclusively to other commands on the + // server. RunGoCommandPiped(ctx context.Context, mode InvocationFlags, inv *gocommand.Invocation, stdout, stderr io.Writer) error // RunGoCommandDirect runs the given `go` command. Verb, Args, and