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
