gopls/internal/cache: don't mark initialized after cancellation
When a snapshot reinitialization is canceled, we should not mark it as
complete. Initialization is special, in that it does not set
GOPROXY=off. Canceling it can result in subsequent load failures.
Fixes golang/go#57626
Change-Id: I9bc2c9226539fa8d240ecf2be62cc8d9915e5642
Reviewed-on: https://go-review.googlesource.com/c/tools/+/460795
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index a873b50..c2ad0d4 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -657,11 +657,22 @@
s.collectAllKnownSubdirs(ctx)
}
-func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) {
+func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadErr error) {
+ // A failure is retryable if it may have been due to context cancellation,
+ // and this is not the initial workspace load (firstAttempt==true).
+ //
+ // The IWL runs on a detached context with a long (~10m) timeout, so
+ // if the context was canceled we consider loading to have failed
+ // permanently.
+ retryableFailure := func() bool {
+ return loadErr != nil && ctx.Err() != nil && !firstAttempt
+ }
defer func() {
- s.mu.Lock()
- s.initialized = true
- s.mu.Unlock()
+ if !retryableFailure() {
+ s.mu.Lock()
+ s.initialized = true
+ s.mu.Unlock()
+ }
if firstAttempt {
close(s.view.initialWorkspaceLoad)
}
@@ -719,26 +730,24 @@
if len(scopes) > 0 {
scopes = append(scopes, packageLoadScope("builtin"))
}
- err := s.load(ctx, true, scopes...)
+ loadErr = s.load(ctx, true, scopes...)
- // If the context is canceled on the first attempt, loading has failed
- // because the go command has timed out--that should be a critical error.
- if err != nil && !firstAttempt && ctx.Err() != nil {
- return
+ if retryableFailure() {
+ return loadErr
}
var criticalErr *source.CriticalError
switch {
- case err != nil && ctx.Err() != nil:
- event.Error(ctx, fmt.Sprintf("initial workspace load: %v", err), err)
+ case loadErr != nil && ctx.Err() != nil:
+ event.Error(ctx, fmt.Sprintf("initial workspace load: %v", loadErr), loadErr)
criticalErr = &source.CriticalError{
- MainError: err,
+ MainError: loadErr,
}
- case err != nil:
- event.Error(ctx, "initial workspace load failed", err)
- extractedDiags := s.extractGoCommandErrors(ctx, err)
+ case loadErr != nil:
+ event.Error(ctx, "initial workspace load failed", loadErr)
+ extractedDiags := s.extractGoCommandErrors(ctx, loadErr)
criticalErr = &source.CriticalError{
- MainError: err,
+ MainError: loadErr,
Diagnostics: append(modDiagnostics, extractedDiags...),
}
case len(modDiagnostics) == 1:
@@ -757,6 +766,7 @@
s.mu.Lock()
defer s.mu.Unlock()
s.initializedErr = criticalErr
+ return loadErr
}
// invalidateContent invalidates the content of a Go file,
diff --git a/gopls/internal/lsp/regtest/expectation.go b/gopls/internal/lsp/regtest/expectation.go
index c30a075..3b623d0 100644
--- a/gopls/internal/lsp/regtest/expectation.go
+++ b/gopls/internal/lsp/regtest/expectation.go
@@ -328,6 +328,13 @@
return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidSave), saves, true)
}
+// StartedChangeWatchedFiles expects that the server has at least started
+// processing all didChangeWatchedFiles notifications sent from the client.
+func (e *Env) StartedChangeWatchedFiles() Expectation {
+ changes := e.Editor.Stats().DidChangeWatchedFiles
+ return StartedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), changes)
+}
+
// DoneWithChangeWatchedFiles expects all didChangeWatchedFiles notifications
// currently sent by the editor to be completely processed.
func (e *Env) DoneWithChangeWatchedFiles() Expectation {
diff --git a/gopls/internal/regtest/workspace/metadata_test.go b/gopls/internal/regtest/workspace/metadata_test.go
index 55bc01e..4620b93 100644
--- a/gopls/internal/regtest/workspace/metadata_test.go
+++ b/gopls/internal/regtest/workspace/metadata_test.go
@@ -5,6 +5,7 @@
package workspace
import (
+ "strings"
"testing"
. "golang.org/x/tools/gopls/internal/lsp/regtest"
@@ -109,3 +110,80 @@
)
})
}
+
+func TestReinitializeRepeatedly(t *testing.T) {
+ testenv.NeedsGo1Point(t, 18) // uses go.work
+
+ const multiModule = `
+-- go.work --
+go 1.18
+
+use (
+ moda/a
+ modb
+)
+-- moda/a/go.mod --
+module a.com
+
+require b.com v1.2.3
+-- moda/a/go.sum --
+b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI=
+b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8=
+-- moda/a/a.go --
+package a
+
+import (
+ "b.com/b"
+)
+
+func main() {
+ var x int
+ _ = b.Hello()
+ // AAA
+}
+-- modb/go.mod --
+module b.com
+
+-- modb/b/b.go --
+package b
+
+func Hello() int {
+ var x int
+}
+`
+ WithOptions(
+ ProxyFiles(workspaceModuleProxy),
+ Settings{
+ // For this test, we want workspace diagnostics to start immediately
+ // during change processing.
+ "diagnosticsDelay": "0",
+ },
+ ).Run(t, multiModule, func(t *testing.T, env *Env) {
+ env.OpenFile("moda/a/a.go")
+ env.AfterChange()
+
+ // This test verifies that we fully process workspace reinitialization
+ // (which allows GOPROXY), even when the reinitialized snapshot is
+ // invalidated by subsequent changes.
+ //
+ // First, update go.work to remove modb. This will cause reinitialization
+ // to fetch b.com from the proxy.
+ env.WriteWorkspaceFile("go.work", "go 1.18\nuse moda/a")
+ // Next, wait for gopls to start processing the change. Because we've set
+ // diagnosticsDelay to zero, this will start diagnosing the workspace (and
+ // try to reinitialize on the snapshot context).
+ env.Await(env.StartedChangeWatchedFiles())
+ // Finally, immediately make a file change to cancel the previous
+ // operation. This is racy, but will usually cause initialization to be
+ // canceled.
+ env.RegexpReplace("moda/a/a.go", "AAA", "BBB")
+ env.AfterChange()
+ // Now, to satisfy a definition request, gopls will try to reload moda. But
+ // without access to the proxy (because this is no longer a
+ // reinitialization), this loading will fail.
+ got, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
+ if want := "b.com@v1.2.3/b/b.go"; !strings.HasSuffix(got, want) {
+ t.Errorf("expected %s, got %v", want, got)
+ }
+ })
+}
