internal/lsp/lsprpc: expose configuration for auto-started daemon

Three new flags are added to the serve command, and threaded through to
the LSP forwarder:
 -remote.listen.timeout: -listen.timeout for the auto-started daemon
 -remote.debug: -debug for the auto-started daemon
 -remote.logfile: -logfile for the auto-started daemon

As part of this change, no longer enable debugging the daemon by
default.

Notably none of this configuration affects serving, so modifying this
configuration has been chosen not to change the path to the automatic
daemon. In other words, this configuration has effect only for the
forwarder process that starts the daemon: all others will connect to the
daemon and inherit whatever configuration it had at startup. This should
be OK, because in the common case this configuration should be static
across all clients (e.g., many Vim sessions all sharing the same
.vimrc).

Exposing this configuration made the signature of lsprpc.NewForwarder
a bit hard to understand, so I decided to go ahead and switch to a
variadic options pattern for initializing both the Forwarder and
StreamServer, the latter just for consistency with the Forwarder.

Updates golang/go#34111

Change-Id: Iefb71e337befe08b23e451477d19fd57e69f36c6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222670
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

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

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
 }

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

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 != "" {

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)

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

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