internal/jsonrpc2: make it an error to fail to call Reply

It is now a programatic error to have a handler registered to a connection that
does not call reply for all messages, including notifications.
This normalizes the flow making the code easier to understand  and fixes a
couple of long standing hard to find bugs.

Change-Id: If41c39ece70e3bc64420abefac75ec647a8f8b37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226838
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

internal/jsonrpc2/handler.go

@@ -6,6 +6,7 @@ package jsonrpc2
 
 import (
 	"context"
+	"fmt"
 )
 
 // Handler is invoked to handle incoming requests.
@@ -65,8 +66,17 @@ func (d Direction) String() string {
 // standard method not found response.
 // This should normally be the final handler in a chain.
 func MethodNotFound(ctx context.Context, r *Request) error {
-	if !r.IsNotify() {
 	return r.Reply(ctx, nil, NewErrorf(CodeMethodNotFound, "method %q not found", r.Method))
-	}
+}
-	return nil
+
+// MustReply creates a Handler that panics if the wrapped handler does
+// not call Reply for every request that it is passed.
+func MustReply(handler Handler) Handler {
+	return func(ctx context.Context, req *Request) error {
+		err := handler(ctx, req)
+		if req.state < requestReplied {
+			panic(fmt.Errorf("request %q was never replied to", req.Method))
+		}
+		return err
+	}
 }

internal/jsonrpc2/jsonrpc2.go

@@ -296,6 +296,7 @@ type combined struct {
 // It must be called exactly once for each Conn.
 // It returns only when the reader is closed or there is an error in the stream.
 func (c *Conn) Run(runCtx context.Context, handler Handler) error {
+	handler = MustReply(handler)
 	// we need to make the next request "lock" in an unlocked state to allow
 	// the first incoming request to proceed. All later requests are unlocked
 	// by the preceding request going to parallel mode.
@@ -353,9 +354,6 @@ func (c *Conn) Run(runCtx context.Context, handler Handler) error {
 				req.state = requestSerial
 				defer func() {
 					setHandling(req, false)
-					if req.state < requestReplied {
-						req.Reply(reqCtx, nil, nil)
-					}
 					done()
 					cancelReq()
 				}()

internal/lsp/lsprpc/lsprpc.go

@@ -418,10 +418,10 @@ func forwarderHandler(handler jsonrpc2.Handler) jsonrpc2.Handler {
 		// once that process exists.
 		if r.Method == "exit" {
 			ForwarderExitFunc(0)
-			// return nil here to consume the message: in
+			// reply nil here to consume the message: in
 			// tests, ForwarderExitFunc may be overridden to something that doesn't
 			// exit the process.
-			return nil
+			return r.Reply(ctx, nil, nil)
 		}
 		return handler(ctx, r)
 	}
@@ -505,10 +505,7 @@ func handshaker(client *debugClient, goplsPath string, handler jsonrpc2.Handler)
 				resp.DebugAddr = di.ListenedDebugAddress
 			}
 
-			if err := r.Reply(ctx, resp, nil); err != nil {
+			return r.Reply(ctx, resp, nil)
-				event.Error(ctx, "replying to handshake", err)
-			}
-			return nil
 		case sessionsMethod:
 			resp := ServerState{
 				GoplsPath:       goplsPath,
@@ -526,10 +523,7 @@ func handshaker(client *debugClient, goplsPath string, handler jsonrpc2.Handler)
 					})
 				}
 			}
-			if err := r.Reply(ctx, resp, nil); err != nil {
+			return r.Reply(ctx, resp, nil)
-				event.Error(ctx, "replying to sessions request", err)
-			}
-			return nil
 		}
 		return handler(ctx, r)
 	}

internal/lsp/protocol/protocol.go

@@ -69,7 +69,11 @@ func (Canceller) Cancel(ctx context.Context, conn *jsonrpc2.Conn, id jsonrpc2.ID
 
 func (Canceller) Deliver(ctx context.Context, r *jsonrpc2.Request, delivered bool) bool {
 	// Hide cancellations from downstream handlers.
-	return r.Method == "$/cancelRequest"
+	if r.Method != "$/cancelRequest" {
+		return false
+	}
+	r.Reply(ctx, nil, nil)
+	return true
 }
 
 func sendParseError(ctx context.Context, req *jsonrpc2.Request, err error) error {

internal/lsp/protocol/tsclient.go

@@ -42,31 +42,36 @@ func ClientHandler(client Client, handler jsonrpc2.Handler) jsonrpc2.Handler {
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return client.ShowMessage(ctx, &params)
+			err := client.ShowMessage(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "window/logMessage": // notif
 			var params LogMessageParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return client.LogMessage(ctx, &params)
+			err := client.LogMessage(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "telemetry/event": // notif
 			var params interface{}
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return client.Event(ctx, &params)
+			err := client.Event(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "textDocument/publishDiagnostics": // notif
 			var params PublishDiagnosticsParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return client.PublishDiagnostics(ctx, &params)
+			err := client.PublishDiagnostics(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "$/progress": // notif
 			var params ProgressParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return client.Progress(ctx, &params)
+			err := client.Progress(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "workspace/workspaceFolders": // req
 			if r.Params != nil {
 				return r.Reply(ctx, nil, jsonrpc2.NewErrorf(jsonrpc2.CodeInvalidParams, "Expected no params"))

internal/lsp/protocol/tsserver.go

@@ -80,19 +80,22 @@ func ServerHandler(server Server, handler jsonrpc2.Handler) jsonrpc2.Handler {
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.DidChangeWorkspaceFolders(ctx, &params)
+			err := server.DidChangeWorkspaceFolders(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "window/workDoneProgress/cancel": // notif
 			var params WorkDoneProgressCancelParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.WorkDoneProgressCancel(ctx, &params)
+			err := server.WorkDoneProgressCancel(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "initialized": // notif
 			var params InitializedParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.Initialized(ctx, &params)
+			err := server.Initialized(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "exit": // notif
 			return server.Exit(ctx)
 		case "workspace/didChangeConfiguration": // notif
@@ -100,55 +103,64 @@ func ServerHandler(server Server, handler jsonrpc2.Handler) jsonrpc2.Handler {
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.DidChangeConfiguration(ctx, &params)
+			err := server.DidChangeConfiguration(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "textDocument/didOpen": // notif
 			var params DidOpenTextDocumentParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.DidOpen(ctx, &params)
+			err := server.DidOpen(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "textDocument/didChange": // notif
 			var params DidChangeTextDocumentParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.DidChange(ctx, &params)
+			err := server.DidChange(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "textDocument/didClose": // notif
 			var params DidCloseTextDocumentParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.DidClose(ctx, &params)
+			err := server.DidClose(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "textDocument/didSave": // notif
 			var params DidSaveTextDocumentParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.DidSave(ctx, &params)
+			err := server.DidSave(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "textDocument/willSave": // notif
 			var params WillSaveTextDocumentParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.WillSave(ctx, &params)
+			err := server.WillSave(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "workspace/didChangeWatchedFiles": // notif
 			var params DidChangeWatchedFilesParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.DidChangeWatchedFiles(ctx, &params)
+			err := server.DidChangeWatchedFiles(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "$/setTraceNotification": // notif
 			var params SetTraceParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.SetTraceNotification(ctx, &params)
+			err := server.SetTraceNotification(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "$/logTraceNotification": // notif
 			var params LogTraceParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {
 				return sendParseError(ctx, r, err)
 			}
-			return server.LogTraceNotification(ctx, &params)
+			err := server.LogTraceNotification(ctx, &params)
+			return r.Reply(ctx, nil, err)
 		case "textDocument/implementation": // req
 			var params ImplementationParams
 			if err := json.Unmarshal(*r.Params, &params); err != nil {

internal/lsp/protocol/typescript/code.ts

@@ -926,7 +926,8 @@ function goNot(side: side, m: string) {
     if err := json.Unmarshal(*r.Params, &params); err != nil {
       return sendParseError(ctx, r, err)
     }
-    return ${side.name}.${nm}(ctx, &params)`
+    err:= ${side.name}.${nm}(ctx, &params)
+    return r.Reply(ctx, nil, err)`
   } else {
     case1 = `return ${side.name}.${nm}(ctx)`;
   }