internal/lsp: cancel the initial workspace load when view shuts down
The error messages from view cancellation clutter up the logs when
testing, especially if you're running a single subtest.
A few quick staticcheck fixes in the CL also.
Change-Id: Ia1ed5360ac754023c589ed526ec0ed3e94a79b2f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237637
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 34448af..44636bc 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -159,7 +159,9 @@
}
// Initialize the view without blocking.
- go v.initialize(xcontext.Detach(ctx), v.snapshot)
+ initCtx, initCancel := context.WithCancel(xcontext.Detach(ctx))
+ v.initCancel = initCancel
+ go v.initialize(initCtx, v.snapshot)
return v, v.snapshot, nil
}
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index d4a59db..a5bf043 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -87,6 +87,7 @@
// 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
// initializedErr needs no mutex, since any access to it happens after it
// has been set.
@@ -525,7 +526,9 @@
}
func (v *View) shutdown(ctx context.Context) {
- // TODO: Cancel the view's initialization.
+ // Cancel the initial workspace load if it is still running.
+ v.initCancel()
+
v.mu.Lock()
defer v.mu.Unlock()
if v.cancel != nil {
@@ -561,6 +564,9 @@
defer close(v.initialized)
if err := s.load(ctx, viewLoadScope("LOAD_VIEW"), packagePath("builtin")); err != nil {
+ if ctx.Err() != nil {
+ return
+ }
v.initializedErr = err
event.Error(ctx, "initial workspace load failed", err)
}
diff --git a/internal/lsp/cmd/capabilities_test.go b/internal/lsp/cmd/capabilities_test.go
index aebed67..7b6d3bd 100644
--- a/internal/lsp/cmd/capabilities_test.go
+++ b/internal/lsp/cmd/capabilities_test.go
@@ -143,6 +143,12 @@
t.Errorf("textEdit is not a *protocol.TextEdit, instead it is %T", textEdit)
}
}
+ if err := c.Server.Shutdown(ctx); err != nil {
+ t.Fatal(err)
+ }
+ if err := c.Server.Exit(ctx); err != nil {
+ t.Fatal(err)
+ }
}
func validateCapabilities(result *protocol.InitializeResult) error {
diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go
index f8e72f0..023729d 100644
--- a/internal/lsp/cmd/cmd.go
+++ b/internal/lsp/cmd/cmd.go
@@ -496,3 +496,8 @@
//TODO: right now calling exit terminates the process, we should rethink that
//server.Exit(ctx)
}
+
+// Implement io.Closer.
+func (c *cmdClient) Close() error {
+ return nil
+}
diff --git a/internal/lsp/general.go b/internal/lsp/general.go
index 89dff8d..c2051ce 100644
--- a/internal/lsp/general.go
+++ b/internal/lsp/general.go
@@ -308,10 +308,12 @@
func (s *Server) exit(ctx context.Context) error {
s.stateMu.Lock()
defer s.stateMu.Unlock()
- //TODO: would be nice to have a better way to find the conn close method
+
+ // TODO: We need a better way to find the conn close method.
s.client.(io.Closer).Close()
+
if s.state != serverShutDown {
- //TODO: we shoud be able to do better than this
+ // TODO: We should be able to do better than this.
os.Exit(1)
}
// we don't terminate the process on a normal exit, we just allow it to
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index d3be652..ff31a10 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -55,20 +55,21 @@
options := tests.DefaultOptions()
session.SetOptions(options)
options.Env = datum.Config.Env
- v, snapshot, err := session.NewView(ctx, datum.Config.Dir, span.URIFromPath(datum.Config.Dir), options)
+ view, snapshot, err := session.NewView(ctx, datum.Config.Dir, span.URIFromPath(datum.Config.Dir), options)
if err != nil {
t.Fatal(err)
}
+ defer view.Shutdown(ctx)
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
tests.EnableAllAnalyzers(snapshot, &options)
- v.SetOptions(ctx, options)
+ view.SetOptions(ctx, options)
// Check to see if the -modfile flag is available, this is basically a check
// to see if the go version >= 1.14. Otherwise, the modfile specific tests
// will always fail if this flag is not available.
- for _, flag := range v.Snapshot().Config(ctx).BuildFlags {
+ for _, flag := range view.Snapshot().Config(ctx).BuildFlags {
if strings.Contains(flag, "-modfile=") {
datum.ModfileFlagAvailable = true
break
@@ -385,9 +386,8 @@
},
Range: rng,
})
-
- if len(actions) == 0 {
- return
+ if err != nil {
+ t.Fatal(err)
}
for _, action := range actions {
// There may be more code actions available at spn (Span),
@@ -409,7 +409,7 @@
}
return
}
- t.Fatalf("expected code action: %v but none", title)
+ t.Fatalf("expected code action %q, but got none", title)
}
func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string) {
diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go
index 9ff9b3b..183fd8e 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -56,7 +56,7 @@
}
func formatSource(ctx context.Context, fh FileHandle) ([]byte, error) {
- ctx, done := event.Start(ctx, "source.formatSource")
+ _, done := event.Start(ctx, "source.formatSource")
defer done()
data, err := fh.Read()
@@ -212,7 +212,7 @@
}
func computeTextEdits(ctx context.Context, view View, fh FileHandle, m *protocol.ColumnMapper, formatted string) ([]protocol.TextEdit, error) {
- ctx, done := event.Start(ctx, "source.computeTextEdits")
+ _, done := event.Start(ctx, "source.computeTextEdits")
defer done()
data, err := fh.Read()
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index af702f1..93b4114 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -56,6 +56,8 @@
if err != nil {
t.Fatal(err)
}
+ defer view.Shutdown(ctx)
+
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
tests.EnableAllAnalyzers(snapshot, &options)
