gopls/internal/lsp: remove access to mutable session state from the view
To avoid concurrency bugs and/or operation on inconsistent session
state, remove visibility into the session from views. In a few places,
this refactoring highlighted bugs (such as incorrect usage of
view.SetOptions).
This broke one of the debug templates, for which it was simply easier to
remove the back-link from view to session.
Change-Id: I4dbce0dcebab6f25a9c70524310ae1e0e04e2d97
Reviewed-on: https://go-review.googlesource.com/c/tools/+/453815
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/cache.go b/gopls/internal/lsp/cache/cache.go
index 0eb00f2..8c06b6d 100644
--- a/gopls/internal/lsp/cache/cache.go
+++ b/gopls/internal/lsp/cache/cache.go
@@ -173,11 +173,11 @@
c.options(options)
}
s := &Session{
- cache: c,
id: strconv.FormatInt(index, 10),
+ cache: c,
+ gocmdRunner: &gocommand.Runner{},
options: options,
overlays: make(map[span.URI]*overlay),
- gocmdRunner: &gocommand.Runner{},
}
event.Log(ctx, "New session", KeyCreateSession.Of(s))
return s
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index 321ff5f..ccba4e8 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -105,11 +105,6 @@
}
sort.Strings(query) // for determinism
- if s.view.Options().VerboseWorkDoneProgress {
- work := s.view.session.progress.Start(ctx, "Load", fmt.Sprintf("Loading query=%s", query), nil, nil)
- defer work.End(ctx, "Done.")
- }
-
ctx, done := event.Start(ctx, "cache.view.load", tag.Query.Of(query))
defer done()
diff --git a/gopls/internal/lsp/cache/mod.go b/gopls/internal/lsp/cache/mod.go
index 97bdb2f..757bb5e 100644
--- a/gopls/internal/lsp/cache/mod.go
+++ b/gopls/internal/lsp/cache/mod.go
@@ -186,7 +186,7 @@
var sumFH source.FileHandle = s.FindFile(sumURI)
if sumFH == nil {
var err error
- sumFH, err = s.view.session.cache.getFile(ctx, sumURI)
+ sumFH, err = s.view.cache.getFile(ctx, sumURI)
if err != nil {
return nil
}
diff --git a/gopls/internal/lsp/cache/mod_tidy.go b/gopls/internal/lsp/cache/mod_tidy.go
index fc97ee6..f433211 100644
--- a/gopls/internal/lsp/cache/mod_tidy.go
+++ b/gopls/internal/lsp/cache/mod_tidy.go
@@ -106,7 +106,7 @@
// Keep the temporary go.mod file around long enough to parse it.
defer cleanup()
- if _, err := snapshot.view.session.gocmdRunner.Run(ctx, *inv); err != nil {
+ if _, err := snapshot.view.gocmdRunner.Run(ctx, *inv); err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go
index 28acd5d..5eca7c8 100644
--- a/gopls/internal/lsp/cache/session.go
+++ b/gopls/internal/lsp/cache/session.go
@@ -14,9 +14,9 @@
"sync/atomic"
"golang.org/x/tools/gopls/internal/govulncheck"
- "golang.org/x/tools/gopls/internal/lsp/progress"
"golang.org/x/tools/gopls/internal/lsp/source"
"golang.org/x/tools/gopls/internal/span"
+ "golang.org/x/tools/internal/bug"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/imports"
@@ -25,8 +25,12 @@
)
type Session struct {
- cache *Cache
- id string
+ // Unique identifier for this session.
+ id string
+
+ // Immutable attributes shared across views.
+ cache *Cache // shared cache
+ gocmdRunner *gocommand.Runner // limits go command concurrency
optionsMu sync.Mutex
options *source.Options
@@ -37,11 +41,6 @@
overlayMu sync.Mutex
overlays map[span.URI]*overlay
-
- // gocmdRunner guards go command calls from concurrency errors.
- gocmdRunner *gocommand.Runner
-
- progress *progress.Tracker
}
type overlay struct {
@@ -139,12 +138,6 @@
s.options = options
}
-// SetProgressTracker sets the progress tracker for the session.
-func (s *Session) SetProgressTracker(tracker *progress.Tracker) {
- // The progress tracker should be set before any view is initialized.
- s.progress = tracker
-}
-
// Shutdown the session and all views it has created.
func (s *Session) Shutdown(ctx context.Context) {
var views []*View
@@ -154,7 +147,7 @@
s.viewMap = nil
s.viewMu.Unlock()
for _, view := range views {
- view.shutdown(ctx)
+ view.shutdown()
}
event.Log(ctx, "Shutdown session", KeyShutdownSession.Of(s))
}
@@ -169,7 +162,7 @@
// of its gopls workspace module in that directory, so that client tooling
// can execute in the same main module. On success it also returns a release
// function that must be called when the Snapshot is no longer needed.
-func (s *Session) NewView(ctx context.Context, name string, folder span.URI, options *source.Options) (source.View, source.Snapshot, func(), error) {
+func (s *Session) NewView(ctx context.Context, name string, folder span.URI, options *source.Options) (*View, source.Snapshot, func(), error) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
for _, view := range s.views {
@@ -230,10 +223,11 @@
backgroundCtx, cancel := context.WithCancel(baseCtx)
v := &View{
- session: s,
+ id: strconv.FormatInt(index, 10),
+ cache: s.cache,
+ gocmdRunner: s.gocmdRunner,
initialWorkspaceLoad: make(chan struct{}),
initializationSema: make(chan struct{}, 1),
- id: strconv.FormatInt(index, 10),
options: options,
baseCtx: baseCtx,
name: name,
@@ -308,7 +302,7 @@
}
// View returns a view with a matching name, if the session has one.
-func (s *Session) View(name string) source.View {
+func (s *Session) View(name string) *View {
s.viewMu.RLock()
defer s.viewMu.RUnlock()
for _, view := range s.views {
@@ -321,11 +315,7 @@
// ViewOf returns a view corresponding to the given URI.
// If the file is not already associated with a view, pick one using some heuristics.
-func (s *Session) ViewOf(uri span.URI) (source.View, error) {
- return s.viewOf(uri)
-}
-
-func (s *Session) viewOf(uri span.URI) (*View, error) {
+func (s *Session) ViewOf(uri span.URI) (*View, error) {
s.viewMu.RLock()
defer s.viewMu.RUnlock()
// Check if we already know this file.
@@ -340,13 +330,11 @@
return s.viewMap[uri], nil
}
-func (s *Session) Views() []source.View {
+func (s *Session) Views() []*View {
s.viewMu.RLock()
defer s.viewMu.RUnlock()
- result := make([]source.View, len(s.views))
- for i, v := range s.views {
- result[i] = v
- }
+ result := make([]*View, len(s.views))
+ copy(result, s.views)
return result
}
@@ -378,19 +366,19 @@
return views[0]
}
-func (s *Session) removeView(ctx context.Context, view *View) error {
+// RemoveView removes the view v from the session
+func (s *Session) RemoveView(view *View) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
- i, err := s.dropView(ctx, view)
- if err != nil {
- return err
+ i := s.dropView(view)
+ if i == -1 { // error reported elsewhere
+ return
}
// delete this view... we don't care about order but we do want to make
// sure we can garbage collect the view
s.views[i] = s.views[len(s.views)-1]
s.views[len(s.views)-1] = nil
s.views = s.views[:len(s.views)-1]
- return nil
}
func (s *Session) updateView(ctx context.Context, view *View, options *source.Options) (*View, error) {
@@ -406,9 +394,9 @@
seqID := view.snapshot.sequenceID // Preserve sequence IDs when updating a view in place.
view.snapshotMu.Unlock()
- i, err := s.dropView(ctx, view)
- if err != nil {
- return nil, err
+ i := s.dropView(view)
+ if i == -1 {
+ return nil, fmt.Errorf("view %q not found", view.id)
}
v, _, release, err := s.createView(ctx, view.name, view.folder, options, seqID)
@@ -428,18 +416,24 @@
return v, nil
}
-func (s *Session) dropView(ctx context.Context, v *View) (int, error) {
+// dropView removes v from the set of views for the receiver s and calls
+// v.shutdown, returning the index of v in s.views (if found), or -1 if v was
+// not found. s.viewMu must be held while calling this function.
+func (s *Session) dropView(v *View) int {
// we always need to drop the view map
s.viewMap = make(map[span.URI]*View)
for i := range s.views {
if v == s.views[i] {
// we found the view, drop it and return the index it was found at
s.views[i] = nil
- v.shutdown(ctx)
- return i, nil
+ v.shutdown()
+ return i
}
}
- return -1, fmt.Errorf("view %s for %v not found", v.Name(), v.Folder())
+ // TODO(rfindley): it looks wrong that we don't shutdown v in this codepath.
+ // We should never get here.
+ bug.Reportf("tried to drop nonexistent view %q", v.id)
+ return -1
}
func (s *Session) ModifyFiles(ctx context.Context, changes []source.FileModification) error {
@@ -498,7 +492,7 @@
if c.OnDisk {
continue
}
- bestView, err := s.viewOf(c.URI)
+ bestView, err := s.ViewOf(c.URI)
if err != nil {
return nil, nil, err
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 1f738c6..5a15641 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -258,7 +258,7 @@
}
func (s *snapshot) FileSet() *token.FileSet {
- return s.view.session.cache.fset
+ return s.view.cache.fset
}
func (s *snapshot) ModFiles() []span.URI {
@@ -376,7 +376,7 @@
if typesinternal.SetUsesCgo(&types.Config{}) {
cfg.Mode |= packages.LoadMode(packagesinternal.TypecheckCgo)
}
- packagesinternal.SetGoCmdRunner(cfg, s.view.session.gocmdRunner)
+ packagesinternal.SetGoCmdRunner(cfg, s.view.gocmdRunner)
return cfg
}
@@ -387,7 +387,7 @@
}
defer cleanup()
- return s.view.session.gocmdRunner.Run(ctx, *inv)
+ return s.view.gocmdRunner.Run(ctx, *inv)
}
func (s *snapshot) RunGoCommandPiped(ctx context.Context, mode source.InvocationFlags, inv *gocommand.Invocation, stdout, stderr io.Writer) error {
@@ -396,7 +396,7 @@
return err
}
defer cleanup()
- return s.view.session.gocmdRunner.RunPiped(ctx, *inv, stdout, stderr)
+ return s.view.gocmdRunner.RunPiped(ctx, *inv, stdout, stderr)
}
func (s *snapshot) RunGoCommands(ctx context.Context, allowNetwork bool, wd string, run func(invoke func(...string) (*bytes.Buffer, error)) error) (bool, []byte, []byte, error) {
@@ -417,7 +417,7 @@
invoke := func(args ...string) (*bytes.Buffer, error) {
inv.Verb = args[0]
inv.Args = args[1:]
- return s.view.session.gocmdRunner.Run(ctx, *inv)
+ return s.view.gocmdRunner.Run(ctx, *inv)
}
if err := run(invoke); err != nil {
return false, nil, nil, err
@@ -1319,7 +1319,7 @@
return fh, nil
}
- fh, err := s.view.session.cache.getFile(ctx, f.URI()) // read the file
+ fh, err := s.view.cache.getFile(ctx, f.URI()) // read the file
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index db7cbb7..4adcfb1 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -36,8 +36,10 @@
)
type View struct {
- session *Session
- id string
+ id string
+
+ cache *Cache // shared cache
+ gocmdRunner *gocommand.Runner // limits go command concurrency
optionsMu sync.Mutex
options *source.Options
@@ -343,7 +345,11 @@
return reflect.DeepEqual(aBuildFlags, bBuildFlags)
}
-func (v *View) SetOptions(ctx context.Context, options *source.Options) (source.View, error) {
+// SetViewOptions sets the options of the given view to new values. Calling
+// this may cause the view to be invalidated and a replacement view added to
+// the session. If so the new view will be returned, otherwise the original one
+// will be returned.
+func (s *Session) SetViewOptions(ctx context.Context, v *View, options *source.Options) (*View, error) {
// no need to rebuild the view if the options were not materially changed
v.optionsMu.Lock()
if minorOptionsChange(v.options, options) {
@@ -352,7 +358,7 @@
return v, nil
}
v.optionsMu.Unlock()
- newView, err := v.session.updateView(ctx, v, options)
+ newView, err := s.updateView(ctx, v, options)
return newView, err
}
@@ -375,7 +381,7 @@
fullEnv[s[0]] = s[1]
}
}
- goVersion, err := s.view.session.gocmdRunner.Run(ctx, gocommand.Invocation{
+ goVersion, err := s.view.gocmdRunner.Run(ctx, gocommand.Invocation{
Verb: "version",
Env: env,
WorkingDir: s.view.rootURI.Filename(),
@@ -584,16 +590,9 @@
return nil, nil
}
-func (v *View) Shutdown(ctx context.Context) {
- v.session.removeView(ctx, v)
-}
-
// shutdown releases resources associated with the view, and waits for ongoing
// work to complete.
-//
-// TODO(rFindley): probably some of this should also be one in View.Shutdown
-// above?
-func (v *View) shutdown(ctx context.Context) {
+func (v *View) shutdown() {
// Cancel the initial workspace load if it is still running.
v.initCancelFirstAttempt()
@@ -610,10 +609,6 @@
v.snapshotWG.Wait()
}
-func (v *View) Session() *Session {
- return v.session
-}
-
func (s *snapshot) IgnoredFile(uri span.URI) bool {
filename := uri.Filename()
var prefixes []string
diff --git a/gopls/internal/lsp/completion_test.go b/gopls/internal/lsp/completion_test.go
index 23d69ed..2257846 100644
--- a/gopls/internal/lsp/completion_test.go
+++ b/gopls/internal/lsp/completion_test.go
@@ -129,12 +129,12 @@
original := view.Options()
modified := view.Options().Clone()
options(modified)
- view, err = view.SetOptions(r.ctx, modified)
+ view, err = r.server.session.SetViewOptions(r.ctx, view, modified)
if err != nil {
t.Error(err)
return nil
}
- defer view.SetOptions(r.ctx, original)
+ defer r.server.session.SetViewOptions(r.ctx, view, original)
list, err := r.server.Completion(r.ctx, &protocol.CompletionParams{
TextDocumentPositionParams: protocol.TextDocumentPositionParams{
diff --git a/gopls/internal/lsp/debug/serve.go b/gopls/internal/lsp/debug/serve.go
index 248041e..abc939e 100644
--- a/gopls/internal/lsp/debug/serve.go
+++ b/gopls/internal/lsp/debug/serve.go
@@ -130,11 +130,7 @@
func (st *State) Views() []*cache.View {
var views []*cache.View
for _, s := range st.Sessions() {
- for _, v := range s.Views() {
- if cv, ok := v.(*cache.View); ok {
- views = append(views, cv)
- }
- }
+ views = append(views, s.Views()...)
}
return views
}
@@ -897,7 +893,6 @@
{{define "body"}}
Name: <b>{{.Name}}</b><br>
Folder: <b>{{.Folder}}</b><br>
-From: <b>{{template "sessionlink" .Session.ID}}</b><br>
<h2>Environment</h2>
<ul>{{range .Options.Env}}<li>{{.}}</li>{{end}}</ul>
{{end}}
diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go
index d966d94..5a29cd8 100644
--- a/gopls/internal/lsp/lsp_test.go
+++ b/gopls/internal/lsp/lsp_test.go
@@ -70,12 +70,12 @@
t.Fatal(err)
}
- defer view.Shutdown(ctx)
+ defer session.RemoveView(view)
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
tests.EnableAllAnalyzers(view, options)
- view.SetOptions(ctx, options)
+ session.SetViewOptions(ctx, view, options)
// Enable all inlay hints for tests.
tests.EnableAllInlayHints(view, options)
@@ -233,10 +233,11 @@
}
original := view.Options()
modified := original
+ defer r.server.session.SetViewOptions(r.ctx, view, original)
// Test all folding ranges.
modified.LineFoldingOnly = false
- view, err = view.SetOptions(r.ctx, modified)
+ view, err = r.server.session.SetViewOptions(r.ctx, view, modified)
if err != nil {
t.Error(err)
return
@@ -254,7 +255,7 @@
// Test folding ranges with lineFoldingOnly = true.
modified.LineFoldingOnly = true
- view, err = view.SetOptions(r.ctx, modified)
+ view, err = r.server.session.SetViewOptions(r.ctx, view, modified)
if err != nil {
t.Error(err)
return
@@ -269,7 +270,6 @@
return
}
r.foldingRanges(t, "foldingRange-lineFolding", uri, ranges)
- view.SetOptions(r.ctx, original)
}
func (r *runner) foldingRanges(t *testing.T, prefix string, uri span.URI, ranges []protocol.FoldingRange) {
@@ -1328,7 +1328,7 @@
}
}
-func (r *runner) collectDiagnostics(view source.View) {
+func (r *runner) collectDiagnostics(view *cache.View) {
if r.diagnostics != nil {
return
}
diff --git a/gopls/internal/lsp/server.go b/gopls/internal/lsp/server.go
index 9f70591..6d1775f 100644
--- a/gopls/internal/lsp/server.go
+++ b/gopls/internal/lsp/server.go
@@ -23,8 +23,6 @@
// NewServer creates an LSP server and binds it to handle incoming client
// messages on on the supplied stream.
func NewServer(session *cache.Session, client protocol.ClientCloser) *Server {
- tracker := progress.NewTracker(client)
- session.SetProgressTracker(tracker)
return &Server{
diagnostics: map[span.URI]*fileReports{},
gcOptimizationDetails: make(map[source.PackageID]struct{}),
@@ -33,7 +31,7 @@
session: session,
client: client,
diagnosticsSema: make(chan struct{}, concurrentAnalyses),
- progress: tracker,
+ progress: progress.NewTracker(client),
diagDebouncer: newDebouncer(),
watchedFileDebouncer: newDebouncer(),
}
diff --git a/gopls/internal/lsp/source/source_test.go b/gopls/internal/lsp/source/source_test.go
index d7dc77c..e540530 100644
--- a/gopls/internal/lsp/source/source_test.go
+++ b/gopls/internal/lsp/source/source_test.go
@@ -38,8 +38,9 @@
}
type runner struct {
+ session *cache.Session
+ view *cache.View
snapshot source.Snapshot
- view source.View
data *tests.Data
ctx context.Context
normalizers []tests.Normalizer
@@ -58,12 +59,15 @@
t.Fatal(err)
}
release()
- defer view.Shutdown(ctx)
+ defer session.RemoveView(view)
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
tests.EnableAllAnalyzers(view, options)
- view.SetOptions(ctx, options)
+ view, err = session.SetViewOptions(ctx, view, options)
+ if err != nil {
+ t.Fatal(err)
+ }
var modifications []source.FileModification
for filename, content := range datum.Config.Overlay {
@@ -84,6 +88,7 @@
snapshot, release := view.Snapshot(ctx)
defer release()
r := &runner{
+ session: session,
view: view,
snapshot: snapshot,
data: datum,
@@ -283,14 +288,14 @@
original := r.view.Options()
modified := original.Clone()
options(modified)
- newView, err := r.view.SetOptions(r.ctx, modified)
- if newView != r.view {
+ view, err := r.session.SetViewOptions(r.ctx, r.view, modified)
+ if view != r.view {
t.Fatalf("options change unexpectedly created new view")
}
if err != nil {
t.Fatal(err)
}
- defer r.view.SetOptions(r.ctx, original)
+ defer r.session.SetViewOptions(r.ctx, view, original)
list, surrounding, err := completion.Completion(r.ctx, r.snapshot, fh, protocol.Position{
Line: uint32(src.Start().Line() - 1),
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 0e9dcd2..fa55cde 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -283,18 +283,9 @@
// Folder returns the folder with which this view was created.
Folder() span.URI
- // Shutdown closes this view, and detaches it from its session.
- Shutdown(ctx context.Context)
-
// Options returns a copy of the Options for this view.
Options() *Options
- // SetOptions sets the options of this view to new values.
- // Calling this may cause the view to be invalidated and a replacement view
- // added to the session. If so the new view will be returned, otherwise the
- // original one will be.
- SetOptions(context.Context, *Options) (View, error)
-
// Snapshot returns the current snapshot for the view, and a
// release function that must be called when the Snapshot is
// no longer needed.
diff --git a/gopls/internal/lsp/workspace.go b/gopls/internal/lsp/workspace.go
index 4f9948e..e4326b1 100644
--- a/gopls/internal/lsp/workspace.go
+++ b/gopls/internal/lsp/workspace.go
@@ -18,7 +18,7 @@
for _, folder := range event.Removed {
view := s.session.View(folder.Name)
if view != nil {
- view.Shutdown(ctx)
+ s.session.RemoveView(view)
} else {
return fmt.Errorf("view %s for %v not found", folder.Name, folder.URI)
}
@@ -58,7 +58,7 @@
if err := s.fetchConfig(ctx, view.Name(), view.Folder(), options); err != nil {
return err
}
- view, err := view.SetOptions(ctx, options)
+ view, err := s.session.SetViewOptions(ctx, view, options)
if err != nil {
return err
}
diff --git a/gopls/internal/lsp/workspace_symbol.go b/gopls/internal/lsp/workspace_symbol.go
index 9101a3e..88b3e88 100644
--- a/gopls/internal/lsp/workspace_symbol.go
+++ b/gopls/internal/lsp/workspace_symbol.go
@@ -7,9 +7,9 @@
import (
"context"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
)
func (s *Server) symbol(ctx context.Context, params *protocol.WorkspaceSymbolParams) ([]protocol.SymbolInformation, error) {
@@ -19,5 +19,14 @@
views := s.session.Views()
matcher := s.session.Options().SymbolMatcher
style := s.session.Options().SymbolStyle
- return source.WorkspaceSymbols(ctx, matcher, style, views, params.Query)
+ // TODO(rfindley): it looks wrong that we need to pass views here.
+ //
+ // Evidence:
+ // - this is the only place we convert views to []source.View
+ // - workspace symbols is the only place where we call source.View.Snapshot
+ var sourceViews []source.View
+ for _, v := range views {
+ sourceViews = append(sourceViews, v)
+ }
+ return source.WorkspaceSymbols(ctx, matcher, style, sourceViews, params.Query)
}
diff --git a/gopls/internal/vulncheck/command_test.go b/gopls/internal/vulncheck/command_test.go
index b7a270e..4032693 100644
--- a/gopls/internal/vulncheck/command_test.go
+++ b/gopls/internal/vulncheck/command_test.go
@@ -319,7 +319,7 @@
// The snapshot must be released before calling view.Shutdown, to avoid a
// deadlock.
release()
- view.Shutdown(ctx)
+ session.RemoveView(view)
}()
test(ctx, snapshot)
