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 <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

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)
 	})

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
 	}

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 {

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{})

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 {

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)