From 0b1f1d4bc227cc2e610854f23e14696becb9e46c Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 9 Jan 2024 18:45:09 -0500 Subject: [PATCH] gopls/internal/lsp/cache: (re-)ensure clean shutdown CL 549415 (rightly) changed the logic for View shutdown to not await all work on the Snapshot, as this leads to potential deadlocks: we should never await work while holding a mutex. However, we still need to await all work when shutting down the Session, otherwise we end up with failures like golang/go#64971 ("directory not empty"). Therefore, we need a new synchronization mechanism. Introduce a sync.WaitGroup on the Session to allow awaiting the destruction of all Snapshots created on behalf of the Session. In order to support this, View invalidation becomes a method on the Session, rather than the View, and requires the Session.viewMu. Also make a few unrelated cosmetic improvements. Fixes golang/go#64971 Change-Id: I43fc0b5ff8a7762887fbfd64df7596e524383279 Reviewed-on: https://go-review.googlesource.com/c/tools/+/554996 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/session.go | 12 +++++-- gopls/internal/lsp/cache/snapshot.go | 14 +++++--- gopls/internal/lsp/cache/view.go | 52 ++++++++++++++++------------ gopls/internal/server/command.go | 20 +++++------ 4 files changed, 58 insertions(+), 40 deletions(-) diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go index 7436bcb60f2..bf8cc54c0e4 100644 --- a/gopls/internal/lsp/cache/session.go +++ b/gopls/internal/lsp/cache/session.go @@ -42,6 +42,11 @@ type Session struct { views []*View viewMap map[protocol.DocumentURI]*View // file->best view; nil after shutdown + // snapshots is a counting semaphore that records the number + // of unreleased snapshots associated with this session. + // Shutdown waits for it to fall to zero. + snapshotWG sync.WaitGroup + parseCache *parseCache *overlayFS @@ -68,6 +73,7 @@ func (s *Session) Shutdown(ctx context.Context) { view.shutdown() } s.parseCache.stop() + s.snapshotWG.Wait() // wait for all work on associated snapshots to finish event.Log(ctx, "Shutdown session", KeyShutdownSession.Of(s)) } @@ -183,12 +189,14 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition) (*View, * }, } + s.snapshotWG.Add(1) v.snapshot = &Snapshot{ view: v, backgroundCtx: backgroundCtx, cancel: cancel, store: s.cache.store, refcount: 1, // Snapshots are born referenced. + done: s.snapshotWG.Done, packages: new(persistent.Map[PackageID, *packageHandle]), meta: new(metadata.Graph), files: newFileMap(), @@ -217,7 +225,7 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition) (*View, * // Initialize the view without blocking. initCtx, initCancel := context.WithCancel(xcontext.Detach(ctx)) - v.initCancelFirstAttempt = initCancel + v.cancelInitialWorkspaceLoad = initCancel snapshot := v.snapshot // Pass a second reference to the background goroutine. @@ -822,7 +830,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []file.Modificatio // ...but changes may be relevant to other views, for example if they are // changes to a shared package. for _, v := range s.views { - _, release, needsDiagnosis := v.Invalidate(ctx, StateChange{Files: changed}) + _, release, needsDiagnosis := s.invalidateViewLocked(ctx, v, StateChange{Files: changed}) release() if needsDiagnosis || checkViews { diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 1b1ed129a79..83e02d27c3d 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -85,12 +85,14 @@ type Snapshot struct { store *memoize.Store // cache of handles shared by all snapshots refMu sync.Mutex + // refcount holds the number of outstanding references to the current - // Snapshot. When refcount is decremented to 0, the Snapshot maps can be - // safely destroyed. + // Snapshot. When refcount is decremented to 0, the Snapshot maps are + // destroyed and the done function is called. // // TODO(rfindley): use atomic.Int32 on Go 1.19+. refcount int + done func() // for implementing Session.Shutdown // mu guards all of the maps in the snapshot, as well as the builtin URI and // initialized. @@ -248,6 +250,7 @@ func (s *Snapshot) decref() { s.unloadableFiles.Destroy() s.moduleUpgrades.Destroy() s.vulns.Destroy() + s.done() } } @@ -1663,10 +1666,10 @@ func inVendor(uri protocol.DocumentURI) bool { // also require more strictness about diagnostic dependencies. For example, // template.Diagnostics currently re-parses every time: there is no Snapshot // data responsible for providing these diagnostics. -func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange) (*Snapshot, bool) { +func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done func()) (*Snapshot, bool) { changedFiles := changed.Files - ctx, done := event.Start(ctx, "cache.snapshot.clone") - defer done() + ctx, stop := event.Start(ctx, "cache.snapshot.clone") + defer stop() s.mu.Lock() defer s.mu.Unlock() @@ -1680,6 +1683,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange) (*Snap sequenceID: s.sequenceID + 1, store: s.store, refcount: 1, // Snapshots are born referenced. + done: done, view: s.view, backgroundCtx: bgCtx, cancel: cancel, diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index 0b1e1dbf505..bf2a8f045eb 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -29,6 +29,7 @@ import ( "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/util/maps" "golang.org/x/tools/gopls/internal/util/pathutil" + "golang.org/x/tools/gopls/internal/util/slices" "golang.org/x/tools/gopls/internal/vulncheck" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" @@ -100,21 +101,12 @@ type View struct { // ignoreFilter is used for fast checking of ignored files. ignoreFilter *ignoreFilter - // initCancelFirstAttempt can be used to terminate the view's first + // cancelInitialWorkspaceLoad can be used to terminate the view's first // attempt at initialization. - initCancelFirstAttempt context.CancelFunc + cancelInitialWorkspaceLoad context.CancelFunc - // Track the latest snapshot via the snapshot field, guarded by snapshotMu. - // - // Invariant: whenever the snapshot field is overwritten, destroy(snapshot) - // is called on the previous (overwritten) snapshot while snapshotMu is held, - // incrementing snapshotWG. During shutdown the final snapshot is - // overwritten with nil and destroyed, guaranteeing that all observed - // snapshots have been destroyed via the destroy method, and snapshotWG may - // be waited upon to let these destroy operations complete. snapshotMu sync.Mutex - snapshot *Snapshot // latest snapshot; nil after shutdown has been called - snapshotWG sync.WaitGroup // refcount for pending destroy operations + snapshot *Snapshot // latest snapshot; nil after shutdown has been called // initialWorkspaceLoad is closed when the first workspace initialization has // completed. If we failed to load, we only retry if the go.mod file changes, @@ -513,11 +505,10 @@ func (v *View) filterFunc() func(protocol.DocumentURI) bool { } } -// shutdown releases resources associated with the view, and waits for ongoing -// work to complete. +// shutdown releases resources associated with the view. func (v *View) shutdown() { // Cancel the initial workspace load if it is still running. - v.initCancelFirstAttempt() + v.cancelInitialWorkspaceLoad() v.snapshotMu.Lock() if v.snapshot != nil { @@ -526,8 +517,6 @@ func (v *View) shutdown() { v.snapshot = nil } v.snapshotMu.Unlock() - - v.snapshotWG.Wait() } // IgnoredFile reports if a file would be ignored by a `go list` of the whole @@ -767,16 +756,33 @@ type StateChange struct { GCDetails map[metadata.PackageID]bool // package -> whether or not we want details } -// Invalidate processes the provided state change, invalidating any derived +// InvalidateView processes the provided state change, invalidating any derived // results that depend on the changed state. // // The resulting snapshot is non-nil, representing the outcome of the state // change. The second result is a function that must be called to release the // snapshot when the snapshot is no longer needed. // -// The resulting bool reports whether the new View needs to be re-diagnosed. -// See Snapshot.clone for more details. -func (v *View) Invalidate(ctx context.Context, changed StateChange) (*Snapshot, func(), bool) { +// An error is returned if the given view is no longer active in the session. +func (s *Session) InvalidateView(ctx context.Context, view *View, changed StateChange) (*Snapshot, func(), error) { + s.viewMu.Lock() + defer s.viewMu.Unlock() + + if !slices.Contains(s.views, view) { + return nil, nil, fmt.Errorf("view is no longer active") + } + snapshot, release, _ := s.invalidateViewLocked(ctx, view, changed) + return snapshot, release, nil +} + +// invalidateViewLocked invalidates the content of the given view. +// (See [Session.InvalidateView]). +// +// The resulting bool reports whether the View needs to be re-diagnosed. +// (See [Snapshot.clone]). +// +// s.viewMu must be held while calling this method. +func (s *Session) invalidateViewLocked(ctx context.Context, v *View, changed StateChange) (*Snapshot, func(), bool) { // Detach the context so that content invalidation cannot be canceled. ctx = xcontext.Detach(ctx) @@ -799,9 +805,9 @@ func (v *View) Invalidate(ctx context.Context, changed StateChange) (*Snapshot, // TODO(rfindley): shouldn't we do this before canceling? prevSnapshot.AwaitInitialized(ctx) - // Save one lease of the cloned snapshot in the view. var needsDiagnosis bool - v.snapshot, needsDiagnosis = prevSnapshot.clone(ctx, v.baseCtx, changed) + s.snapshotWG.Add(1) + v.snapshot, needsDiagnosis = prevSnapshot.clone(ctx, v.baseCtx, changed, s.snapshotWG.Done) // Remove the initial reference created when prevSnapshot was created. prevSnapshot.decref() diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go index 330f0a8ba3e..60c71840f4f 100644 --- a/gopls/internal/server/command.go +++ b/gopls/internal/server/command.go @@ -285,10 +285,9 @@ func (c *commandHandler) CheckUpgrades(ctx context.Context, args command.CheckUp if err != nil { return nil, nil, err } - snapshot, release, _ := deps.snapshot.View().Invalidate(ctx, cache.StateChange{ + return c.s.session.InvalidateView(ctx, deps.snapshot.View(), cache.StateChange{ ModuleUpgrades: map[protocol.DocumentURI]map[string]string{args.URI: upgrades}, }) - return snapshot, release, nil }) }) } @@ -306,7 +305,7 @@ func (c *commandHandler) ResetGoModDiagnostics(ctx context.Context, args command forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { return c.modifyState(ctx, FromResetGoModDiagnostics, func() (*cache.Snapshot, func(), error) { - snapshot, release, _ := deps.snapshot.View().Invalidate(ctx, cache.StateChange{ + return c.s.session.InvalidateView(ctx, deps.snapshot.View(), cache.StateChange{ ModuleUpgrades: map[protocol.DocumentURI]map[string]string{ deps.fh.URI(): nil, }, @@ -314,7 +313,6 @@ func (c *commandHandler) ResetGoModDiagnostics(ctx context.Context, args command deps.fh.URI(): nil, }, }) - return snapshot, release, nil }) }) } @@ -443,7 +441,7 @@ func (c *commandHandler) RemoveDependency(ctx context.Context, args command.Remo if err != nil { return err } - edits, err := dropDependency(deps.snapshot, pm, args.ModulePath) + edits, err := dropDependency(pm, args.ModulePath) if err != nil { return err } @@ -476,7 +474,7 @@ func (c *commandHandler) RemoveDependency(ctx context.Context, args command.Remo // dropDependency returns the edits to remove the given require from the go.mod // file. -func dropDependency(snapshot *cache.Snapshot, pm *cache.ParsedModule, modulePath string) ([]protocol.TextEdit, error) { +func dropDependency(pm *cache.ParsedModule, modulePath string) ([]protocol.TextEdit, error) { // We need a private copy of the parsed go.mod file, since we're going to // modify it. copied, err := modfile.Parse("", pm.Mapper.Content, nil) @@ -796,12 +794,11 @@ func (c *commandHandler) ToggleGCDetails(ctx context.Context, args command.URIAr return nil, nil, err } wantDetails := !deps.snapshot.WantGCDetails(meta.ID) // toggle the gc details state - snapshot, release, _ := deps.snapshot.View().Invalidate(ctx, cache.StateChange{ + return c.s.session.InvalidateView(ctx, deps.snapshot.View(), cache.StateChange{ GCDetails: map[metadata.PackageID]bool{ meta.ID: wantDetails, }, }) - return snapshot, release, nil }) }) } @@ -995,9 +992,12 @@ func (c *commandHandler) RunGovulncheck(ctx context.Context, args command.Vulnch return err } - snapshot, release, _ := deps.snapshot.View().Invalidate(ctx, cache.StateChange{ + snapshot, release, err := c.s.session.InvalidateView(ctx, deps.snapshot.View(), cache.StateChange{ Vulns: map[protocol.DocumentURI]*vulncheck.Result{args.URI: result}, }) + if err != nil { + return err + } defer release() c.s.diagnoseSnapshot(snapshot, nil, 0) @@ -1292,7 +1292,7 @@ func (c *commandHandler) ChangeSignature(ctx context.Context, args command.Chang func (c *commandHandler) DiagnoseFiles(ctx context.Context, args command.DiagnoseFilesArgs) error { return c.run(ctx, commandConfig{ progress: "Diagnose files", - }, func(ctx context.Context, deps commandDeps) error { + }, func(ctx context.Context, _ commandDeps) error { // TODO(rfindley): even better would be textDocument/diagnostics (golang/go#60122). // Though note that implementing pull diagnostics may cause some servers to