From 64bd808b7353cbb5638c19123a9b9b980f277ebb Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 2 Jul 2021 16:06:11 -0400 Subject: [PATCH] internal/lsp/cache: don't report a context error if load succeeded If a context was canceled during load, only report it as a critical error if the load actually failed. Along the way, simplify evaulation of the critical error to use a switch statement. Also await IWL in the second Env used in shared regtests. Presumably it is this Env that is being shutdown prior to IWL, triggering the panicking code-path from golang/go#47030. I wasn't able to reproduce, but all panics are occurring in regtest/misc, and this seems highly plausible. Fixes golang/go#47030 Change-Id: I4df65697f644cff275ab1babb783868fd9e10c2d Reviewed-on: https://go-review.googlesource.com/c/tools/+/332589 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- gopls/internal/regtest/misc/shared_test.go | 1 + internal/lsp/cache/view.go | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/gopls/internal/regtest/misc/shared_test.go b/gopls/internal/regtest/misc/shared_test.go index 129a5ff3e0..e6dc0ac41f 100644 --- a/gopls/internal/regtest/misc/shared_test.go +++ b/gopls/internal/regtest/misc/shared_test.go @@ -31,6 +31,7 @@ func runShared(t *testing.T, testFunc func(env1 *Env, env2 *Env)) { // Create a second test session connected to the same workspace and server // as the first. env2 := NewEnv(env1.Ctx, t, env1.Sandbox, env1.Server, env1.Editor.Config, true) + env2.Await(InitialWorkspaceLoad) testFunc(env1, env2) }) } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index cb08ea0fb4..3f39882469 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -607,35 +607,40 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) { } else { scopes = append(scopes, viewLoadScope("LOAD_VIEW")) } - var err error + + // If we're loading anything, ensure we also load builtin. + // TODO(rstambler): explain the rationale for this. if len(scopes) > 0 { - err = s.load(ctx, firstAttempt, append(scopes, packagePath("builtin"))...) + scopes = append(scopes, packagePath("builtin")) } + err := s.load(ctx, firstAttempt, 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 !firstAttempt && ctx.Err() != nil { + if err != nil && !firstAttempt && ctx.Err() != nil { return } var criticalErr *source.CriticalError - if ctx.Err() != nil { + switch { + case err != nil && ctx.Err() != nil: event.Error(ctx, fmt.Sprintf("initial workspace load: %v", err), err) criticalErr = &source.CriticalError{ MainError: err, } - } else if err != nil { + case err != nil: event.Error(ctx, "initial workspace load failed", err) extractedDiags, _ := s.extractGoCommandErrors(ctx, err.Error()) criticalErr = &source.CriticalError{ MainError: err, DiagList: append(modDiagnostics, extractedDiags...), } - } else if len(modDiagnostics) == 1 { + case len(modDiagnostics) == 1: criticalErr = &source.CriticalError{ MainError: fmt.Errorf(modDiagnostics[0].Message), DiagList: modDiagnostics, } - } else if len(modDiagnostics) > 1 { + case len(modDiagnostics) > 1: criticalErr = &source.CriticalError{ MainError: fmt.Errorf("error loading module names"), DiagList: modDiagnostics,