From 3393d29bb9feddd4fb8d6d2f81faf9f50b70616d Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 5 Dec 2019 16:37:47 -0500 Subject: [PATCH] internal/lsp: propagate and handle context cancellation errors We don't distinguish between genuine errors and context cancellation in diagnostics, which often results in superfluous logging of these errors. Avoid spamming the logs with them by checking. Also, remove the logic for sending undelivered diagnostics. It's a relic of old bugs and isn't useful. Change-Id: I7df226539b9b1eb49ab3aae8d7b0a67f59fb3058 Reviewed-on: https://go-review.googlesource.com/c/tools/+/210197 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/analysis.go | 20 ++--------- internal/lsp/diagnostics.go | 56 +++++++++++++----------------- internal/lsp/source/diagnostics.go | 10 +++--- internal/lsp/watched_files.go | 7 ++-- 4 files changed, 37 insertions(+), 56 deletions(-) diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index ff502891ad..3e31b6f9ed 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -38,8 +38,7 @@ func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*analysis for _, ah := range roots { diagnostics, _, err := ah.analyze(ctx) if err != nil { - log.Error(ctx, "no results", err) - continue + return nil, err } results = append(results, diagnostics...) } @@ -148,7 +147,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id packageID, mode source.P func (act *actionHandle) analyze(ctx context.Context) ([]*source.Error, interface{}, error) { v := act.handle.Get(ctx) if v == nil { - return nil, nil, errors.Errorf("no analyses for %s", act.pkg.ID()) + return nil, nil, ctx.Err() } data, ok := v.(*actionData) if !ok { @@ -160,21 +159,6 @@ func (act *actionHandle) analyze(ctx context.Context) ([]*source.Error, interfac return data.diagnostics, data.result, data.err } -func (act *actionHandle) cached() ([]*source.Error, interface{}, error) { - v := act.handle.Cached() - if v == nil { - return nil, nil, errors.Errorf("no cached analyses for %s", act.pkg.ID()) - } - data, ok := v.(*actionData) - if !ok { - return nil, nil, errors.Errorf("unexpected type for cached analysis %s:%s", act.pkg.ID(), act.analyzer.Name) - } - if data == nil { - return nil, nil, errors.Errorf("unexpected nil cached analysis for %s:%s", act.pkg.ID(), act.analyzer.Name) - } - return data.diagnostics, data.result, data.err -} - func buildActionKey(a *analysis.Analyzer, ph *packageHandle) string { return hashContents([]byte(fmt.Sprintf("%p %s", a, string(ph.key)))) } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index af7b59b165..40fff9d205 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -50,7 +50,7 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot) { } } -func (s *Server) diagnoseFile(snapshot source.Snapshot, uri span.URI) error { +func (s *Server) diagnoseFile(snapshot source.Snapshot, uri span.URI) { ctx := snapshot.View().BackgroundContext() ctx, done := trace.StartSpan(ctx, "lsp:background-worker") defer done() @@ -59,57 +59,51 @@ func (s *Server) diagnoseFile(snapshot source.Snapshot, uri span.URI) error { f, err := snapshot.View().GetFile(ctx, uri) if err != nil { - return err + log.Error(ctx, "diagnoseFile: no file", err) + return } reports, warningMsg, err := source.Diagnostics(ctx, snapshot, f, true, snapshot.View().Options().DisabledAnalyses) - if err != nil { - return err - } + // Check the warning message first. if warningMsg != "" { s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ Type: protocol.Info, Message: warningMsg, }) } + if err != nil { + if err != context.Canceled { + log.Error(ctx, "diagnoseFile: could not generate diagnostics", err) + } + return + } // Publish empty diagnostics for files. s.publishReports(ctx, reports, true) - return nil } func (s *Server) publishReports(ctx context.Context, reports map[source.FileIdentity][]source.Diagnostic, publishEmpty bool) { - undelivered := make(map[source.FileIdentity][]source.Diagnostic) + // Check for context cancellation before publishing diagnostics. + if ctx.Err() != nil { + return + } + for fileID, diagnostics := range reports { + // Don't deliver diagnostics if the context has already been canceled. + if ctx.Err() != nil { + break + } // Don't publish empty diagnostics unless specified. if len(diagnostics) == 0 && !publishEmpty { continue } - if err := s.publishDiagnostics(ctx, fileID, diagnostics); err != nil { - undelivered[fileID] = diagnostics - - log.Error(ctx, "failed to deliver diagnostic (will retry)", err, telemetry.File) + if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ + Diagnostics: toProtocolDiagnostics(ctx, diagnostics), + URI: protocol.NewURI(fileID.URI), + Version: fileID.Version, + }); err != nil { + log.Error(ctx, "failed to deliver diagnostic", err, telemetry.File) continue } - // In case we had old, undelivered diagnostics. - delete(undelivered, fileID) } - // Any time we compute diagnostics, make sure to also send along any - // undelivered ones (only for remaining URIs). - for uri, diagnostics := range undelivered { - if err := s.publishDiagnostics(ctx, uri, diagnostics); err != nil { - log.Error(ctx, "failed to deliver diagnostic for (will not retry)", err, telemetry.File) - } - - // If we fail to deliver the same diagnostics twice, just give up. - delete(undelivered, uri) - } -} - -func (s *Server) publishDiagnostics(ctx context.Context, fileID source.FileIdentity, diagnostics []source.Diagnostic) error { - return s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ - Diagnostics: toProtocolDiagnostics(ctx, diagnostics), - URI: protocol.NewURI(fileID.URI), - Version: fileID.Version, - }) } func toProtocolDiagnostics(ctx context.Context, diagnostics []source.Diagnostic) []protocol.Diagnostic { diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 9f2d7b7c45..d53249b694 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -56,15 +56,13 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, withAnalysis bo // not correctly configured. Report errors, if possible. var warningMsg string if len(ph.MissingDependencies()) > 0 { - warningMsg, err = checkCommonErrors(ctx, snapshot.View(), f.URI()) - if err != nil { + if warningMsg, err = checkCommonErrors(ctx, snapshot.View(), f.URI()); err != nil { log.Error(ctx, "error checking common errors", err, telemetry.File.Of(f.URI)) } } pkg, err := ph.Check(ctx) if err != nil { - log.Error(ctx, "no package for file", err) - return singleDiagnostic(fh.Identity(), "%s is not part of a package", f.URI()), "", nil + return nil, "", err } // Prepare the reports we will send for the files in this package. reports := make(map[FileIdentity][]Diagnostic) @@ -82,6 +80,10 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, withAnalysis bo if !diagnostics(ctx, snapshot, pkg, reports) && withAnalysis { // If we don't have any list, parse, or type errors, run analyses. if err := analyses(ctx, snapshot, ph, disabledAnalyses, reports); err != nil { + // Exit early if the context has been canceled. + if err == context.Canceled { + return nil, "", err + } log.Error(ctx, "failed to run analyses", err, telemetry.File.Of(f.URI())) } } diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go index d708bd0d3d..30c243ec5c 100644 --- a/internal/lsp/watched_files.go +++ b/internal/lsp/watched_files.go @@ -72,9 +72,10 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did // If this was the only file in the package, clear its diagnostics. if otherFile == nil { - if err := s.publishDiagnostics(ctx, source.FileIdentity{ - URI: uri, - }, []source.Diagnostic{}); err != nil { + if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ + URI: protocol.NewURI(uri), + Version: fh.Identity().Version, + }); err != nil { log.Error(ctx, "failed to clear diagnostics", err, telemetry.URI.Of(uri)) } return nil