From 980829d8a1276d654bdf03f4a38f26eee797d0b1 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 2 Jul 2021 11:20:25 -0400 Subject: [PATCH] internal/lsp/lsprpc: add an AutoDialer abstraction Refactor the lsprpc package to move the logic for 'automatic' server discovery into an AutoDialer abstraction, which both implements the v2 jsonrpc2 Dialer interface, and provides a dialNet method that can be used for the existing v1 APIs. Along the way, simplify the evaluation of remote arguments to eliminate the overly abstract RemoteOption. Change-Id: Ic3def17ccc237007a7eb2cc41a12cf058fca9be3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/332490 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Ian Cottrell --- internal/lsp/cmd/serve.go | 27 ++- internal/lsp/lsprpc/autostart_default.go | 6 +- internal/lsp/lsprpc/autostart_posix.go | 12 +- internal/lsp/lsprpc/dialer.go | 115 +++++++++++++ internal/lsp/lsprpc/lsprpc.go | 206 +++-------------------- internal/lsp/lsprpc/lsprpc_test.go | 10 +- internal/lsp/regtest/runner.go | 13 +- 7 files changed, 187 insertions(+), 202 deletions(-) create mode 100644 internal/lsp/lsprpc/dialer.go diff --git a/internal/lsp/cmd/serve.go b/internal/lsp/cmd/serve.go index 6d0787efdf5..4164b5869fd 100644 --- a/internal/lsp/cmd/serve.go +++ b/internal/lsp/cmd/serve.go @@ -56,6 +56,22 @@ gopls server flags are: f.PrintDefaults() } +func (s *Serve) remoteArgs(network, address string) []string { + args := []string{"serve", + "-listen", fmt.Sprintf(`%s;%s`, network, address), + } + if s.RemoteDebug != "" { + args = append(args, "-debug", s.RemoteDebug) + } + if s.RemoteListenTimeout != 0 { + args = append(args, "-listen.timeout", s.RemoteListenTimeout.String()) + } + if s.RemoteLogfile != "" { + args = append(args, "-logfile", s.RemoteLogfile) + } + return args +} + // Run configures a server based on the flags, and then runs it. // It blocks until the server shuts down. func (s *Serve) Run(ctx context.Context, args ...string) error { @@ -77,12 +93,11 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { } var ss jsonrpc2.StreamServer if s.app.Remote != "" { - network, addr := lsprpc.ParseAddr(s.app.Remote) - ss = lsprpc.NewForwarder(network, addr, - lsprpc.RemoteDebugAddress(s.RemoteDebug), - lsprpc.RemoteListenTimeout(s.RemoteListenTimeout), - lsprpc.RemoteLogfile(s.RemoteLogfile), - ) + var err error + ss, err = lsprpc.NewForwarder(s.app.Remote, s.remoteArgs) + if err != nil { + return errors.Errorf("creating forwarder: %w", err) + } } else { ss = lsprpc.NewStreamServer(cache.New(s.app.options), isDaemon) } diff --git a/internal/lsp/lsprpc/autostart_default.go b/internal/lsp/lsprpc/autostart_default.go index dc04f663f63..b23a1e50882 100644 --- a/internal/lsp/lsprpc/autostart_default.go +++ b/internal/lsp/lsprpc/autostart_default.go @@ -11,13 +11,13 @@ import ( ) var ( - startRemote = startRemoteDefault + daemonize = func(*exec.Cmd) {} autoNetworkAddress = autoNetworkAddressDefault verifyRemoteOwnership = verifyRemoteOwnershipDefault ) -func startRemoteDefault(goplsPath string, args ...string) error { - cmd := exec.Command(goplsPath, args...) +func runRemote(cmd *exec.Cmd) error { + daemonize(cmd) if err := cmd.Start(); err != nil { return errors.Errorf("starting remote gopls: %w", err) } diff --git a/internal/lsp/lsprpc/autostart_posix.go b/internal/lsp/lsprpc/autostart_posix.go index 45089b8e357..d5644e2b685 100644 --- a/internal/lsp/lsprpc/autostart_posix.go +++ b/internal/lsp/lsprpc/autostart_posix.go @@ -11,7 +11,6 @@ import ( "crypto/sha256" "errors" "fmt" - exec "golang.org/x/sys/execabs" "log" "os" "os/user" @@ -19,24 +18,21 @@ import ( "strconv" "syscall" + exec "golang.org/x/sys/execabs" + "golang.org/x/xerrors" ) func init() { - startRemote = startRemotePosix + daemonize = daemonizePosix autoNetworkAddress = autoNetworkAddressPosix verifyRemoteOwnership = verifyRemoteOwnershipPosix } -func startRemotePosix(goplsPath string, args ...string) error { - cmd := exec.Command(goplsPath, args...) +func daemonizePosix(cmd *exec.Cmd) { cmd.SysProcAttr = &syscall.SysProcAttr{ Setsid: true, } - if err := cmd.Start(); err != nil { - return xerrors.Errorf("starting remote gopls: %w", err) - } - return nil } // autoNetworkAddress resolves an id on the 'auto' pseduo-network to a diff --git a/internal/lsp/lsprpc/dialer.go b/internal/lsp/lsprpc/dialer.go new file mode 100644 index 00000000000..713307ca04e --- /dev/null +++ b/internal/lsp/lsprpc/dialer.go @@ -0,0 +1,115 @@ +// Copyright 2021 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 lsprpc + +import ( + "context" + "fmt" + "io" + "net" + "os" + "time" + + exec "golang.org/x/sys/execabs" + "golang.org/x/tools/internal/event" + errors "golang.org/x/xerrors" +) + +// AutoNetwork is the pseudo network type used to signal that gopls should use +// automatic discovery to resolve a remote address. +const AutoNetwork = "auto" + +// An AutoDialer is a jsonrpc2 dialer that understands the 'auto' network. +type AutoDialer struct { + network, addr string // the 'real' network and address + isAuto bool // whether the server is on the 'auto' network + + executable string + argFunc func(network, addr string) []string +} + +func NewAutoDialer(rawAddr string, argFunc func(network, addr string) []string) (*AutoDialer, error) { + d := AutoDialer{ + argFunc: argFunc, + } + d.network, d.addr = ParseAddr(rawAddr) + if d.network == AutoNetwork { + d.isAuto = true + bin, err := os.Executable() + if err != nil { + return nil, errors.Errorf("getting executable: %w", err) + } + d.executable = bin + d.network, d.addr = autoNetworkAddress(bin, d.addr) + } + return &d, nil +} + +// Dial implements the jsonrpc2.Dialer interface. +func (d *AutoDialer) Dial(ctx context.Context) (io.ReadWriteCloser, error) { + conn, err := d.dialNet(ctx) + return conn, err +} + +// TODO(rFindley): remove this once we no longer need to integrate with v1 of +// the jsonrpc2 package. +func (d *AutoDialer) dialNet(ctx context.Context) (net.Conn, error) { + // Attempt to verify that we own the remote. This is imperfect, but if we can + // determine that the remote is owned by a different user, we should fail. + ok, err := verifyRemoteOwnership(d.network, d.addr) + if err != nil { + // If the ownership check itself failed, we fail open but log an error to + // the user. + event.Error(ctx, "unable to check daemon socket owner, failing open", err) + } else if !ok { + // We successfully checked that the socket is not owned by us, we fail + // closed. + return nil, fmt.Errorf("socket %q is owned by a different user", d.addr) + } + const dialTimeout = 1 * time.Second + // Try dialing our remote once, in case it is already running. + netConn, err := net.DialTimeout(d.network, d.addr, dialTimeout) + if err == nil { + return netConn, nil + } + if d.isAuto && d.argFunc != nil { + if d.network == "unix" { + // Sometimes the socketfile isn't properly cleaned up when the server + // shuts down. Since we have already tried and failed to dial this + // address, it should *usually* be safe to remove the socket before + // binding to the address. + // TODO(rfindley): there is probably a race here if multiple server + // instances are simultaneously starting up. + if _, err := os.Stat(d.addr); err == nil { + if err := os.Remove(d.addr); err != nil { + return nil, errors.Errorf("removing remote socket file: %w", err) + } + } + } + args := d.argFunc(d.network, d.addr) + cmd := exec.Command(d.executable, args...) + if err := runRemote(cmd); err != nil { + return nil, err + } + } + + const retries = 5 + // It can take some time for the newly started server to bind to our address, + // so we retry for a bit. + for retry := 0; retry < retries; retry++ { + startDial := time.Now() + netConn, err = net.DialTimeout(d.network, d.addr, dialTimeout) + if err == nil { + return netConn, nil + } + event.Log(ctx, fmt.Sprintf("failed attempt #%d to connect to remote: %v\n", retry+2, err)) + // In case our failure was a fast-failure, ensure we wait at least + // f.dialTimeout before trying again. + if retry != retries-1 { + time.Sleep(dialTimeout - time.Since(startDial)) + } + } + return nil, errors.Errorf("dialing remote: %w", err) +} diff --git a/internal/lsp/lsprpc/lsprpc.go b/internal/lsp/lsprpc/lsprpc.go index 18e0299898e..91770780c22 100644 --- a/internal/lsp/lsprpc/lsprpc.go +++ b/internal/lsp/lsprpc/lsprpc.go @@ -21,7 +21,6 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/jsonrpc2" - jsonrpc2_v2 "golang.org/x/tools/internal/jsonrpc2_v2" "golang.org/x/tools/internal/lsp" "golang.org/x/tools/internal/lsp/cache" "golang.org/x/tools/internal/lsp/command" @@ -31,10 +30,6 @@ import ( errors "golang.org/x/xerrors" ) -// AutoNetwork is the pseudo network type used to signal that gopls should use -// automatic discovery to resolve a remote address. -const AutoNetwork = "auto" - // Unique identifiers for client/server. var serverIndex int64 @@ -113,13 +108,7 @@ func (s *StreamServer) ServeStream(ctx context.Context, conn jsonrpc2.Conn) erro // be instrumented with telemetry, and want to be able to in some cases hijack // the jsonrpc2 connection with the daemon. type Forwarder struct { - network, addr string - - // goplsPath is the path to the current executing gopls binary. - goplsPath string - - // configuration for the auto-started gopls remote. - remoteConfig remoteConfig + dialer *AutoDialer mu sync.Mutex // Hold on to the server connection so that we can redo the handshake if any @@ -128,68 +117,19 @@ type Forwarder struct { serverID string } -type remoteConfig struct { - debug string - listenTimeout time.Duration - logfile string -} - -// A RemoteOption configures the behavior of the auto-started remote. -type RemoteOption interface { - set(*remoteConfig) -} - -// RemoteDebugAddress configures the address used by the auto-started Gopls daemon -// for serving debug information. -type RemoteDebugAddress string - -func (d RemoteDebugAddress) set(cfg *remoteConfig) { - cfg.debug = string(d) -} - -// RemoteListenTimeout configures the amount of time the auto-started gopls -// daemon will wait with no client connections before shutting down. -type RemoteListenTimeout time.Duration - -func (d RemoteListenTimeout) set(cfg *remoteConfig) { - cfg.listenTimeout = time.Duration(d) -} - -// RemoteLogfile configures the logfile location for the auto-started gopls -// daemon. -type RemoteLogfile string - -func (l RemoteLogfile) set(cfg *remoteConfig) { - cfg.logfile = string(l) -} - -func defaultRemoteConfig() remoteConfig { - return remoteConfig{ - listenTimeout: 1 * time.Minute, - } -} - // NewForwarder creates a new Forwarder, ready to forward connections to the -// remote server specified by network and addr. -func NewForwarder(network, addr string, opts ...RemoteOption) *Forwarder { - gp, err := os.Executable() +// remote server specified by rawAddr. If provided and rawAddr indicates an +// 'automatic' address (starting with 'auto;'), argFunc may be used to start a +// remote server for the auto-discovered address. +func NewForwarder(rawAddr string, argFunc func(network, address string) []string) (*Forwarder, error) { + dialer, err := NewAutoDialer(rawAddr, argFunc) if err != nil { - log.Printf("error getting gopls path for forwarder: %v", err) - gp = "" - } - - rcfg := defaultRemoteConfig() - for _, opt := range opts { - opt.set(&rcfg) + return nil, err } - fwd := &Forwarder{ - network: network, - addr: addr, - goplsPath: gp, - remoteConfig: rcfg, + dialer: dialer, } - return fwd + return fwd, nil } // QueryServerState queries the server state of the current server. @@ -247,7 +187,7 @@ func ExecuteCommand(ctx context.Context, addr string, id string, request, result func (f *Forwarder) ServeStream(ctx context.Context, clientConn jsonrpc2.Conn) error { client := protocol.ClientDispatcher(clientConn) - netConn, err := f.connectToRemote(ctx) + netConn, err := f.dialer.dialNet(ctx) if err != nil { return errors.Errorf("forwarder: connecting to remote: %w", err) } @@ -293,19 +233,19 @@ func (f *Forwarder) ServeStream(ctx context.Context, clientConn jsonrpc2.Conn) e return err } -func (f *Forwarder) Binder() *ForwardBinder { - network, address := realNetworkAddress(f.network, f.addr, f.goplsPath) - dialer := jsonrpc2_v2.NetDialer(network, address, net.Dialer{ - Timeout: 5 * time.Second, - }) - return NewForwardBinder(dialer) -} - +// TODO(rfindley): remove this handshaking in favor of middleware. func (f *Forwarder) handshake(ctx context.Context) { + // This call to os.Execuable is redundant, and will be eliminated by the + // transition to the V2 API. + goplsPath, err := os.Executable() + if err != nil { + event.Error(ctx, "getting executable for handshake", err) + goplsPath = "" + } var ( hreq = handshakeRequest{ ServerID: f.serverID, - GoplsPath: f.goplsPath, + GoplsPath: goplsPath, } hresp handshakeResponse ) @@ -318,8 +258,8 @@ func (f *Forwarder) handshake(ctx context.Context) { // here. Handshakes have become functional in nature. event.Error(ctx, "forwarder: gopls handshake failed", err) } - if hresp.GoplsPath != f.goplsPath { - event.Error(ctx, "", fmt.Errorf("forwarder: gopls path mismatch: forwarder is %q, remote is %q", f.goplsPath, hresp.GoplsPath)) + if hresp.GoplsPath != goplsPath { + event.Error(ctx, "", fmt.Errorf("forwarder: gopls path mismatch: forwarder is %q, remote is %q", goplsPath, hresp.GoplsPath)) } event.Log(ctx, "New server", tag.NewServer.Of(f.serverID), @@ -330,108 +270,12 @@ func (f *Forwarder) handshake(ctx context.Context) { ) } -func (f *Forwarder) connectToRemote(ctx context.Context) (net.Conn, error) { - return connectToRemote(ctx, f.network, f.addr, f.goplsPath, f.remoteConfig) -} - -func ConnectToRemote(ctx context.Context, addr string, opts ...RemoteOption) (net.Conn, error) { - rcfg := defaultRemoteConfig() - for _, opt := range opts { - opt.set(&rcfg) - } - // This is not strictly necessary, as it won't be used if not connecting to - // the 'auto' remote. - goplsPath, err := os.Executable() - if err != nil { - return nil, fmt.Errorf("unable to resolve gopls path: %v", err) - } - network, address := ParseAddr(addr) - return connectToRemote(ctx, network, address, goplsPath, rcfg) -} - -func realNetworkAddress(inNetwork, inAddr, goplsPath string) (network, address string) { - if inNetwork != AutoNetwork { - return inNetwork, inAddr - } - // The "auto" network is a fake network used for service discovery. It - // resolves a known address based on gopls binary path. - return autoNetworkAddress(goplsPath, inAddr) -} - -func connectToRemote(ctx context.Context, inNetwork, inAddr, goplsPath string, rcfg remoteConfig) (net.Conn, error) { - var ( - netConn net.Conn - err error - network, address = realNetworkAddress(inNetwork, inAddr, goplsPath) - ) - // Attempt to verify that we own the remote. This is imperfect, but if we can - // determine that the remote is owned by a different user, we should fail. - ok, err := verifyRemoteOwnership(network, address) +func ConnectToRemote(ctx context.Context, addr string) (net.Conn, error) { + dialer, err := NewAutoDialer(addr, nil) if err != nil { - // If the ownership check itself failed, we fail open but log an error to - // the user. - event.Error(ctx, "unable to check daemon socket owner, failing open", err) - } else if !ok { - // We successfully checked that the socket is not owned by us, we fail - // closed. - return nil, fmt.Errorf("socket %q is owned by a different user", address) - } - const dialTimeout = 1 * time.Second - // Try dialing our remote once, in case it is already running. - netConn, err = net.DialTimeout(network, address, dialTimeout) - if err == nil { - return netConn, nil - } - // If our remote is on the 'auto' network, start it if it doesn't exist. - if inNetwork == AutoNetwork { - if goplsPath == "" { - return nil, fmt.Errorf("cannot auto-start remote: gopls path is unknown") - } - if network == "unix" { - // Sometimes the socketfile isn't properly cleaned up when gopls shuts - // down. Since we have already tried and failed to dial this address, it - // should *usually* be safe to remove the socket before binding to the - // address. - // TODO(rfindley): there is probably a race here if multiple gopls - // instances are simultaneously starting up. - if _, err := os.Stat(address); err == nil { - if err := os.Remove(address); err != nil { - return nil, errors.Errorf("removing remote socket file: %w", err) - } - } - } - args := []string{"serve", - "-listen", fmt.Sprintf(`%s;%s`, network, address), - "-listen.timeout", rcfg.listenTimeout.String(), - } - if rcfg.logfile != "" { - args = append(args, "-logfile", rcfg.logfile) - } - if rcfg.debug != "" { - args = append(args, "-debug", rcfg.debug) - } - if err := startRemote(goplsPath, args...); err != nil { - return nil, errors.Errorf("startRemote(%q, %v): %w", goplsPath, args, err) - } - } - - const retries = 5 - // It can take some time for the newly started server to bind to our address, - // so we retry for a bit. - for retry := 0; retry < retries; retry++ { - startDial := time.Now() - netConn, err = net.DialTimeout(network, address, dialTimeout) - if err == nil { - return netConn, nil - } - event.Log(ctx, fmt.Sprintf("failed attempt #%d to connect to remote: %v\n", retry+2, err)) - // In case our failure was a fast-failure, ensure we wait at least - // f.dialTimeout before trying again. - if retry != retries-1 { - time.Sleep(dialTimeout - time.Since(startDial)) - } + return nil, err } - return nil, errors.Errorf("dialing remote: %w", err) + return dialer.dialNet(ctx) } // handler intercepts messages to the daemon to enrich them with local diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go index b2902fac468..24decbef369 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/internal/lsp/lsprpc/lsprpc_test.go @@ -126,7 +126,10 @@ func setupForwarding(ctx context.Context, t *testing.T, s protocol.Server) (dire tsDirect := servertest.NewTCPServer(serveCtx, ss, nil) forwarderCtx := debug.WithInstance(ctx, "", "") - forwarder := NewForwarder("tcp", tsDirect.Addr) + forwarder, err := NewForwarder("tcp;"+tsDirect.Addr, nil) + if err != nil { + t.Fatal(err) + } tsForwarded := servertest.NewPipeServer(forwarderCtx, forwarder, nil) return tsDirect, tsForwarded, func() { checkClose(t, tsDirect.Close) @@ -218,7 +221,10 @@ func TestDebugInfoLifecycle(t *testing.T) { ss := NewStreamServer(cache, false) tsBackend := servertest.NewTCPServer(serverCtx, ss, nil) - forwarder := NewForwarder("tcp", tsBackend.Addr) + forwarder, err := NewForwarder("tcp;"+tsBackend.Addr, nil) + if err != nil { + t.Fatal(err) + } tsForwarder := servertest.NewPipeServer(clientCtx, forwarder, nil) conn1 := tsForwarder.Connect(clientCtx) diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go index fb17b492832..6b3501c1e12 100644 --- a/internal/lsp/regtest/runner.go +++ b/internal/lsp/regtest/runner.go @@ -415,7 +415,7 @@ func experimentalServer(_ context.Context, t *testing.T, optsHook func(*source.O func (r *Runner) forwardedServer(ctx context.Context, t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { ts := r.getTestServer(optsHook) - return lsprpc.NewForwarder("tcp", ts.Addr) + return newForwarder("tcp", ts.Addr) } // getTestServer gets the shared test server instance to connect to, or creates @@ -436,7 +436,16 @@ func (r *Runner) separateProcessServer(ctx context.Context, t *testing.T, optsHo // TODO(rfindley): can we use the autostart behavior here, instead of // pre-starting the remote? socket := r.getRemoteSocket(t) - return lsprpc.NewForwarder("unix", socket) + return newForwarder("unix", socket) +} + +func newForwarder(network, address string) *lsprpc.Forwarder { + server, err := lsprpc.NewForwarder(network+";"+address, nil) + if err != nil { + // This should never happen, as we are passing an explicit address. + panic(fmt.Sprintf("internal error: unable to create forwarder: %v", err)) + } + return server } // runTestAsGoplsEnvvar triggers TestMain to run gopls instead of running