From 5fb17a1e7b9b41efa62ea3f2c593156af7609897 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 10 Feb 2020 11:34:13 -0500 Subject: [PATCH] internal/jsonrpc2: support serving over unix domain sockets For tests (and perhaps later, for daemon discovery), unix domain sockets offer advantages over TCP: we can know the exact socket address that will be used when starting a server subprocess. They also offer performance and security advantages over TCP, and were specifically requested on golang.org/issues/34111. This CL adds support for listening on UDS, and uses this to implement an additional regtest environment mode that starts up an external process. This mode is disabled by default, but may be enabled by the -enable_gopls_subprocess_tests. The regtest TestMain may be hijacked to instead run as gopls, if a special environment variable is set. This allows the the test runner to start a separate process by using os.Argv[0]. The -gopls_test_binary flag may be used to point tests at a separate gopls binary. Updates golang/go#36879 Updates golang/go#34111 Change-Id: I1cfdf55040e81ffa69a6726878a96529e5522e82 Reviewed-on: https://go-review.googlesource.com/c/tools/+/218839 Run-TryBot: Robert Findley TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/jsonrpc2/serve.go | 11 +- internal/jsonrpc2/servertest/servertest.go | 6 +- internal/lsp/cmd/cmd.go | 2 +- internal/lsp/cmd/serve.go | 19 +++- internal/lsp/cmd/serve_test.go | 26 +++++ internal/lsp/lsprpc/lsprpc.go | 39 ++++++- internal/lsp/lsprpc/lsprpc_test.go | 2 +- internal/lsp/regtest/env.go | 112 ++++++++++++++++++--- internal/lsp/regtest/reg_test.go | 50 ++++++++- internal/lsp/regtest/shared_test.go | 10 +- 10 files changed, 237 insertions(+), 40 deletions(-) create mode 100644 internal/lsp/cmd/serve_test.go diff --git a/internal/jsonrpc2/serve.go b/internal/jsonrpc2/serve.go index cc50a260f9..ff9a7a394b 100644 --- a/internal/jsonrpc2/serve.go +++ b/internal/jsonrpc2/serve.go @@ -6,6 +6,7 @@ package jsonrpc2 import ( "context" + "log" "net" ) @@ -41,8 +42,8 @@ func HandlerServer(h Handler) StreamServer { // ListenAndServe starts an jsonrpc2 server on the given address. It exits only // on error. -func ListenAndServe(ctx context.Context, addr string, server StreamServer) error { - ln, err := net.Listen("tcp", addr) +func ListenAndServe(ctx context.Context, network, addr string, server StreamServer) error { + ln, err := net.Listen(network, addr) if err != nil { return err } @@ -58,6 +59,10 @@ func Serve(ctx context.Context, ln net.Listener, server StreamServer) error { return err } stream := NewHeaderStream(netConn, netConn) - go server.ServeStream(ctx, stream) + go func() { + if err := server.ServeStream(ctx, stream); err != nil { + log.Printf("serving stream: %v", err) + } + }() } } diff --git a/internal/jsonrpc2/servertest/servertest.go b/internal/jsonrpc2/servertest/servertest.go index 818662d500..393d5f13bb 100644 --- a/internal/jsonrpc2/servertest/servertest.go +++ b/internal/jsonrpc2/servertest/servertest.go @@ -27,7 +27,7 @@ type Connector interface { type TCPServer struct { Addr string - ln net.Listener + ln net.Listener cls *closerList } @@ -67,7 +67,7 @@ func (s *TCPServer) Close() error { // PipeServer is a test server that handles connections over io.Pipes. type PipeServer struct { server jsonrpc2.StreamServer - cls *closerList + cls *closerList } // NewPipeServer returns a test server that can be connected to via io.Pipes. @@ -107,7 +107,7 @@ func (s *PipeServer) Close() error { // convenience, so that callers don't have to worry about closing each // connection. type closerList struct { - mu sync.Mutex + mu sync.Mutex closers []func() } diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index 7bb5cb9c9c..114ab9591c 100644 --- a/internal/lsp/cmd/cmd.go +++ b/internal/lsp/cmd/cmd.go @@ -58,7 +58,7 @@ type Application struct { env []string // Support for remote lsp server - Remote string `flag:"remote" help:"*EXPERIMENTAL* - forward all commands to a remote lsp"` + Remote string `flag:"remote" help:"*EXPERIMENTAL* - forward all commands to a remote lsp specified by this flag. If prefixed by 'unix;', the subsequent address is assumed to be a unix domain socket. Otherwise, TCP is used."` // Enable verbose logging Verbose bool `flag:"v" help:"verbose output"` diff --git a/internal/lsp/cmd/serve.go b/internal/lsp/cmd/serve.go index 7c41d79201..54a443ee01 100644 --- a/internal/lsp/cmd/serve.go +++ b/internal/lsp/cmd/serve.go @@ -9,6 +9,7 @@ import ( "flag" "fmt" "os" + "strings" "golang.org/x/tools/internal/jsonrpc2" "golang.org/x/tools/internal/lsp/cache" @@ -23,7 +24,7 @@ type Serve struct { Logfile string `flag:"logfile" help:"filename to log to. if value is \"auto\", then logging to a default output file is enabled"` Mode string `flag:"mode" help:"no effect"` Port int `flag:"port" help:"port on which to run gopls for debugging purposes"` - Address string `flag:"listen" help:"address on which to listen for remote connections"` + Address string `flag:"listen" help:"address on which to listen for remote connections. If prefixed by 'unix;', the subsequent address is assumed to be a unix domain socket. Otherwise, TCP is used."` Trace bool `flag:"rpc.trace" help:"print the full rpc trace in lsp inspector format"` Debug string `flag:"debug" help:"serve debug information on the supplied address"` @@ -64,17 +65,19 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { var ss jsonrpc2.StreamServer if s.app.Remote != "" { - ss = lsprpc.NewForwarder(s.app.Remote, true) + network, addr := parseAddr(s.app.Remote) + ss = lsprpc.NewForwarder(network, addr, true) } else { ss = lsprpc.NewStreamServer(cache.New(s.app.options), true) } if s.Address != "" { - return jsonrpc2.ListenAndServe(ctx, s.Address, ss) + network, addr := parseAddr(s.Address) + return jsonrpc2.ListenAndServe(ctx, network, addr, ss) } if s.Port != 0 { addr := fmt.Sprintf(":%v", s.Port) - return jsonrpc2.ListenAndServe(ctx, addr, ss) + return jsonrpc2.ListenAndServe(ctx, "tcp", addr, ss) } stream := jsonrpc2.NewHeaderStream(os.Stdin, os.Stdout) if s.Trace { @@ -82,3 +85,11 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { } return ss.ServeStream(ctx, stream) } + +// parseAddr parses the -listen flag in to a network, and address. +func parseAddr(listen string) (network string, address string) { + if parts := strings.SplitN(listen, ";", 2); len(parts) == 2 { + return parts[0], parts[1] + } + return "tcp", listen +} diff --git a/internal/lsp/cmd/serve_test.go b/internal/lsp/cmd/serve_test.go new file mode 100644 index 0000000000..a5a958316f --- /dev/null +++ b/internal/lsp/cmd/serve_test.go @@ -0,0 +1,26 @@ +// Copyright 2020 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 cmd + +import "testing" + +func TestListenParsing(t *testing.T) { + tests := []struct { + input, wantNetwork, wantAddr string + }{ + {"127.0.0.1:0", "tcp", "127.0.0.1:0"}, + {"unix;/tmp/sock", "unix", "/tmp/sock"}, + } + + for _, test := range tests { + gotNetwork, gotAddr := parseAddr(test.input) + if gotNetwork != test.wantNetwork { + t.Errorf("network = %q, want %q", gotNetwork, test.wantNetwork) + } + if gotAddr != test.wantAddr { + t.Errorf("addr = %q, want %q", gotAddr, test.wantAddr) + } + } +} diff --git a/internal/lsp/lsprpc/lsprpc.go b/internal/lsp/lsprpc/lsprpc.go index 53df859d50..346d5e6bb4 100644 --- a/internal/lsp/lsprpc/lsprpc.go +++ b/internal/lsp/lsprpc/lsprpc.go @@ -9,8 +9,10 @@ package lsprpc import ( "context" "fmt" + "log" "net" "os" + "time" "golang.org/x/sync/errgroup" "golang.org/x/tools/internal/jsonrpc2" @@ -63,16 +65,24 @@ func (s *StreamServer) ServeStream(ctx context.Context, stream jsonrpc2.Stream) // be instrumented with telemetry, and want to be able to in some cases hijack // the jsonrpc2 connection with the daemon. type Forwarder struct { - remote string + network, addr string + + // Configuration. Right now, not all of this may be customizable, but in the + // future it probably will be. withTelemetry bool + dialTimeout time.Duration + retries int } // NewForwarder creates a new Forwarder, ready to forward connections to the -// given remote. -func NewForwarder(remote string, withTelemetry bool) *Forwarder { +// remote server specified by network and addr. +func NewForwarder(network, addr string, withTelemetry bool) *Forwarder { return &Forwarder{ - remote: remote, + network: network, + addr: addr, withTelemetry: withTelemetry, + dialTimeout: 1 * time.Second, + retries: 5, } } @@ -82,7 +92,26 @@ func (f *Forwarder) ServeStream(ctx context.Context, stream jsonrpc2.Stream) err clientConn := jsonrpc2.NewConn(stream) client := protocol.ClientDispatcher(clientConn) - netConn, err := net.Dial("tcp", f.remote) + var ( + netConn net.Conn + err error + ) + // Sometimes the forwarder will be started immediately after the server is + // started. To account for these cases, add in some simple retrying. + // Note that the number of total attempts is f.retries + 1. + for attempt := 0; attempt <= f.retries; attempt++ { + startDial := time.Now() + netConn, err = net.DialTimeout(f.network, f.addr, f.dialTimeout) + if err == nil { + break + } + log.Printf("failed an attempt to connect to remote: %v\n", err) + // In case our failure was a fast-failure, ensure we wait at least + // f.dialTimeout before trying again. + if attempt != f.retries { + time.Sleep(f.dialTimeout - time.Since(startDial)) + } + } if err != nil { return fmt.Errorf("forwarder: dialing remote: %v", err) } diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go index 4ecf974805..26aae8fffa 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/internal/lsp/lsprpc/lsprpc_test.go @@ -102,7 +102,7 @@ func TestRequestCancellation(t *testing.T) { ctx := context.Background() tsDirect := servertest.NewTCPServer(ctx, ss) - forwarder := NewForwarder(tsDirect.Addr, false) + forwarder := NewForwarder("tcp", tsDirect.Addr, false) tsForwarded := servertest.NewPipeServer(ctx, forwarder) tests := []struct { diff --git a/internal/lsp/regtest/env.go b/internal/lsp/regtest/env.go index 03d7a551bf..cb5808618d 100644 --- a/internal/lsp/regtest/env.go +++ b/internal/lsp/regtest/env.go @@ -6,8 +6,13 @@ package regtest import ( + "bytes" "context" "fmt" + "io/ioutil" + "os" + "os/exec" + "path/filepath" "strings" "sync" "testing" @@ -25,14 +30,17 @@ import ( type EnvMode int const ( - // Singleton mode uses a separate cache for each test + // Singleton mode uses a separate cache for each test. Singleton EnvMode = 1 << iota - // Shared mode uses a Shared cache + // Shared mode uses a Shared cache. Shared - // Forwarded forwards connections + // Forwarded forwards connections to an in-process gopls instance. Forwarded - // AllModes runs tests in all modes - AllModes = Singleton | Shared | Forwarded + // SeparateProcess runs a separate gopls process, and forwards connections to + // it. + SeparateProcess + // NormalModes runs tests in all modes. + NormalModes = Singleton | Shared | Forwarded ) // A Runner runs tests in gopls execution environments, as specified by its @@ -40,26 +48,90 @@ const ( // remote), any tests that execute on the same Runner will share the same // state. type Runner struct { - ts *servertest.TCPServer defaultModes EnvMode timeout time.Duration + goplsPath string + + mu sync.Mutex + ts *servertest.TCPServer + socketDir string } // NewTestRunner creates a Runner with its shared state initialized, ready to // run tests. -func NewTestRunner(modes EnvMode, testTimeout time.Duration) *Runner { - ss := lsprpc.NewStreamServer(cache.New(nil), false) - ts := servertest.NewTCPServer(context.Background(), ss) +func NewTestRunner(modes EnvMode, testTimeout time.Duration, goplsPath string) *Runner { return &Runner{ - ts: ts, defaultModes: modes, timeout: testTimeout, + goplsPath: goplsPath, } } +// Modes returns the bitmask of environment modes this runner is configured to +// test. +func (r *Runner) Modes() EnvMode { + return r.defaultModes +} + +// getTestServer gets the test server instance to connect to, or creates one if +// it doesn't exist. +func (r *Runner) getTestServer() *servertest.TCPServer { + r.mu.Lock() + defer r.mu.Unlock() + if r.ts == nil { + ss := lsprpc.NewStreamServer(cache.New(nil), false) + r.ts = servertest.NewTCPServer(context.Background(), ss) + } + return r.ts +} + +// runTestAsGoplsEnvvar triggers TestMain to run gopls instead of running +// tests. It's a trick to allow tests to find a binary to use to start a gopls +// subprocess. +const runTestAsGoplsEnvvar = "_GOPLS_TEST_BINARY_RUN_AS_GOPLS" + +func (r *Runner) getRemoteSocket(t *testing.T) string { + t.Helper() + r.mu.Lock() + defer r.mu.Unlock() + const daemonFile = "gopls-test-daemon" + if r.socketDir != "" { + return filepath.Join(r.socketDir, daemonFile) + } + + if r.goplsPath == "" { + t.Fatal("cannot run tests with a separate process unless a path to a gopls binary is configured") + } + var err error + r.socketDir, err = ioutil.TempDir("", "gopls-regtests") + if err != nil { + t.Fatalf("creating tempdir: %v", err) + } + socket := filepath.Join(r.socketDir, daemonFile) + args := []string{"serve", "-listen", "unix;" + socket} + cmd := exec.Command(r.goplsPath, args...) + cmd.Env = append(os.Environ(), runTestAsGoplsEnvvar+"=true") + var stderr bytes.Buffer + cmd.Stderr = &stderr + go func() { + if err := cmd.Run(); err != nil { + panic(fmt.Sprintf("error running external gopls: %v\nstderr:\n%s", err, stderr.String())) + } + }() + return socket +} + // Close cleans up resource that have been allocated to this workspace. func (r *Runner) Close() error { - return r.ts.Close() + r.mu.Lock() + defer r.mu.Unlock() + if r.ts != nil { + r.ts.Close() + } + if r.socketDir != "" { + os.RemoveAll(r.socketDir) + } + return nil } // Run executes the test function in the default configured gopls execution @@ -81,6 +153,7 @@ func (r *Runner) RunInMode(modes EnvMode, t *testing.T, filedata string, test fu {"singleton", Singleton, r.singletonEnv}, {"shared", Shared, r.sharedEnv}, {"forwarded", Forwarded, r.forwardedEnv}, + {"separate_process", SeparateProcess, r.separateProcessEnv}, } for _, tc := range tests { @@ -115,12 +188,23 @@ func (r *Runner) singletonEnv(ctx context.Context, t *testing.T) (servertest.Con } func (r *Runner) sharedEnv(ctx context.Context, t *testing.T) (servertest.Connector, func()) { - return r.ts, func() {} + return r.getTestServer(), func() {} } func (r *Runner) forwardedEnv(ctx context.Context, t *testing.T) (servertest.Connector, func()) { - forwarder := lsprpc.NewForwarder(r.ts.Addr, false) - ts2 := servertest.NewTCPServer(ctx, forwarder) + ts := r.getTestServer() + forwarder := lsprpc.NewForwarder("tcp", ts.Addr, false) + ts2 := servertest.NewPipeServer(ctx, forwarder) + cleanup := func() { + ts2.Close() + } + return ts2, cleanup +} + +func (r *Runner) separateProcessEnv(ctx context.Context, t *testing.T) (servertest.Connector, func()) { + socket := r.getRemoteSocket(t) + forwarder := lsprpc.NewForwarder("unix", socket, false) + ts2 := servertest.NewPipeServer(ctx, forwarder) cleanup := func() { ts2.Close() } diff --git a/internal/lsp/regtest/reg_test.go b/internal/lsp/regtest/reg_test.go index 15c712aab8..d139e2a4b8 100644 --- a/internal/lsp/regtest/reg_test.go +++ b/internal/lsp/regtest/reg_test.go @@ -5,18 +5,33 @@ package regtest import ( + "context" + "flag" "fmt" "os" + "path/filepath" "testing" "time" "golang.org/x/tools/internal/lsp" + "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/lsp/lsprpc" + "golang.org/x/tools/internal/tool" +) + +var ( + runSubprocessTests = flag.Bool("enable_gopls_subprocess_tests", false, "run regtests against a gopls subprocess") + goplsBinaryPath = flag.String("gopls_test_binary", "", "path to the gopls binary for use as a remote, for use with the -gopls_subprocess_testmode flag") ) var runner *Runner func TestMain(m *testing.M) { + flag.Parse() + if os.Getenv("_GOPLS_TEST_BINARY_RUN_AS_GOPLS") == "true" { + tool.Main(context.Background(), cmd.New("gopls", "", nil, nil), os.Args[1:]) + os.Exit(0) + } // Override functions that would shut down the test process defer func(lspExit, forwarderExit func(code int)) { lsp.ServerExitFunc = lspExit @@ -28,7 +43,36 @@ func TestMain(m *testing.M) { } // We don't want our forwarders to exit, but it's OK if they would have. lsprpc.ForwarderExitFunc = func(code int) {} - runner = NewTestRunner(AllModes, 30*time.Second) - defer runner.Close() - os.Exit(m.Run()) + + if *runSubprocessTests { + goplsPath := *goplsBinaryPath + if goplsPath == "" { + var err error + goplsPath, err = testBinaryPath() + if err != nil { + panic(fmt.Sprintf("finding test binary path: %v", err)) + } + } + runner = NewTestRunner(NormalModes|SeparateProcess, 30*time.Second, goplsPath) + } else { + runner = NewTestRunner(NormalModes, 30*time.Second, "") + } + code := m.Run() + runner.Close() + os.Exit(code) +} + +func testBinaryPath() (string, error) { + pth := os.Args[0] + if !filepath.IsAbs(pth) { + cwd, err := os.Getwd() + if err == nil { + return "", fmt.Errorf("os.Getwd: %v", err) + } + pth = filepath.Join(cwd, pth) + } + if _, err := os.Stat(pth); err != nil { + return "", fmt.Errorf("os.Stat: %v", err) + } + return pth, nil } diff --git a/internal/lsp/regtest/shared_test.go b/internal/lsp/regtest/shared_test.go index 6914e35f02..565241b4bd 100644 --- a/internal/lsp/regtest/shared_test.go +++ b/internal/lsp/regtest/shared_test.go @@ -26,7 +26,9 @@ func main() { }` func runShared(t *testing.T, program string, testFunc func(ctx context.Context, t *testing.T, env1 *Env, env2 *Env)) { - runner.RunInMode(Forwarded, t, sharedProgram, func(ctx context.Context, t *testing.T, env1 *Env) { + // Only run these tests in forwarded modes. + modes := runner.Modes() & (Forwarded | SeparateProcess) + runner.RunInMode(modes, t, sharedProgram, func(ctx context.Context, t *testing.T, env1 *Env) { // Create a second test session connected to the same workspace and server // as the first. env2 := NewEnv(ctx, t, env1.W, env1.Server) @@ -36,11 +38,7 @@ func runShared(t *testing.T, program string, testFunc func(ctx context.Context, func TestSimultaneousEdits(t *testing.T) { t.Parallel() - runner.Run(t, exampleProgram, func(ctx context.Context, t *testing.T, env1 *Env) { - // Create a second test session connected to the same workspace and server - // as the first. - env2 := NewEnv(ctx, t, env1.W, env1.Server) - + runShared(t, exampleProgram, func(ctx context.Context, t *testing.T, env1 *Env, env2 *Env) { // In editor #1, break fmt.Println as before. edit1 := fake.NewEdit(5, 11, 5, 12, "") env1.OpenFile("main.go")