diff --git a/gopls/test/gopls_test.go b/gopls/test/gopls_test.go index a17b1c1677..e074b0c556 100644 --- a/gopls/test/gopls_test.go +++ b/gopls/test/gopls_test.go @@ -44,7 +44,7 @@ func testCommandLine(t *testing.T, exporter packagestest.Exporter) { ctx := tests.Context(t) ctx = debug.WithInstance(ctx, "", "") cache := cache.New(ctx, commandLineOptions) - ss := lsprpc.NewStreamServer(cache, false) + ss := lsprpc.NewStreamServer(cache) ts := servertest.NewTCPServer(ctx, ss) for _, data := range data { defer data.Exported.Cleanup() diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index 6e1ca40c3a..99b804de00 100644 --- a/internal/lsp/cmd/cmd.go +++ b/internal/lsp/cmd/cmd.go @@ -18,6 +18,7 @@ import ( "os" "strings" "sync" + "time" "golang.org/x/tools/internal/jsonrpc2" "golang.org/x/tools/internal/lsp" @@ -81,6 +82,11 @@ func New(name, wd string, env []string, options func(*source.Options)) *Applicat wd: wd, env: env, OCAgent: "off", //TODO: Remove this line to default the exporter to on + + Serve: Serve{ + RemoteListenTimeout: 1 * time.Minute, + RemoteLogfile: "auto", + }, } return app } diff --git a/internal/lsp/cmd/cmd_test.go b/internal/lsp/cmd/cmd_test.go index be985ef4b9..454bc2c698 100644 --- a/internal/lsp/cmd/cmd_test.go +++ b/internal/lsp/cmd/cmd_test.go @@ -49,7 +49,7 @@ func testCommandLine(t *testing.T, exporter packagestest.Exporter) { func testServer(ctx context.Context) *servertest.TCPServer { ctx = debug.WithInstance(ctx, "", "") cache := cache.New(ctx, nil) - ss := lsprpc.NewStreamServer(cache, false) + ss := lsprpc.NewStreamServer(cache) return servertest.NewTCPServer(ctx, ss) } diff --git a/internal/lsp/cmd/serve.go b/internal/lsp/cmd/serve.go index fa33def500..90a34f2719 100644 --- a/internal/lsp/cmd/serve.go +++ b/internal/lsp/cmd/serve.go @@ -31,6 +31,10 @@ type Serve struct { 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"` + RemoteListenTimeout time.Duration `flag:"remote.listen.timeout" help:"when used with -remote=auto, the listen.timeout used when auto-starting the remote"` + RemoteDebug string `flag:"remote.debug" help:"when used with -remote=auto, the debug address used when auto-starting the remote"` + RemoteLogfile string `flag:"remote.logfile" help:"when used with -remote=auto, the filename for the remote daemon to log to"` + app *Application } @@ -71,9 +75,14 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { var ss jsonrpc2.StreamServer if s.app.Remote != "" { network, addr := parseAddr(s.app.Remote) - ss = lsprpc.NewForwarder(network, addr, true) + ss = lsprpc.NewForwarder(network, addr, + lsprpc.WithTelemetry(true), + lsprpc.RemoteDebugAddress(s.RemoteDebug), + lsprpc.RemoteListenTimeout(s.RemoteListenTimeout), + lsprpc.RemoteLogfile(s.RemoteLogfile), + ) } else { - ss = lsprpc.NewStreamServer(cache.New(ctx, s.app.options), true) + ss = lsprpc.NewStreamServer(cache.New(ctx, s.app.options), lsprpc.WithTelemetry(true)) } if s.Address != "" { diff --git a/internal/lsp/lsprpc/lsprpc.go b/internal/lsp/lsprpc/lsprpc.go index 39640dd79d..5502126ee6 100644 --- a/internal/lsp/lsprpc/lsprpc.go +++ b/internal/lsp/lsprpc/lsprpc.go @@ -30,6 +30,9 @@ import ( // automatic discovery to resolve a remote address. const AutoNetwork = "auto" +// Unique identifiers for client/server. +var clientIndex, serverIndex int64 + // The StreamServer type is a jsonrpc2.StreamServer that handles incoming // streams as a new LSP session, using a shared cache. type StreamServer struct { @@ -40,15 +43,28 @@ type StreamServer struct { serverForTest protocol.Server } -var clientIndex, serverIndex int64 +// A ServerOption configures the behavior of the LSP server. +type ServerOption interface { + setServer(*StreamServer) +} + +// WithTelemetry configures either a Server or Forwarder to instrument RPCs +// with additional telemetry. +type WithTelemetry bool + +func (t WithTelemetry) setServer(s *StreamServer) { + s.withTelemetry = bool(t) +} // NewStreamServer creates a StreamServer using the shared cache. If // withTelemetry is true, each session is instrumented with telemetry that // records RPC statistics. -func NewStreamServer(cache *cache.Cache, withTelemetry bool) *StreamServer { +func NewStreamServer(cache *cache.Cache, opts ...ServerOption) *StreamServer { s := &StreamServer{ - withTelemetry: withTelemetry, - cache: cache, + cache: cache, + } + for _, opt := range opts { + opt.setServer(s) } return s } @@ -133,7 +149,11 @@ func (s *StreamServer) ServeStream(ctx context.Context, stream jsonrpc2.Stream) // Clients may or may not send a shutdown message. Make sure the server is // shut down. // TODO(rFindley): this shutdown should perhaps be on a disconnected context. - defer server.Shutdown(ctx) + defer func() { + if err := server.Shutdown(ctx); err != nil { + event.Error(ctx, "error shutting down", err) + } + }() conn.AddHandler(protocol.ServerHandler(server)) conn.AddHandler(protocol.Canceller{}) if s.withTelemetry { @@ -160,31 +180,73 @@ func (s *StreamServer) ServeStream(ctx context.Context, stream jsonrpc2.Stream) type Forwarder struct { 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 - goplsPath string + // goplsPath is the path to the current executing gopls binary. + goplsPath string + + // configuration + withTelemetry bool + dialTimeout time.Duration + retries int + remoteDebug string + remoteListenTimeout time.Duration + remoteLogfile string +} + +// A ForwarderOption configures the behavior of the LSP forwarder. +type ForwarderOption interface { + setForwarder(*Forwarder) +} + +func (t WithTelemetry) setForwarder(fwd *Forwarder) { + fwd.withTelemetry = bool(t) +} + +// RemoteDebugAddress configures the address used by the auto-started Gopls daemon +// for serving debug information. +type RemoteDebugAddress string + +func (d RemoteDebugAddress) setForwarder(fwd *Forwarder) { + fwd.remoteDebug = 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) setForwarder(fwd *Forwarder) { + fwd.remoteListenTimeout = time.Duration(d) +} + +// RemoteLogfile configures the logfile location for the auto-started gopls +// daemon. +type RemoteLogfile string + +func (l RemoteLogfile) setForwarder(fwd *Forwarder) { + fwd.remoteLogfile = string(l) } // NewForwarder creates a new Forwarder, ready to forward connections to the // remote server specified by network and addr. -func NewForwarder(network, addr string, withTelemetry bool) *Forwarder { +func NewForwarder(network, addr string, opts ...ForwarderOption) *Forwarder { gp, err := os.Executable() if err != nil { log.Printf("error getting gopls path for forwarder: %v", err) gp = "" } - return &Forwarder{ - network: network, - addr: addr, - withTelemetry: withTelemetry, - dialTimeout: 1 * time.Second, - retries: 5, - goplsPath: gp, + fwd := &Forwarder{ + network: network, + addr: addr, + goplsPath: gp, + dialTimeout: 1 * time.Second, + retries: 5, + remoteLogfile: "auto", + remoteListenTimeout: 1 * time.Minute, } + for _, opt := range opts { + opt.setForwarder(fwd) + } + return fwd } // QueryServerState queries the server state of the current server. @@ -327,9 +389,11 @@ func (f *Forwarder) connectToRemote(ctx context.Context) (net.Conn, error) { } args := []string{"serve", "-listen", fmt.Sprintf(`%s;%s`, network, address), - "-listen.timeout", "1m", - "-debug", "localhost:0", - "-logfile", "auto", + "-listen.timeout", f.remoteListenTimeout.String(), + "-logfile", f.remoteLogfile, + } + if f.remoteDebug != "" { + args = append(args, "-debug", f.remoteDebug) } if err := startRemote(f.goplsPath, args...); err != nil { return nil, fmt.Errorf("startRemote(%q, %v): %v", f.goplsPath, args, err) diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go index 18e19d6623..bb2c3b3213 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/internal/lsp/lsprpc/lsprpc_test.go @@ -49,7 +49,7 @@ func TestClientLogging(t *testing.T) { client := fakeClient{logs: make(chan string, 10)} ctx = debug.WithInstance(ctx, "", "") - ss := NewStreamServer(cache.New(ctx, nil), false) + ss := NewStreamServer(cache.New(ctx, nil)) ss.serverForTest = server ts := servertest.NewPipeServer(ctx, ss) defer ts.Close() @@ -106,13 +106,13 @@ func TestRequestCancellation(t *testing.T) { } baseCtx := context.Background() serveCtx := debug.WithInstance(baseCtx, "", "") - ss := NewStreamServer(cache.New(serveCtx, nil), false) + ss := NewStreamServer(cache.New(serveCtx, nil)) ss.serverForTest = server tsDirect := servertest.NewTCPServer(serveCtx, ss) defer tsDirect.Close() forwarderCtx := debug.WithInstance(baseCtx, "", "") - forwarder := NewForwarder("tcp", tsDirect.Addr, false) + forwarder := NewForwarder("tcp", tsDirect.Addr) tsForwarded := servertest.NewPipeServer(forwarderCtx, forwarder) defer tsForwarded.Close() @@ -183,10 +183,10 @@ func TestDebugInfoLifecycle(t *testing.T) { serverCtx := debug.WithInstance(baseCtx, "", "") cache := cache.New(serverCtx, nil) - ss := NewStreamServer(cache, false) + ss := NewStreamServer(cache) tsBackend := servertest.NewTCPServer(serverCtx, ss) - forwarder := NewForwarder("tcp", tsBackend.Addr, false) + forwarder := NewForwarder("tcp", tsBackend.Addr) tsForwarder := servertest.NewPipeServer(clientCtx, forwarder) ws, err := fake.NewWorkspace("gopls-lsprpc-test", []byte(exampleProgram)) diff --git a/internal/lsp/regtest/env.go b/internal/lsp/regtest/env.go index 8a8f1dd1fe..3971754a50 100644 --- a/internal/lsp/regtest/env.go +++ b/internal/lsp/regtest/env.go @@ -83,7 +83,7 @@ func (r *Runner) getTestServer() *servertest.TCPServer { if r.ts == nil { ctx := context.Background() ctx = debug.WithInstance(ctx, "", "") - ss := lsprpc.NewStreamServer(cache.New(ctx, nil), false) + ss := lsprpc.NewStreamServer(cache.New(ctx, nil)) r.ts = servertest.NewTCPServer(context.Background(), ss) } return r.ts @@ -189,7 +189,7 @@ func (r *Runner) RunInMode(modes EnvMode, t *testing.T, filedata string, test fu func (r *Runner) singletonEnv(ctx context.Context, t *testing.T) (servertest.Connector, func()) { ctx = debug.WithInstance(ctx, "", "") - ss := lsprpc.NewStreamServer(cache.New(ctx, nil), false) + ss := lsprpc.NewStreamServer(cache.New(ctx, nil)) ts := servertest.NewPipeServer(ctx, ss) cleanup := func() { ts.Close() @@ -204,7 +204,7 @@ func (r *Runner) sharedEnv(ctx context.Context, t *testing.T) (servertest.Connec func (r *Runner) forwardedEnv(ctx context.Context, t *testing.T) (servertest.Connector, func()) { ctx = debug.WithInstance(ctx, "", "") ts := r.getTestServer() - forwarder := lsprpc.NewForwarder("tcp", ts.Addr, false) + forwarder := lsprpc.NewForwarder("tcp", ts.Addr) ts2 := servertest.NewPipeServer(ctx, forwarder) cleanup := func() { ts2.Close() @@ -217,7 +217,7 @@ func (r *Runner) separateProcessEnv(ctx context.Context, t *testing.T) (serverte socket := r.getRemoteSocket(t) // TODO(rfindley): can we use the autostart behavior here, instead of // pre-starting the remote? - forwarder := lsprpc.NewForwarder("unix", socket, false) + forwarder := lsprpc.NewForwarder("unix", socket) ts2 := servertest.NewPipeServer(ctx, forwarder) cleanup := func() { ts2.Close()