Skip to content

Commit

Permalink
gopls/internal/lsp/cache: remove baseCtx from the View
Browse files Browse the repository at this point in the history
Views don't do work, so should have no need of a context. Instead, chain
the contexts inside of clone.

Also, fix a latent bug where the context used in the view importsState
is the snapshot backgroundCtx rather than view baseCtx: the importsState
outlives the snapshot, so should not reference the snapshot context.
Fortunately, the context was thus far only used for logging.

For golang/go#57979

Change-Id: Icf55d69e82f19b3fd52ca7d9266df2b5589bb36e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/539676
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
  • Loading branch information
findleyr committed Nov 6, 2023
1 parent 8b89cfa commit 4124316
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 10 deletions.
3 changes: 1 addition & 2 deletions gopls/internal/lsp/cache/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,14 @@ func (s *Session) createView(ctx context.Context, info *workspaceInformation, fo
folder: folder,
initialWorkspaceLoad: make(chan struct{}),
initializationSema: make(chan struct{}, 1),
baseCtx: baseCtx,
moduleUpgrades: map[span.URI]map[string]string{},
vulns: map[span.URI]*vulncheck.Result{},
parseCache: s.parseCache,
fs: s.overlayFS,
workspaceInformation: info,
}
v.importsState = &importsState{
ctx: backgroundCtx,
ctx: baseCtx,
processEnv: &imports.ProcessEnv{
GocmdRunner: s.gocmdRunner,
SkipPathInScan: func(dir string) bool {
Expand Down
7 changes: 4 additions & 3 deletions gopls/internal/lsp/cache/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"golang.org/x/tools/internal/packagesinternal"
"golang.org/x/tools/internal/persistent"
"golang.org/x/tools/internal/typesinternal"
"golang.org/x/tools/internal/xcontext"
)

type snapshot struct {
Expand Down Expand Up @@ -1813,20 +1814,20 @@ func inVendor(uri span.URI) bool {
return found && strings.Contains(after, "/")
}

func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source.FileHandle) (*snapshot, func()) {
func (s *snapshot) clone(ctx context.Context, changes map[span.URI]source.FileHandle) (*snapshot, func()) {
ctx, done := event.Start(ctx, "cache.snapshot.clone")
defer done()

s.mu.Lock()
defer s.mu.Unlock()

bgCtx, cancel := context.WithCancel(bgCtx)
backgroundCtx, cancel := context.WithCancel(event.Detach(xcontext.Detach(s.backgroundCtx)))
result := &snapshot{
sequenceID: s.sequenceID + 1,
globalID: nextSnapshotID(),
store: s.store,
view: s.view,
backgroundCtx: bgCtx,
backgroundCtx: backgroundCtx,
cancel: cancel,
builtin: s.builtin,
initialized: s.initialized,
Expand Down
6 changes: 1 addition & 5 deletions gopls/internal/lsp/cache/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,6 @@ type View struct {

gocmdRunner *gocommand.Runner // limits go command concurrency

// baseCtx is the context handed to NewView. This is the parent of all
// background contexts created for this view.
baseCtx context.Context

folder *Folder

// Workspace information. The fields below are immutable, and together with
Expand Down Expand Up @@ -893,7 +889,7 @@ func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]sourc
prevSnapshot.AwaitInitialized(ctx)

// Save one lease of the cloned snapshot in the view.
v.snapshot, v.releaseSnapshot = prevSnapshot.clone(ctx, v.baseCtx, changes)
v.snapshot, v.releaseSnapshot = prevSnapshot.clone(ctx, changes)

prevReleaseSnapshot()
v.destroy(prevSnapshot, "View.invalidateContent")
Expand Down

0 comments on commit 4124316

Please sign in to comment.