From 0a5cd101910672cd55ba2bc84716813f2f477fd2 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Sun, 12 Jul 2020 01:52:22 -0400 Subject: [PATCH] internal/lsp: handle unknown revision in go.mod file This change ensures that, when the initial workspace load fails, we re-run it if the go.mod file changes. Previously, if a user opened a workspace with a corrupt go.mod file, we never recovered. To reinitialize the workspace on-demand, we use the initializeOnce field as an indicator of whether or not we should reinitialize. Every call to awaitInitialized (which is called by all functions that need the IWL), passes through the initialization code. If a retry isn't necessary, this is a no-op, but if it is, we will call the initialization logic. Only the first attempt uses a detached context; subsequent attempts can be canceled by their contexts. To indicate that we should reinitialize, we call maybeReinitialize. Right now, we only call this when the go.mod file changes. In the future, we may need it in other cases. Fixes golang/go#38232 Change-Id: I77eefebb0ac38fbd0fe2c7da09c864eba45b075f Reviewed-on: https://go-review.googlesource.com/c/tools/+/242159 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/session.go | 48 ++++++++++------ internal/lsp/cache/view.go | 85 ++++++++++++++++++++++------ internal/lsp/diagnostics.go | 65 +++++++++++++-------- internal/lsp/regtest/env.go | 8 ++- internal/lsp/regtest/modfile_test.go | 75 ++++++++++++++++++++++++ 5 files changed, 219 insertions(+), 62 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index bec8fce39a..3fadd36012 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -128,17 +128,19 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, backgroundCtx, cancel := context.WithCancel(baseCtx) v := &View{ - session: s, - initialized: make(chan struct{}), - id: strconv.FormatInt(index, 10), - options: options, - baseCtx: baseCtx, - backgroundCtx: backgroundCtx, - cancel: cancel, - name: name, - folder: folder, - filesByURI: make(map[span.URI]*fileBase), - filesByBase: make(map[string][]*fileBase), + session: s, + initialized: make(chan struct{}), + initializationSema: make(chan struct{}, 1), + initializeOnce: &sync.Once{}, + id: strconv.FormatInt(index, 10), + options: options, + baseCtx: baseCtx, + backgroundCtx: backgroundCtx, + cancel: cancel, + name: name, + folder: folder, + filesByURI: make(map[span.URI]*fileBase), + filesByBase: make(map[string][]*fileBase), snapshot: &snapshot{ id: snapshotID, packages: make(map[packageKey]*packageHandle), @@ -171,8 +173,8 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, // Initialize the view without blocking. initCtx, initCancel := context.WithCancel(xcontext.Detach(ctx)) - v.initCancel = initCancel - go v.initialize(initCtx, v.snapshot) + v.initCancelFirstAttempt = initCancel + go v.initialize(initCtx, v.snapshot, true) return v, v.snapshot, nil } @@ -324,7 +326,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif if err != nil { return nil, err } - forceReloadMetadata := false + var forceReloadMetadata bool for _, c := range changes { if c.Action == source.InvalidateMetadata { forceReloadMetadata = true @@ -344,15 +346,27 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif if _, ok := views[view]; !ok { views[view] = make(map[span.URI]source.FileHandle) } - if o, ok := overlays[c.URI]; ok { - views[view][c.URI] = o + var ( + fh source.FileHandle + ok bool + err error + ) + if fh, ok = overlays[c.URI]; ok { + views[view][c.URI] = fh } else { - fh, err := s.cache.getFile(ctx, c.URI) + fh, err = s.cache.getFile(ctx, c.URI) if err != nil { return nil, err } views[view][c.URI] = fh } + // If the file change is to a go.mod file, and initialization for + // the view has previously failed, we should attempt to retry. + // TODO(rstambler): We can use unsaved contents with -modfile, so + // maybe we should do that and retry on any change? + if fh.Kind() == source.Mod && (c.OnDisk || c.Action == source.Save) { + view.maybeReinitialize() + } } } var snapshots []source.Snapshot diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 95f03b173a..2d2b1e6798 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -86,17 +86,30 @@ type View struct { snapshotMu sync.Mutex snapshot *snapshot - // initialized is closed when the view has been fully initialized. - // On initialization, the view's workspace packages are loaded. - // All of the fields below are set as part of initialization. - // If we failed to load, we don't re-try to avoid too many go/packages calls. - initializeOnce sync.Once - initialized chan struct{} - initCancel context.CancelFunc + // initialized is closed when the view has been fully initialized. On + // initialization, the view's workspace packages are loaded. All of the + // fields below are set as part of initialization. If we failed to load, we + // only retry if the go.mod file changes, to avoid too many go/packages + // calls. + // + // When the view is created, initializeOnce is non-nil, initialized is + // open, and initCancelFirstAttempt can be used to terminate + // initialization. Once initialization completes, initializedErr may be set + // and initializeOnce becomes nil. If initializedErr is non-nil, + // initialization may be retried (depending on how files are changed). To + // indicate that initialization should be retried, initializeOnce will be + // set. The next time a caller requests workspace packages, the + // initialization will retry. + initialized chan struct{} + initCancelFirstAttempt context.CancelFunc - // initializedErr needs no mutex, since any access to it happens after it - // has been set. - initializedErr error + // initializationSema is used as a mutex to guard initializeOnce and + // initializedErr, which will be updated after each attempt to initialize + // the view. We use a channel instead of a mutex to avoid blocking when a + // context is canceled. + initializationSema chan struct{} + initializeOnce *sync.Once + initializedErr error // builtin pins the AST and package for builtin.go in memory. builtin *builtinPackageHandle @@ -639,7 +652,7 @@ func (v *View) Shutdown(ctx context.Context) { func (v *View) shutdown(ctx context.Context) { // Cancel the initial workspace load if it is still running. - v.initCancel() + v.initCancelFirstAttempt() v.mu.Lock() defer v.mu.Unlock() @@ -702,25 +715,48 @@ func (v *View) getSnapshot() *snapshot { return v.snapshot } -func (v *View) initialize(ctx context.Context, s *snapshot) { - v.initializeOnce.Do(func() { - defer close(v.initialized) +func (v *View) initialize(ctx context.Context, s *snapshot, firstAttempt bool) { + select { + case <-ctx.Done(): + return + case v.initializationSema <- struct{}{}: + } - if err := s.load(ctx, viewLoadScope("LOAD_VIEW"), packagePath("builtin")); err != nil { - if ctx.Err() != nil { - return + defer func() { + <-v.initializationSema + }() + + if v.initializeOnce == nil { + return + } + v.initializeOnce.Do(func() { + defer func() { + v.initializeOnce = nil + if firstAttempt { + close(v.initialized) } - v.initializedErr = err + }() + + err := s.load(ctx, viewLoadScope("LOAD_VIEW"), packagePath("builtin")) + if ctx.Err() != nil { + return + } + if err != nil { event.Error(ctx, "initial workspace load failed", err) } + v.initializedErr = err }) } func (v *View) awaitInitialized(ctx context.Context) { select { case <-ctx.Done(): + return case <-v.initialized: } + // We typically prefer to run something as intensive as the IWL without + // blocking. I'm not sure if there is a way to do that here. + v.initialize(ctx, v.getSnapshot(), false) } // invalidateContent invalidates the content of a Go file, @@ -756,6 +792,19 @@ func (v *View) cancelBackground() { v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx) } +func (v *View) maybeReinitialize() { + v.initializationSema <- struct{}{} + defer func() { + <-v.initializationSema + }() + + if v.initializedErr == nil { + return + } + var once sync.Once + v.initializeOnce = &once +} + func (v *View) setBuildInformation(ctx context.Context, folder span.URI, env []string, modfileFlagEnabled bool) error { if err := checkPathCase(folder.Filename()); err != nil { return fmt.Errorf("invalid workspace configuration: %w", err) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index b1f9085659..e5a83ad7de 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -28,7 +28,7 @@ func (s *Server) diagnoseDetached(snapshot source.Snapshot) { ctx = xcontext.Detach(ctx) reports, shows := s.diagnose(ctx, snapshot, false) if shows != nil { - // If a view has been created or the configuration changed, warn the user + // If a view has been created or the configuration changed, warn the user. s.client.ShowMessage(ctx, shows) } s.publishReports(ctx, snapshot, reports) @@ -36,7 +36,8 @@ func (s *Server) diagnoseDetached(snapshot source.Snapshot) { func (s *Server) diagnoseSnapshot(snapshot source.Snapshot) { ctx := snapshot.View().BackgroundContext() - // Ignore possible workspace configuration warnings in the normal flow + + // Ignore possible workspace configuration warnings in the normal flow. reports, _ := s.diagnose(ctx, snapshot, false) s.publishReports(ctx, snapshot, reports) } @@ -67,7 +68,6 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA if ctx.Err() != nil { return nil, nil } - modURI := snapshot.View().ModFile() for id, diags := range reports { if id.URI == "" { event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename())) @@ -82,27 +82,8 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA // Diagnose all of the packages in the workspace. wsPackages, err := snapshot.WorkspacePackages(ctx) - if err == source.InconsistentVendoring { - item, err := s.client.ShowMessageRequest(ctx, &protocol.ShowMessageRequestParams{ - Type: protocol.Error, - Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". -See https://github.com/golang/go/issues/39164 for more detail on this issue.`, - Actions: []protocol.MessageActionItem{ - {Title: "go mod vendor"}, - }, - }) - if item == nil || err != nil { - return nil, nil - } - if err := s.directGoModCommand(ctx, protocol.URIFromSpanURI(modURI), "mod", []string{"vendor"}...); err != nil { - return nil, &protocol.ShowMessageParams{ - Type: protocol.Error, - Message: fmt.Sprintf(`"go mod vendor" failed with %v`, err), - } - } - return nil, nil - } else if err != nil { - event.Error(ctx, "failed to load workspace packages, skipping diagnostics", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) + if err != nil { + s.handleFatalErrors(ctx, snapshot, err) return nil, nil } var shows *protocol.ShowMessageParams @@ -247,3 +228,39 @@ func toProtocolDiagnostics(diagnostics []*source.Diagnostic) []protocol.Diagnost } return reports } + +func (s *Server) handleFatalErrors(ctx context.Context, snapshot source.Snapshot, err error) { + switch err { + case source.InconsistentVendoring: + item, err := s.client.ShowMessageRequest(ctx, &protocol.ShowMessageRequestParams{ + Type: protocol.Error, + Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". +See https://github.com/golang/go/issues/39164 for more detail on this issue.`, + Actions: []protocol.MessageActionItem{ + {Title: "go mod vendor"}, + }, + }) + if item == nil || err != nil { + event.Error(ctx, "go mod vendor ShowMessageRequest failed", err, tag.Directory.Of(snapshot.View().Folder())) + return + } + modURI := snapshot.View().ModFile() + if err := s.directGoModCommand(ctx, protocol.URIFromSpanURI(modURI), "mod", []string{"vendor"}...); err != nil { + if err := s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: fmt.Sprintf(`"go mod vendor" failed with %v`, err), + }); err != nil { + event.Error(ctx, "ShowMessage failed", err) + } + } + default: + msg := "failed to load workspace packages, skipping diagnostics" + event.Error(ctx, msg, err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) + if err := s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: fmt.Sprintf("%s: %v", msg, err), + }); err != nil { + event.Error(ctx, "ShowMessage failed", err, tag.Directory.Of(snapshot.View().Folder())) + } + } +} diff --git a/internal/lsp/regtest/env.go b/internal/lsp/regtest/env.go index b1ef95c2ba..5ce13ca04c 100644 --- a/internal/lsp/regtest/env.go +++ b/internal/lsp/regtest/env.go @@ -364,11 +364,13 @@ func EmptyShowMessage(title string) SimpleExpectation { } } -// SomeShowMessage asserts that the editor has received a ShowMessage. +// SomeShowMessage asserts that the editor has received a ShowMessage with the given title. func SomeShowMessage(title string) SimpleExpectation { check := func(s State) (Verdict, interface{}) { - if len(s.showMessage) > 0 { - return Met, title + for _, m := range s.showMessage { + if strings.Contains(m.Message, title) { + return Met, m + } } return Unmet, nil } diff --git a/internal/lsp/regtest/modfile_test.go b/internal/lsp/regtest/modfile_test.go index 503a789c09..b71fdd3d47 100644 --- a/internal/lsp/regtest/modfile_test.go +++ b/internal/lsp/regtest/modfile_test.go @@ -322,3 +322,78 @@ require ( } }, WithProxy(badModule)) } + +// Reproduces golang/go#38232. +func TestUnknownRevision(t *testing.T) { + const unknown = ` +-- go.mod -- +module mod.com + +require ( + example.com v1.2.2 +) +-- main.go -- +package main + +import "example.com/blah" + +func main() { + var x = blah.Name +} +` + + // Start from a bad state/bad IWL, and confirm that we recover. + t.Run("bad", func(t *testing.T) { + runner.Run(t, unknown, func(t *testing.T, env *Env) { + env.Await( + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromInitialWorkspaceLoad), 1), + ) + env.OpenFile("go.mod") + env.Await( + SomeShowMessage("failed to load workspace packages, skipping diagnostics"), + ) + env.RegexpReplace("go.mod", "v1.2.2", "v1.2.3") + env.Editor.SaveBufferWithoutActions(env.Ctx, "go.mod") // go.mod changes must be on disk + env.Await( + env.DiagnosticAtRegexp("main.go", "x = "), + ) + }, WithProxy(proxy)) + }) + + const known = ` +-- go.mod -- +module mod.com + +require ( + example.com v1.2.3 +) +-- main.go -- +package main + +import "example.com/blah" + +func main() { + var x = blah.Name +} +` + // Start from a good state, transform to a bad state, and confirm that we + // still recover. + t.Run("good", func(t *testing.T) { + runner.Run(t, known, func(t *testing.T, env *Env) { + env.OpenFile("go.mod") + env.Await( + env.DiagnosticAtRegexp("main.go", "x = "), + ) + env.RegexpReplace("go.mod", "v1.2.3", "v1.2.2") + env.Editor.SaveBufferWithoutActions(env.Ctx, "go.mod") // go.mod changes must be on disk + env.Await( + SomeShowMessage("failed to load workspace packages, skipping diagnostics"), + ) + env.RegexpReplace("go.mod", "v1.2.2", "v1.2.3") + env.Editor.SaveBufferWithoutActions(env.Ctx, "go.mod") // go.mod changes must be on disk + env.Await( + env.DiagnosticAtRegexp("main.go", "x = "), + ) + }, WithProxy(proxy)) + }) +}