langserver: Refactor logic for easier testing

Separate package for high-level errors and server controller
makes it easier to test the server and help avoid import cycles.

commands/serve_command.go

@@ -10,6 +10,7 @@ import (
 
 	lsctx "github.com/hashicorp/terraform-ls/internal/context"
 	"github.com/hashicorp/terraform-ls/langserver"
+	"github.com/hashicorp/terraform-ls/langserver/handlers"
 	"github.com/mitchellh/cli"
 )
 
@@ -35,14 +36,16 @@ func (c *ServeCommand) Run(args []string) int {
 		syscall.SIGINT, syscall.SIGTERM)
 	defer cancelFunc()
 
-	srv := langserver.NewLangServer(ctx, c.Logger)
+	hp := handlers.New()
+	srv := langserver.NewLangServer(ctx, hp)
+	srv.SetLogger(c.Logger)
 
 	if port != -1 {
 		srv.StartTCP(fmt.Sprintf("localhost:%d", port))
 		return 0
 	}
 
-	srv.Start(os.Stdin, os.Stdout)
+	srv.StartAndWait(os.Stdin, os.Stdout)
 
 	return 0
 }

internal/terraform/exec/exec.go

@@ -59,7 +59,7 @@ func (e *Executor) run(args ...string) ([]byte, error) {
 	}
 
 	var outBuf bytes.Buffer
-	var errBuf strings.Builder
+	var errBuf bytes.Buffer
 
 	path, err := exec.LookPath("terraform")
 	if err != nil {

langserver/errors/errors.go

@@ -0,0 +1,7 @@
+package errors
+
+import (
+	"github.com/creachadair/jrpc2/code"
+)
+
+const ServerNotInitialized code.Code = -32002

langserver/handlers.go → langserver/handlers/handlers.go

@@ -1,4 +1,4 @@
-package langserver
+package handlers
 
 import (
 	"context"
@@ -12,105 +12,116 @@ import (
 	lsctx "github.com/hashicorp/terraform-ls/internal/context"
 	"github.com/hashicorp/terraform-ls/internal/filesystem"
 	"github.com/hashicorp/terraform-ls/internal/terraform/exec"
-	"github.com/hashicorp/terraform-ls/langserver/handlers"
+	"github.com/hashicorp/terraform-ls/langserver/srvctl"
 	"github.com/sourcegraph/go-lsp"
 )
 
-type handlerMap struct {
+type handlerProvider struct {
 	logger *log.Logger
+	srvCtl srvctl.ServerController
+}
+
+func New() *handlerProvider {
+	return &handlerProvider{}
+}
 
-	srv         *server
-	srvStopFunc context.CancelFunc
+func (hp *handlerProvider) SetLogger(logger *log.Logger) {
+	hp.logger = logger
 }
 
-// Map builds out the jrpc2.Map according to the LSP protocol
-// and passes related dependencies to methods via context
-func (hm *handlerMap) Map() rpch.Map {
+// Handlers builds out the jrpc2.Map according to the LSP protocol
+// and passes related dependencies to handlers via context
+func (hp *handlerProvider) Handlers(ctl srvctl.ServerController) jrpc2.Assigner {
+	hp.srvCtl = ctl
 	fs := filesystem.NewFilesystem()
-	fs.SetLogger(hm.logger)
-	lh := handlers.LogHandler(hm.logger)
+	fs.SetLogger(hp.logger)
+	lh := LogHandler(hp.logger)
 	cc := &lsp.ClientCapabilities{}
 
 	m := map[string]rpch.Func{
 		"initialize": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			err := hm.srv.Initialize(req)
+			err := hp.srvCtl.Initialize(req)
 			if err != nil {
 				return nil, err
 			}
 			ctx = lsctx.WithFilesystem(fs, ctx)
 			ctx = lsctx.WithClientCapabilitiesSetter(cc, ctx)
 
-			return handle(ctx, req, handlers.Initialize)
+			return handle(ctx, req, Initialize)
 		},
 		"initialized": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			err := hm.srv.ConfirmInitialization(req)
+			err := hp.srvCtl.ConfirmInitialization(req)
 			if err != nil {
 				return nil, err
 			}
 			ctx = lsctx.WithFilesystem(fs, ctx)
 
-			return handle(ctx, req, handlers.Initialized)
+			return handle(ctx, req, Initialized)
 		},
 		"textDocument/didChange": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			if !hm.srv.IsInitializationConfirmed() {
-				return nil, SrvNotInitializedErr(hm.srv.State())
+			err := hp.srvCtl.CheckInitializationIsConfirmed()
+			if err != nil {
+				return nil, err
 			}
 			ctx = lsctx.WithFilesystem(fs, ctx)
-			return handle(ctx, req, handlers.TextDocumentDidChange)
+			return handle(ctx, req, TextDocumentDidChange)
 		},
 		"textDocument/didOpen": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			if !hm.srv.IsInitializationConfirmed() {
-				return nil, SrvNotInitializedErr(hm.srv.State())
+			err := hp.srvCtl.CheckInitializationIsConfirmed()
+			if err != nil {
+				return nil, err
 			}
 			ctx = lsctx.WithFilesystem(fs, ctx)
-			return handle(ctx, req, handlers.TextDocumentDidOpen)
+			return handle(ctx, req, TextDocumentDidOpen)
 		},
 		"textDocument/didClose": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			if !hm.srv.IsInitializationConfirmed() {
-				return nil, SrvNotInitializedErr(hm.srv.State())
+			err := hp.srvCtl.CheckInitializationIsConfirmed()
+			if err != nil {
+				return nil, err
 			}
 			ctx = lsctx.WithFilesystem(fs, ctx)
-			return handle(ctx, req, handlers.TextDocumentDidClose)
+			return handle(ctx, req, TextDocumentDidClose)
 		},
 		"textDocument/completion": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			if !hm.srv.IsInitializationConfirmed() {
-				return nil, SrvNotInitializedErr(hm.srv.State())
+			err := hp.srvCtl.CheckInitializationIsConfirmed()
+			if err != nil {
+				return nil, err
 			}
 
 			ctx = lsctx.WithFilesystem(fs, ctx) // TODO: Read-only FS
 			ctx = lsctx.WithClientCapabilities(cc, ctx)
 
 			tf := exec.NewExecutor(ctx)
-			tf.SetLogger(hm.logger)
+			tf.SetLogger(hp.logger)
 			ctx = lsctx.WithTerraformExecutor(tf, ctx)
 
 			return handle(ctx, req, lh.TextDocumentComplete)
 		},
 		"shutdown": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			err := hm.srv.Shutdown(req)
+			err := hp.srvCtl.Shutdown(req)
 			if err != nil {
 				return nil, err
 			}
 			ctx = lsctx.WithFilesystem(fs, ctx)
 			// TODO: Exit the process after a timeout if `exit` method is not called
 			// to prevent zombie processes (?)
-			return handle(ctx, req, handlers.Shutdown)
+			return handle(ctx, req, Shutdown)
 		},
 		"exit": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			if !hm.srv.IsDown() && !hm.srv.IsPrepared() {
-				return nil, fmt.Errorf("Cannot exit as server is %s", hm.srv.State())
+			err := hp.srvCtl.Exit()
+			if err != nil {
+				return nil, err
 			}
 
-			hm.srvStopFunc()
-
-			return handle(ctx, req, handlers.Shutdown)
+			return handle(ctx, req, Shutdown)
 		},
 		"$/cancelRequest": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
-			if !hm.srv.IsInitializationConfirmed() {
-				return nil, SrvNotInitializedErr(hm.srv.State())
+			err := hp.srvCtl.CheckInitializationIsConfirmed()
+			if err != nil {
+				return nil, err
 			}
 
-			return handle(ctx, req, handlers.CancelRequest)
+			return handle(ctx, req, CancelRequest)
 		},
 	}
 

langserver/langserver.go

@@ -4,57 +4,65 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"log"
 	"net"
 
 	"github.com/creachadair/jrpc2"
 	"github.com/creachadair/jrpc2/channel"
 	rpcServer "github.com/creachadair/jrpc2/server"
+	"github.com/hashicorp/terraform-ls/langserver/srvctl"
 )
 
 type langServer struct {
 	ctx        context.Context
-	assigner   jrpc2.Assigner
+	hp         srvctl.HandlerProvider
 	logger     *log.Logger
 	srvOptions *jrpc2.ServerOptions
 
-	server   *server
+	srvCtl   srvctl.ServerController
 	stopFunc context.CancelFunc
 }
 
-func NewLangServer(srvCtx context.Context, logger *log.Logger) *langServer {
-	srv := newServer()
-
+func NewLangServer(srvCtx context.Context, hp srvctl.HandlerProvider) *langServer {
 	opts := &jrpc2.ServerOptions{
 		AllowPush: true,
-		Logger:    logger,
-		RPCLog:    &rpcLogger{logger},
 	}
 
 	srvCtx, stopFunc := context.WithCancel(srvCtx)
-	hm := &handlerMap{logger: logger, srv: srv, srvStopFunc: stopFunc}
 
 	return &langServer{
 		ctx:        srvCtx,
-		assigner:   hm.Map(),
-		logger:     logger,
+		hp:         hp,
+		logger:     log.New(ioutil.Discard, "", 0),
 		srvOptions: opts,
-		server:     srv,
+		srvCtl:     srvctl.NewServerController(),
 		stopFunc:   stopFunc,
 	}
 }
 
-func (ls *langServer) Start(reader io.Reader, writer io.WriteCloser) {
-	err := ls.server.Prepare()
+func (ls *langServer) SetLogger(logger *log.Logger) {
+	ls.srvOptions.Logger = logger
+	ls.srvOptions.RPCLog = &rpcLogger{logger}
+	ls.hp.SetLogger(logger)
+	ls.logger = logger
+}
+
+func (ls *langServer) start(reader io.Reader, writer io.WriteCloser) *jrpc2.Server {
+	err := ls.srvCtl.Prepare()
 	if err != nil {
 		ls.logger.Printf("Unable to prepare server: %s", err)
 		ls.stopFunc()
+		return nil
 	}
 
 	ch := channel.LSP(reader, writer)
 
-	srv := jrpc2.NewServer(ls.assigner, ls.srvOptions).Start(ch)
+	return jrpc2.NewServer(ls.hp.Handlers(ls.srvCtl), ls.srvOptions).Start(ch)
+}
 
+func (ls *langServer) StartAndWait(reader io.Reader, writer io.WriteCloser) {
+	srv := ls.start(reader, writer)
 	go func() {
 		ls.logger.Println("Starting server ...")
 		err := srv.Wait()
@@ -73,7 +81,7 @@ func (ls *langServer) Start(reader io.Reader, writer io.WriteCloser) {
 }
 
 func (ls *langServer) StartTCP(address string) error {
-	err := ls.server.Prepare()
+	err := ls.srvCtl.Prepare()
 	if err != nil {
 		ls.logger.Printf("Unable to prepare server: %s", err)
 		ls.stopFunc()
@@ -88,7 +96,7 @@ func (ls *langServer) StartTCP(address string) error {
 
 	go func() {
 		ls.logger.Println("Starting loop server ...")
-		err = rpcServer.Loop(lst, ls.assigner, &rpcServer.LoopOptions{
+		err = rpcServer.Loop(lst, ls.hp.Handlers(ls.srvCtl), &rpcServer.LoopOptions{
 			Framing:       channel.LSP,
 			ServerOptions: ls.srvOptions,
 		})

langserver/srvctl/errors.go

@@ -0,0 +1,43 @@
+package srvctl
+
+import (
+	"fmt"
+
+	"github.com/creachadair/jrpc2/code"
+	"github.com/hashicorp/terraform-ls/langserver/errors"
+)
+
+type unexpectedSrvState struct {
+	ExpectedState serverState
+	CurrentState  serverState
+}
+
+func (e *unexpectedSrvState) Error() string {
+	return fmt.Sprintf("server is not %s, current state: %s",
+		e.ExpectedState, e.CurrentState)
+}
+
+func srvNotInitializedErr(state serverState) error {
+	uss := &unexpectedSrvState{
+		ExpectedState: stateInitializedConfirmed,
+		CurrentState:  state,
+	}
+	if state < stateInitializedConfirmed {
+		return fmt.Errorf("%w: %s", errors.ServerNotInitialized.Err(), uss)
+	}
+	if state == stateDown {
+		return fmt.Errorf("%w: %s", code.InvalidRequest.Err(), uss)
+	}
+
+	return uss
+}
+
+func srvAlreadyInitializedErr(reqID string) error {
+	return fmt.Errorf("%w: Server was already initialized via request ID %s",
+		code.SystemError.Err(), reqID)
+}
+
+func srvAlreadyDownErr(reqID string) error {
+	return fmt.Errorf("%w: server was already shut down via request %s",
+		code.InvalidRequest.Err(), reqID)
+}