internal/lsp/cache: don't panic in Snapshot on a shutdown view
There are many places where we may use a view asynchronously while it is
shutting down. In these cases, it should be fine to fail or skip the
view when we fail to acquire its snapshot.
Fixes golang/go#56466
Change-Id: Icc4edfc1b951c40c8aa42a18ba4ecbf85621523d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/462820
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go
index ab42d29..e418a00 100644
--- a/gopls/internal/lsp/cache/session.go
+++ b/gopls/internal/lsp/cache/session.go
@@ -593,7 +593,10 @@
var snapshots []*snapshot
s.viewMu.Lock()
for _, v := range s.views {
- snapshot, release := v.getSnapshot()
+ snapshot, release, err := v.getSnapshot()
+ if err != nil {
+ continue // view is shut down; continue with others
+ }
defer release()
snapshots = append(snapshots, snapshot)
}
@@ -795,7 +798,10 @@
defer s.viewMu.Unlock()
patterns := map[string]struct{}{}
for _, view := range s.views {
- snapshot, release := view.getSnapshot()
+ snapshot, release, err := view.getSnapshot()
+ if err != nil {
+ continue // view is shut down; continue with others
+ }
for k, v := range snapshot.fileWatchingGlobPatterns(ctx) {
patterns[k] = v
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 0eef84e..51d7a79 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -43,7 +43,16 @@
type snapshot struct {
sequenceID uint64
globalID source.GlobalSnapshotID
- view *View
+
+ // TODO(rfindley): the snapshot holding a reference to the view poses
+ // lifecycle problems: a view may be shut down and waiting for work
+ // associated with this snapshot to complete. While most accesses of the view
+ // are benign (options or workspace information), this is not formalized and
+ // it is wrong for the snapshot to use a shutdown view.
+ //
+ // Fix this by passing options and workspace information to the snapshot,
+ // both of which should be immutable for the snapshot.
+ view *View
cancel func()
backgroundCtx context.Context
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index ea9016e..fe1479d 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -9,6 +9,7 @@
"bytes"
"context"
"encoding/json"
+ "errors"
"fmt"
"io/ioutil"
"os"
@@ -677,17 +678,17 @@
return false
}
-func (v *View) Snapshot(ctx context.Context) (source.Snapshot, func()) {
+func (v *View) Snapshot() (source.Snapshot, func(), error) {
return v.getSnapshot()
}
-func (v *View) getSnapshot() (*snapshot, func()) {
+func (v *View) getSnapshot() (*snapshot, func(), error) {
v.snapshotMu.Lock()
defer v.snapshotMu.Unlock()
if v.snapshot == nil {
- panic("getSnapshot called after shutdown")
+ return nil, nil, errors.New("view is shutdown")
}
- return v.snapshot, v.snapshot.Acquire()
+ return v.snapshot, v.snapshot.Acquire(), nil
}
func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) {
diff --git a/gopls/internal/lsp/general.go b/gopls/internal/lsp/general.go
index 87a86f4..af9aff9 100644
--- a/gopls/internal/lsp/general.go
+++ b/gopls/internal/lsp/general.go
@@ -559,7 +559,10 @@
if err != nil {
return nil, nil, false, func() {}, err
}
- snapshot, release := view.Snapshot(ctx)
+ snapshot, release, err := view.Snapshot()
+ if err != nil {
+ return nil, nil, false, func() {}, err
+ }
fh, err := snapshot.GetFile(ctx, uri)
if err != nil {
release()
diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go
index 46ff070..6a6a8ca 100644
--- a/gopls/internal/lsp/lsp_test.go
+++ b/gopls/internal/lsp/lsp_test.go
@@ -1397,7 +1397,10 @@
}
r.diagnostics = make(map[span.URI][]*source.Diagnostic)
- snapshot, release := view.Snapshot(r.ctx)
+ snapshot, release, err := view.Snapshot()
+ if err != nil {
+ panic(err)
+ }
defer release()
// Always run diagnostics with analysis.
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 4b6fd13..640a32e 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -309,7 +309,10 @@
// Snapshot returns the current snapshot for the view, and a
// release function that must be called when the Snapshot is
// no longer needed.
- Snapshot(ctx context.Context) (Snapshot, func())
+ //
+ // If the view is shut down, the resulting error will be non-nil, and the
+ // release function need not be called.
+ Snapshot() (Snapshot, func(), error)
// IsGoPrivatePath reports whether target is a private import path, as identified
// by the GOPRIVATE environment variable.
diff --git a/gopls/internal/lsp/source/workspace_symbol.go b/gopls/internal/lsp/source/workspace_symbol.go
index dc5abd2..1b157c6 100644
--- a/gopls/internal/lsp/source/workspace_symbol.go
+++ b/gopls/internal/lsp/source/workspace_symbol.go
@@ -305,7 +305,10 @@
seen := make(map[span.URI]bool)
// TODO(adonovan): opt: parallelize this loop? How often is len > 1?
for _, v := range views {
- snapshot, release := v.Snapshot(ctx)
+ snapshot, release, err := v.Snapshot()
+ if err != nil {
+ continue // view is shut down; continue with others
+ }
defer release()
// Use the root view URIs for determining (lexically)
diff --git a/gopls/internal/lsp/text_synchronization.go b/gopls/internal/lsp/text_synchronization.go
index 77e081a..3028dc9 100644
--- a/gopls/internal/lsp/text_synchronization.go
+++ b/gopls/internal/lsp/text_synchronization.go
@@ -144,7 +144,10 @@
if err != nil {
return err
}
- snapshot, release := view.Snapshot(ctx)
+ snapshot, release, err := view.Snapshot()
+ if err != nil {
+ return err
+ }
isGenerated := source.IsGenerated(ctx, snapshot, uri)
release()
diff --git a/gopls/internal/lsp/workspace.go b/gopls/internal/lsp/workspace.go
index e4326b1..f2df9c1 100644
--- a/gopls/internal/lsp/workspace.go
+++ b/gopls/internal/lsp/workspace.go
@@ -63,7 +63,10 @@
return err
}
go func() {
- snapshot, release := view.Snapshot(ctx)
+ snapshot, release, err := view.Snapshot()
+ if err != nil {
+ return // view is shut down; no need to diagnose
+ }
defer release()
s.diagnoseDetached(snapshot)
}()
