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) {
