From 697795d6a801a72ad67b8c4fab6fdd74bc9150d1 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Sun, 8 Aug 2021 20:12:22 -0400 Subject: [PATCH] internal/lsp/regtest: don't run the connection on the test context When test awaiting fails, we often fail to shut down the server because the pipe is closed. Fix this by using a detached context for running the connection. Also clean up some unnecessary context arguments. Change-Id: I535c1cc1606e44df5f8e2177c92293d57836f992 Reviewed-on: https://go-review.googlesource.com/c/tools/+/340850 TryBot-Result: Gopher Robot Run-TryBot: Robert Findley gopls-CI: kokoro Reviewed-by: Alan Donovan --- gopls/internal/regtest/misc/shared_test.go | 3 ++- internal/jsonrpc2/servertest/servertest.go | 2 +- internal/jsonrpc2/servertest/servertest_test.go | 2 +- internal/lsp/lsprpc/lsprpc_test.go | 7 +++---- internal/lsp/regtest/env.go | 14 ++++++++++---- internal/lsp/regtest/runner.go | 17 +++++++++-------- 6 files changed, 26 insertions(+), 19 deletions(-) diff --git a/gopls/internal/regtest/misc/shared_test.go b/gopls/internal/regtest/misc/shared_test.go index 6861743ff4..a6b0cd87ef 100644 --- a/gopls/internal/regtest/misc/shared_test.go +++ b/gopls/internal/regtest/misc/shared_test.go @@ -30,7 +30,8 @@ func runShared(t *testing.T, testFunc func(env1 *Env, env2 *Env)) { WithOptions(Modes(modes)).Run(t, sharedProgram, func(t *testing.T, env1 *Env) { // Create a second test session connected to the same workspace and server // as the first. - env2 := NewEnv(env1.Ctx, t, env1.Sandbox, env1.Server, env1.Editor.Config, true) + env2, cleanup := NewEnv(env1.Ctx, t, env1.Sandbox, env1.Server, env1.Editor.Config, true) + defer cleanup() env2.Await(InitialWorkspaceLoad) testFunc(env1, env2) }) diff --git a/internal/jsonrpc2/servertest/servertest.go b/internal/jsonrpc2/servertest/servertest.go index 392e084a9a..b879ebdf18 100644 --- a/internal/jsonrpc2/servertest/servertest.go +++ b/internal/jsonrpc2/servertest/servertest.go @@ -68,7 +68,7 @@ type PipeServer struct { } // NewPipeServer returns a test server that can be connected to via io.Pipes. -func NewPipeServer(ctx context.Context, server jsonrpc2.StreamServer, framer jsonrpc2.Framer) *PipeServer { +func NewPipeServer(server jsonrpc2.StreamServer, framer jsonrpc2.Framer) *PipeServer { if framer == nil { framer = jsonrpc2.NewRawStream } diff --git a/internal/jsonrpc2/servertest/servertest_test.go b/internal/jsonrpc2/servertest/servertest_test.go index 38fa21a24d..1780d4f914 100644 --- a/internal/jsonrpc2/servertest/servertest_test.go +++ b/internal/jsonrpc2/servertest/servertest_test.go @@ -26,7 +26,7 @@ func TestTestServer(t *testing.T) { server := jsonrpc2.HandlerServer(fakeHandler) tcpTS := NewTCPServer(ctx, server, nil) defer tcpTS.Close() - pipeTS := NewPipeServer(ctx, server, nil) + pipeTS := NewPipeServer(server, nil) defer pipeTS.Close() tests := []struct { diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go index 795c887e4b..cde641c920 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/internal/lsp/lsprpc/lsprpc_test.go @@ -60,7 +60,7 @@ func TestClientLogging(t *testing.T) { ctx = debug.WithInstance(ctx, "", "") ss := NewStreamServer(cache.New(nil), false) ss.serverForTest = server - ts := servertest.NewPipeServer(ctx, ss, nil) + ts := servertest.NewPipeServer(ss, nil) defer checkClose(t, ts.Close) cc := ts.Connect(ctx) cc.Go(ctx, protocol.ClientHandler(client, jsonrpc2.MethodNotFound)) @@ -125,12 +125,11 @@ func setupForwarding(ctx context.Context, t *testing.T, s protocol.Server) (dire ss.serverForTest = s tsDirect := servertest.NewTCPServer(serveCtx, ss, nil) - forwarderCtx := debug.WithInstance(ctx, "", "") forwarder, err := NewForwarder("tcp;"+tsDirect.Addr, nil) if err != nil { t.Fatal(err) } - tsForwarded := servertest.NewPipeServer(forwarderCtx, forwarder, nil) + tsForwarded := servertest.NewPipeServer(forwarder, nil) return tsDirect, tsForwarded, func() { checkClose(t, tsDirect.Close) checkClose(t, tsForwarded.Close) @@ -225,7 +224,7 @@ func TestDebugInfoLifecycle(t *testing.T) { if err != nil { t.Fatal(err) } - tsForwarder := servertest.NewPipeServer(clientCtx, forwarder, nil) + tsForwarder := servertest.NewPipeServer(forwarder, nil) conn1 := tsForwarder.Connect(clientCtx) ed1, err := fake.NewEditor(sb, fake.EditorConfig{}).Connect(clientCtx, conn1, fake.ClientHooks{}) diff --git a/internal/lsp/regtest/env.go b/internal/lsp/regtest/env.go index f095c38f28..a37cbf6661 100644 --- a/internal/lsp/regtest/env.go +++ b/internal/lsp/regtest/env.go @@ -14,6 +14,7 @@ import ( "golang.org/x/tools/internal/jsonrpc2/servertest" "golang.org/x/tools/internal/lsp/fake" "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/xcontext" ) // Env holds an initialized fake Editor, Workspace, and Server, which may be @@ -109,9 +110,14 @@ type condition struct { // NewEnv creates a new test environment using the given scratch environment // and gopls server. -func NewEnv(ctx context.Context, tb testing.TB, sandbox *fake.Sandbox, ts servertest.Connector, editorConfig fake.EditorConfig, withHooks bool) *Env { +// +// The resulting func must be called to close the jsonrpc2 connection. +func NewEnv(ctx context.Context, tb testing.TB, sandbox *fake.Sandbox, ts servertest.Connector, editorConfig fake.EditorConfig, withHooks bool) (_ *Env, cleanup func()) { tb.Helper() - conn := ts.Connect(ctx) + + bgCtx, cleanupConn := context.WithCancel(xcontext.Detach(ctx)) + conn := ts.Connect(bgCtx) + env := &Env{ T: tb, Ctx: ctx, @@ -138,12 +144,12 @@ func NewEnv(ctx context.Context, tb testing.TB, sandbox *fake.Sandbox, ts server OnUnregistration: env.onUnregistration, } } - editor, err := fake.NewEditor(sandbox, editorConfig).Connect(ctx, conn, hooks) + editor, err := fake.NewEditor(sandbox, editorConfig).Connect(bgCtx, conn, hooks) if err != nil { tb.Fatal(err) } env.Editor = editor - return env + return env, cleanupConn } func (e *Env) onDiagnostics(_ context.Context, d *protocol.PublishDiagnosticsParams) error { diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go index 3cfeb772a1..bebec53c52 100644 --- a/internal/lsp/regtest/runner.go +++ b/internal/lsp/regtest/runner.go @@ -234,7 +234,7 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio tests := []struct { name string mode Mode - getServer func(context.Context, *testing.T, func(*source.Options)) jsonrpc2.StreamServer + getServer func(*testing.T, func(*source.Options)) jsonrpc2.StreamServer }{ {"singleton", Singleton, singletonServer}, {"forwarded", Forwarded, r.forwardedServer}, @@ -301,14 +301,15 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio // better solution to ensure that all Go processes started by gopls have // exited before we clean up. r.AddCloser(sandbox) - ss := tc.getServer(ctx, t, config.optionsHook) + ss := tc.getServer(t, config.optionsHook) framer := jsonrpc2.NewRawStream ls := &loggingFramer{} if !config.skipLogs { framer = ls.framer(jsonrpc2.NewRawStream) } - ts := servertest.NewPipeServer(ctx, ss, framer) - env := NewEnv(ctx, t, sandbox, ts, config.editor, !config.skipHooks) + ts := servertest.NewPipeServer(ss, framer) + env, cleanup := NewEnv(ctx, t, sandbox, ts, config.editor, !config.skipHooks) + defer cleanup() defer func() { if t.Failed() && r.PrintGoroutinesOnFailure { pprof.Lookup("goroutine").WriteTo(os.Stderr, 1) @@ -406,11 +407,11 @@ func (s *loggingFramer) printBuffers(testname string, w io.Writer) { fmt.Fprintf(os.Stderr, "#### End Gopls Test Logs for %q\n", testname) } -func singletonServer(ctx context.Context, t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { +func singletonServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { return lsprpc.NewStreamServer(cache.New(optsHook), false) } -func experimentalServer(_ context.Context, t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { +func experimentalServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { options := func(o *source.Options) { optsHook(o) o.EnableAllExperiments() @@ -421,7 +422,7 @@ func experimentalServer(_ context.Context, t *testing.T, optsHook func(*source.O return lsprpc.NewStreamServer(cache.New(options), false) } -func (r *Runner) forwardedServer(ctx context.Context, t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { +func (r *Runner) forwardedServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { ts := r.getTestServer(optsHook) return newForwarder("tcp", ts.Addr) } @@ -440,7 +441,7 @@ func (r *Runner) getTestServer(optsHook func(*source.Options)) *servertest.TCPSe return r.ts } -func (r *Runner) separateProcessServer(ctx context.Context, t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { +func (r *Runner) separateProcessServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { // TODO(rfindley): can we use the autostart behavior here, instead of // pre-starting the remote? socket := r.getRemoteSocket(t)