diff --git a/gopls/internal/regtest/completion/postfix_snippet_test.go b/gopls/internal/regtest/completion/postfix_snippet_test.go index 7e595aaad1..5a7ffb80d2 100644 --- a/gopls/internal/regtest/completion/postfix_snippet_test.go +++ b/gopls/internal/regtest/completion/postfix_snippet_test.go @@ -13,8 +13,6 @@ import ( ) func TestPostfixSnippetCompletion(t *testing.T) { - t.Skipf("skipping test due to suspected synchronization bug; see https://go.dev/issue/50707") - const mod = ` -- go.mod -- module mod.com @@ -400,7 +398,7 @@ func _() { before: ` package foo -func foo() []string { +func foo() []string { x := "test" return x.split }`, @@ -409,7 +407,7 @@ package foo import "strings" -func foo() []string { +func foo() []string { x := "test" return strings.Split(x, "$0") }`, diff --git a/gopls/internal/regtest/inlayHints/inlayHints_test.go b/gopls/internal/regtest/inlayhints/inlayhints_test.go similarity index 98% rename from gopls/internal/regtest/inlayHints/inlayHints_test.go rename to gopls/internal/regtest/inlayhints/inlayhints_test.go index 67931fbdc8..a7cbe65731 100644 --- a/gopls/internal/regtest/inlayHints/inlayHints_test.go +++ b/gopls/internal/regtest/inlayhints/inlayhints_test.go @@ -1,7 +1,7 @@ // Copyright 2022 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -package inlayHint +package inlayhint import ( "testing" @@ -17,6 +17,7 @@ func TestMain(m *testing.M) { bug.PanicOnBugs = true Main(m, hooks.Options) } + func TestEnablingInlayHints(t *testing.T) { testenv.NeedsGo1Point(t, 14) // Test fails on 1.13. const workspace = ` diff --git a/gopls/internal/vulncheck/command_test.go b/gopls/internal/vulncheck/command_test.go index f6e2d1b761..e7bf7085f8 100644 --- a/gopls/internal/vulncheck/command_test.go +++ b/gopls/internal/vulncheck/command_test.go @@ -309,8 +309,13 @@ func runTest(t *testing.T, workspaceData, proxyData string, test func(context.Co if err != nil { t.Fatal(err) } - defer release() - defer view.Shutdown(ctx) + + defer func() { + // The snapshot must be released before calling view.Shutdown, to avoid a + // deadlock. + release() + view.Shutdown(ctx) + }() test(ctx, snapshot) } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 8c90c77ea8..9c1505850c 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -301,19 +301,6 @@ func (s *Session) viewOf(uri span.URI) (*View, error) { return s.viewMap[uri], nil } -func (s *Session) viewsOf(uri span.URI) []*View { - s.viewMu.RLock() - defer s.viewMu.RUnlock() - - var views []*View - for _, view := range s.views { - if source.InDir(view.folder.Filename(), uri.Filename()) { - views = append(views, view) - } - } - return views -} - func (s *Session) Views() []source.View { s.viewMu.RLock() defer s.viewMu.RUnlock() diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index c2438cdf20..a9dd1dfb93 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -187,11 +187,25 @@ type actionKey struct { // destroy waits for all leases on the snapshot to expire then releases // any resources (reference counts and files) associated with it. +// Snapshots being destroyed can be awaited using v.destroyWG. // // TODO(adonovan): move this logic into the release function returned // by Acquire when the reference count becomes zero. (This would cost // us the destroyedBy debug info, unless we add it to the signature of // memoize.RefCounted.Acquire.) +// +// The destroyedBy argument is used for debugging. +// +// v.snapshotMu must be held while calling this function, in order to preserve +// the invariants described by the the docstring for v.snapshot. +func (v *View) destroy(s *snapshot, destroyedBy string) { + v.snapshotWG.Add(1) + go func() { + defer v.snapshotWG.Done() + s.destroy(destroyedBy) + }() +} + func (s *snapshot) destroy(destroyedBy string) { // Wait for all leases to end before commencing destruction. s.refcount.Wait() @@ -1678,7 +1692,7 @@ func (s *snapshot) orphanedFiles() []source.VersionedFileHandle { } // If the URI doesn't belong to this view, then it's not in a workspace // package and should not be reloaded directly. - if !contains(s.view.session.viewsOf(uri), s.view) { + if !source.InDir(s.view.folder.Filename(), uri.Filename()) { return } // If the file is not open and is in a vendor directory, don't treat it diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 8ed9ecd0d1..0991797b8e 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -67,9 +67,18 @@ type View struct { // attempt at initialization. initCancelFirstAttempt 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 // nil after shutdown has been called - releaseSnapshot func() // called when snapshot is no longer needed + snapshot *snapshot // latest snapshot + releaseSnapshot func() // called when snapshot is no longer needed + snapshotWG sync.WaitGroup // refcount for pending destroy operations // initialWorkspaceLoad is closed when the first workspace initialization has // completed. If we failed to load, we only retry if the go.mod file changes, @@ -125,6 +134,11 @@ type environmentVariables struct { gocache, gopath, goroot, goprivate, gomodcache, go111module string } +// workspaceMode holds various flags defining how the gopls workspace should +// behave. They may be derived from the environment, user configuration, or +// depend on the Go version. +// +// TODO(rfindley): remove workspace mode, in favor of explicit checks. type workspaceMode int const ( @@ -521,6 +535,9 @@ 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) { @@ -530,13 +547,14 @@ func (v *View) shutdown(ctx context.Context) { v.snapshotMu.Lock() if v.snapshot != nil { v.releaseSnapshot() - go v.snapshot.destroy("View.shutdown") + v.destroy(v.snapshot, "View.shutdown") v.snapshot = nil v.releaseSnapshot = nil } v.snapshotMu.Unlock() v.importsState.destroy() + v.snapshotWG.Wait() } func (v *View) Session() *Session { @@ -732,7 +750,7 @@ func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]*file v.snapshot, v.releaseSnapshot = prevSnapshot.clone(ctx, v.baseCtx, changes, forceReloadMetadata) prevReleaseSnapshot() - go prevSnapshot.destroy("View.invalidateContent") + v.destroy(prevSnapshot, "View.invalidateContent") // Return a second lease to the caller. return v.snapshot, v.snapshot.Acquire() diff --git a/internal/lsp/general.go b/internal/lsp/general.go index fbb9692ea8..8ea4d7f5fa 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -487,6 +487,8 @@ func (s *Server) beginFileRequest(ctx context.Context, pURI protocol.DocumentURI return snapshot, fh, true, release, nil } +// shutdown implements the 'shutdown' LSP handler. It releases resources +// associated with the server and waits for all ongoing work to complete. func (s *Server) shutdown(ctx context.Context) error { s.stateMu.Lock() defer s.stateMu.Unlock() diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go index bebec53c52..5726f88ea0 100644 --- a/internal/lsp/regtest/runner.go +++ b/internal/lsp/regtest/runner.go @@ -66,9 +66,6 @@ type Runner struct { mu sync.Mutex ts *servertest.TCPServer socketDir string - // closers is a queue of clean-up functions to run at the end of the entire - // test suite. - closers []io.Closer } type runConfig struct { @@ -228,6 +225,8 @@ type TestFunc func(t *testing.T, env *Env) // modes. For each a test run, a new workspace is created containing the // un-txtared files specified by filedata. func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOption) { + // TODO(rfindley): this function has gotten overly complicated, and warrants + // refactoring. t.Helper() checkBuilder(t) @@ -259,6 +258,10 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio } t.Run(tc.name, func(t *testing.T) { + // TODO(rfindley): once jsonrpc2 shutdown is fixed, we should not leak + // goroutines in this test function. + // stacktest.NoLeak(t) + ctx := context.Background() if r.Timeout != 0 && !config.noDefaultTimeout { var cancel context.CancelFunc @@ -282,6 +285,7 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio if err := os.MkdirAll(rootDir, 0755); err != nil { t.Fatal(err) } + files := fake.UnpackTxt(files) if config.editor.WindowsLineEndings { for name, data := range files { @@ -294,13 +298,14 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio if err != nil { t.Fatal(err) } - // Deferring the closure of ws until the end of the entire test suite - // has, in testing, given the LSP server time to properly shutdown and - // release any file locks held in workspace, which is a problem on - // Windows. This may still be flaky however, and in the future we need a - // better solution to ensure that all Go processes started by gopls have - // exited before we clean up. - r.AddCloser(sandbox) + defer func() { + if !r.SkipCleanup { + if err := sandbox.Close(); err != nil { + pprof.Lookup("goroutine").WriteTo(os.Stderr, 1) + t.Errorf("closing the sandbox: %v", err) + } + } + }() ss := tc.getServer(t, config.optionsHook) framer := jsonrpc2.NewRawStream ls := &loggingFramer{} @@ -322,6 +327,7 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio closeCtx, cancel := context.WithTimeout(xcontext.Detach(ctx), 5*time.Second) defer cancel() if err := env.Editor.Close(closeCtx); err != nil { + pprof.Lookup("goroutine").WriteTo(os.Stderr, 1) t.Errorf("closing editor: %v", err) } }() @@ -493,14 +499,6 @@ func (r *Runner) getRemoteSocket(t *testing.T) string { return socket } -// AddCloser schedules a closer to be closed at the end of the test run. This -// is useful for Windows in particular, as -func (r *Runner) AddCloser(closer io.Closer) { - r.mu.Lock() - defer r.mu.Unlock() - r.closers = append(r.closers, closer) -} - // Close cleans up resource that have been allocated to this workspace. func (r *Runner) Close() error { r.mu.Lock() @@ -518,11 +516,6 @@ func (r *Runner) Close() error { } } if !r.SkipCleanup { - for _, closer := range r.closers { - if err := closer.Close(); err != nil { - errmsgs = append(errmsgs, err.Error()) - } - } if err := os.RemoveAll(r.TempDir); err != nil { errmsgs = append(errmsgs, err.Error()) }