rpc: rework how responses are written back via HTTP (#7575)

Add writeRPCResponse and writeHTTPResponse helpers, that handle the way RPC
responses are written to HTTP replies. These replace the exported helpers.

Visible effects:

- JSON results are now marshaled without indentation.
- HTTP status codes are now normalized.
- Cache control headers are no longer set.

Details:

- When writing a response to a URL (GET) request, do not marshal the whole
  JSON-RPC object into the body, only encode the result or the error object.
  This is a user-visible change.

- Do not change the HTTP status code for RPC errors. The RPC error already
  reports what went wrong, the HTTP status should only report problems with the
  HTTP transaction itself. This is a user-visible change.

- Encode JSON without indentation in POST response bodies. This is mainly cosmetic
  but saves quite a bit of response data. Indent is still applied to GET responses to make
  life easier for code examples.

- Remove an obsolete TODO about reporting an HTTP error on websocket upgrade.
  Nothing needed to change; the upgrader already reports an error.

- Report an HTTP error when starting the server loop fails.

- Improve logging for encoding errors.

- Log less aggressively.

CHANGELOG_PENDING.md

@@ -12,7 +12,8 @@ Special thanks to external contributors on this release:
 
 - CLI/RPC/Config
 
-  - [rpc] Remove the deprecated gRPC interface to the RPC service. (@creachadair)
+  - [rpc] \#7575 Rework how RPC responses are written back via HTTP. (@creachadair)
+  - [rpc] \#7121 Remove the deprecated gRPC interface to the RPC service. (@creachadair)
   - [blocksync] \#7159 Remove support for disabling blocksync in any circumstance. (@tychoish)
   - [mempool] \#7171 Remove legacy mempool implementation. (@tychoish)
 

rpc/jsonrpc/server/http_json_handler.go

@@ -21,22 +21,18 @@ import (
 // jsonrpc calls grab the given method's function info and runs reflect.Call
 func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.HandlerFunc {
 	return func(w http.ResponseWriter, hreq *http.Request) {
-		fail := func(res rpctypes.RPCResponse) {
-			if err := WriteRPCResponseHTTPError(w, res); err != nil {
-				logger.Error("Failed writing error response", "res", res, "err", err)
-			}
-		}
-
 		// For POST requests, reject a non-root URL path. This should not happen
 		// in the standard configuration, since the wrapper checks the path.
 		if hreq.URL.Path != "/" {
-			fail(rpctypes.RPCInvalidRequestError(nil, fmt.Errorf("invalid path: %q", hreq.URL.Path)))
+			writeRPCResponse(w, logger, rpctypes.RPCInvalidRequestError(
+				nil, fmt.Errorf("invalid path: %q", hreq.URL.Path)))
 			return
 		}
 
 		b, err := io.ReadAll(hreq.Body)
 		if err != nil {
-			fail(rpctypes.RPCInvalidRequestError(nil, fmt.Errorf("reading request body: %w", err)))
+			writeRPCResponse(w, logger, rpctypes.RPCInvalidRequestError(
+				nil, fmt.Errorf("reading request body: %w", err)))
 			return
 		}
 
@@ -49,7 +45,7 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 
 		requests, err := parseRequests(b)
 		if err != nil {
-			fail(rpctypes.RPCParseError(fmt.Errorf("decoding request: %w", err)))
+			writeRPCResponse(w, logger, rpctypes.RPCParseError(fmt.Errorf("decoding request: %w", err)))
 			return
 		}
 
@@ -98,11 +94,10 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 			}
 		}
 
-		if len(responses) > 0 {
-			if wErr := WriteRPCResponseHTTP(w, responses...); wErr != nil {
-				logger.Error("failed to write responses", "err", wErr)
-			}
+		if len(responses) == 0 {
+			return
 		}
+		writeRPCResponse(w, logger, responses...)
 	}
 }
 

rpc/jsonrpc/server/http_server.go

@@ -124,67 +124,58 @@ func ServeTLS(
 	return nil
 }
 
-// WriteRPCResponseHTTPError marshals res as JSON (with indent) and writes it
-// to w.
-//
-// Maps JSON RPC error codes to HTTP Status codes as follows:
-//
-// HTTP Status	code	message
-// 500	-32700	Parse error.
-// 400	-32600	Invalid Request.
-// 404	-32601	Method not found.
-// 500	-32602	Invalid params.
-// 500	-32603	Internal error.
-// 500	-32099..-32000	Server error.
+// writeInternalError writes an internal server error (500) to w with the text
+// of err in the body. This is a fallback used when a handler is unable to
+// write the expected response.
+func writeInternalError(w http.ResponseWriter, err error) {
+	w.Header().Set("Content-Type", "text/plain")
+	w.WriteHeader(http.StatusInternalServerError)
+	fmt.Fprintln(w, err.Error())
+}
+
+// writeHTTPResponse writes a JSON-RPC response to w. If rsp encodes an error,
+// the response body is its error object; otherwise its responses is the result.
 //
-// source: https://www.jsonrpc.org/historical/json-rpc-over-http.html
-func WriteRPCResponseHTTPError(
-	w http.ResponseWriter,
-	res rpctypes.RPCResponse,
-) error {
-	if res.Error == nil {
-		panic("tried to write http error response without RPC error")
+// Unless there is an error encoding the response, the status is 200 OK.
+func writeHTTPResponse(w http.ResponseWriter, log log.Logger, rsp rpctypes.RPCResponse) {
+	var body []byte
+	var err error
+	if rsp.Error != nil {
+		body, err = json.Marshal(rsp.Error)
+	} else {
+		body = rsp.Result
 	}
-
-	jsonBytes, err := json.MarshalIndent(res, "", "  ")
 	if err != nil {
-		return fmt.Errorf("json marshal: %w", err)
-	}
-
-	var httpCode int
-	switch res.Error.Code {
-	case -32600:
-		httpCode = http.StatusBadRequest
-	case -32601:
-		httpCode = http.StatusNotFound
-	default:
-		httpCode = http.StatusInternalServerError
+		log.Error("Error encoding RPC response: %w", err)
+		writeInternalError(w, err)
+		return
 	}
-
 	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(httpCode)
-	_, err = w.Write(jsonBytes)
-	return err
+	w.WriteHeader(http.StatusOK)
+	_, _ = w.Write(body)
 }
 
-// WriteRPCResponseHTTP marshals res as JSON (with indent) and writes it to w.
-// If the rpc response can be cached, add cache-control to the response header.
-func WriteRPCResponseHTTP(w http.ResponseWriter, res ...rpctypes.RPCResponse) error {
-	var v interface{}
-	if len(res) == 1 {
-		v = res[0]
+// writeRPCResponse writes one or more JSON-RPC responses to w. A single
+// response is encoded as an object, otherwise the response is sent as a batch
+// (array) of response objects.
+//
+// Unless there is an error encoding the responses, the status is 200 OK.
+func writeRPCResponse(w http.ResponseWriter, log log.Logger, rsps ...rpctypes.RPCResponse) {
+	var body []byte
+	var err error
+	if len(rsps) == 1 {
+		body, err = json.Marshal(rsps[0])
 	} else {
-		v = res
+		body, err = json.Marshal(rsps)
 	}
-
-	jsonBytes, err := json.MarshalIndent(v, "", "  ")
 	if err != nil {
-		return fmt.Errorf("json marshal: %w", err)
+		log.Error("Error encoding RPC response: %w", err)
+		writeInternalError(w, err)
+		return
 	}
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(http.StatusOK)
-	_, err = w.Write(jsonBytes)
-	return err
+	_, _ = w.Write(body)
 }
 
 //-----------------------------------------------------------------------------
@@ -222,9 +213,7 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler
 
 				// If RPCResponse
 				if res, ok := e.(rpctypes.RPCResponse); ok {
-					if wErr := WriteRPCResponseHTTP(rww, res); wErr != nil {
-						logger.Error("failed to write response", "res", res, "err", wErr)
-					}
+					writeRPCResponse(rww, logger, res)
 				} else {
 					// Panics can contain anything, attempt to normalize it as an error.
 					var err error
@@ -238,12 +227,8 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler
 					default:
 					}
 
-					logger.Error("panic in RPC HTTP handler", "err", e, "stack", string(debug.Stack()))
-
-					res := rpctypes.RPCInternalError(rpctypes.JSONRPCIntID(-1), err)
-					if wErr := WriteRPCResponseHTTPError(rww, res); wErr != nil {
-						logger.Error("failed to write response", "res", res, "err", wErr)
-					}
+					logger.Error("Panic in RPC HTTP handler", "err", e, "stack", string(debug.Stack()))
+					writeInternalError(rww, err)
 				}
 			}
 

rpc/jsonrpc/server/http_server_test.go

@@ -117,13 +117,14 @@ func TestServeTLS(t *testing.T) {
 	assert.Equal(t, []byte("some body"), body)
 }
 
-func TestWriteRPCResponseHTTP(t *testing.T) {
+func TestWriteRPCResponse(t *testing.T) {
 	id := rpctypes.JSONRPCIntID(-1)
 
 	// one argument
 	w := httptest.NewRecorder()
-	err := WriteRPCResponseHTTP(w, rpctypes.NewRPCSuccessResponse(id, &sampleResult{"hello"}))
-	require.NoError(t, err)
+	logger := log.NewTestingLogger(t)
+	writeRPCResponse(w, logger,
+		rpctypes.NewRPCSuccessResponse(id, &sampleResult{"hello"}))
 	resp := w.Result()
 	body, err := io.ReadAll(resp.Body)
 	_ = resp.Body.Close()
@@ -131,21 +132,14 @@ func TestWriteRPCResponseHTTP(t *testing.T) {
 	assert.Equal(t, 200, resp.StatusCode)
 	assert.Equal(t, "application/json", resp.Header.Get("Content-Type"))
 	assert.Equal(t, "", resp.Header.Get("Cache-control"))
-	assert.Equal(t, `{
-  "jsonrpc": "2.0",
-  "id": -1,
-  "result": {
-    "value": "hello"
-  }
-}`, string(body))
+	assert.Equal(t, `{"jsonrpc":"2.0","id":-1,"result":{"value":"hello"}}`, string(body))
 
 	// multiple arguments
 	w = httptest.NewRecorder()
-	err = WriteRPCResponseHTTP(w,
+	writeRPCResponse(w, logger,
 		rpctypes.NewRPCSuccessResponse(id, &sampleResult{"hello"}),
 		rpctypes.NewRPCSuccessResponse(id, &sampleResult{"world"}),
 	)
-	require.NoError(t, err)
 	resp = w.Result()
 	body, err = io.ReadAll(resp.Body)
 	_ = resp.Body.Close()
@@ -153,41 +147,20 @@ func TestWriteRPCResponseHTTP(t *testing.T) {
 
 	assert.Equal(t, 200, resp.StatusCode)
 	assert.Equal(t, "application/json", resp.Header.Get("Content-Type"))
-	assert.Equal(t, `[
-  {
-    "jsonrpc": "2.0",
-    "id": -1,
-    "result": {
-      "value": "hello"
-    }
-  },
-  {
-    "jsonrpc": "2.0",
-    "id": -1,
-    "result": {
-      "value": "world"
-    }
-  }
-]`, string(body))
+	assert.Equal(t, `[{"jsonrpc":"2.0","id":-1,"result":{"value":"hello"}},`+
+		`{"jsonrpc":"2.0","id":-1,"result":{"value":"world"}}]`, string(body))
 }
 
-func TestWriteRPCResponseHTTPError(t *testing.T) {
+func TestWriteHTTPResponse(t *testing.T) {
 	w := httptest.NewRecorder()
-	err := WriteRPCResponseHTTPError(w, rpctypes.RPCInternalError(rpctypes.JSONRPCIntID(-1), errors.New("foo")))
-	require.NoError(t, err)
+	logger := log.NewTestingLogger(t)
+	writeHTTPResponse(w, logger,
+		rpctypes.RPCInternalError(rpctypes.JSONRPCIntID(-1), errors.New("foo")))
 	resp := w.Result()
 	body, err := io.ReadAll(resp.Body)
 	_ = resp.Body.Close()
 	require.NoError(t, err)
-	assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
 	assert.Equal(t, "application/json", resp.Header.Get("Content-Type"))
-	assert.Equal(t, `{
-  "jsonrpc": "2.0",
-  "id": -1,
-  "error": {
-    "code": -32603,
-    "message": "Internal error",
-    "data": "foo"
-  }
-}`, string(body))
+	assert.Equal(t, `{"code":-32603,"message":"Internal error","data":"foo"}`, string(body))
 }

rpc/jsonrpc/server/http_uri_handler.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"net/http/httputil"
 	"reflect"
 	"regexp"
 	"strings"
@@ -26,30 +25,24 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit
 	dummyID := rpctypes.JSONRPCIntID(-1) // URIClientRequestID
 
 	// Exception for websocket endpoints
+	//
+	// TODO(creachadair): Rather than reporting errors for these, we should
+	// remove them from the routing list entirely on this endpoint.
 	if rpcFunc.ws {
 		return func(w http.ResponseWriter, r *http.Request) {
-			res := rpctypes.RPCMethodNotFoundError(dummyID)
-			if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil {
-				logger.Error("failed to write response", "res", res, "err", wErr)
-			}
+			w.WriteHeader(http.StatusNotFound)
 		}
 	}
 
 	// All other endpoints
 	return func(w http.ResponseWriter, r *http.Request) {
-		logger.Debug("HTTP HANDLER", "req", dumpHTTPRequest(r))
-
 		ctx := rpctypes.WithCallInfo(r.Context(), &rpctypes.CallInfo{HTTPRequest: r})
 		args := []reflect.Value{reflect.ValueOf(ctx)}
 
 		fnArgs, err := httpParamsToArgs(rpcFunc, r)
 		if err != nil {
-			res := rpctypes.RPCInvalidParamsError(dummyID,
-				fmt.Errorf("error converting http params to arguments: %w", err),
-			)
-			if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil {
-				logger.Error("failed to write response", "res", res, "err", wErr)
-			}
+			writeHTTPResponse(w, logger, rpctypes.RPCInvalidParamsError(
+				dummyID, fmt.Errorf("error converting http params to arguments: %w", err)))
 			return
 		}
 		args = append(args, fnArgs...)
@@ -61,36 +54,23 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit
 		switch e := err.(type) {
 		// if no error then return a success response
 		case nil:
-			res := rpctypes.NewRPCSuccessResponse(dummyID, result)
-			if wErr := WriteRPCResponseHTTP(w, res); wErr != nil {
-				logger.Error("failed to write response", "res", res, "err", wErr)
-			}
+			writeHTTPResponse(w, logger, rpctypes.NewRPCSuccessResponse(dummyID, result))
 
 		// if this already of type RPC error then forward that error.
 		case *rpctypes.RPCError:
-			res := rpctypes.NewRPCErrorResponse(dummyID, e.Code, e.Message, e.Data)
-			if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil {
-				logger.Error("failed to write response", "res", res, "err", wErr)
-			}
+			writeHTTPResponse(w, logger, rpctypes.NewRPCErrorResponse(dummyID, e.Code, e.Message, e.Data))
 
 		default: // we need to unwrap the error and parse it accordingly
-			var res rpctypes.RPCResponse
-
 			switch errors.Unwrap(err) {
 			case coretypes.ErrZeroOrNegativeHeight,
 				coretypes.ErrZeroOrNegativePerPage,
 				coretypes.ErrPageOutOfRange,
 				coretypes.ErrInvalidRequest:
-				res = rpctypes.RPCInvalidRequestError(dummyID, err)
+				writeHTTPResponse(w, logger, rpctypes.RPCInvalidRequestError(dummyID, err))
 			default: // ctypes.ErrHeightNotAvailable, ctypes.ErrHeightExceedsChainHead:
-				res = rpctypes.RPCInternalError(dummyID, err)
-			}
-
-			if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil {
-				logger.Error("failed to write response", "res", res, "err", wErr)
+				writeHTTPResponse(w, logger, rpctypes.RPCInternalError(dummyID, err))
 			}
 		}
-
 	}
 }
 
@@ -233,12 +213,3 @@ func getParam(r *http.Request, param string) string {
 	}
 	return s
 }
-
-func dumpHTTPRequest(r *http.Request) string {
-	d, e := httputil.DumpRequest(r, true)
-	if e != nil {
-		return e.Error()
-	}
-
-	return string(d)
-}

rpc/jsonrpc/server/ws_handler.go

@@ -71,7 +71,8 @@ func NewWebsocketManager(
 func (wm *WebsocketManager) WebsocketHandler(w http.ResponseWriter, r *http.Request) {
 	wsConn, err := wm.Upgrade(w, r, nil)
 	if err != nil {
-		// TODO - return http error
+		// The upgrader has already reported an HTTP error to the client, so we
+		// need only log it.
 		wm.logger.Error("Failed to upgrade connection", "err", err)
 		return
 	}
@@ -89,6 +90,7 @@ func (wm *WebsocketManager) WebsocketHandler(w http.ResponseWriter, r *http.Requ
 	// starting the conn is blocking
 	if err = conn.Start(r.Context()); err != nil {
 		wm.logger.Error("Failed to start connection", "err", err)
+		writeInternalError(w, err)
 		return
 	}
 
@@ -453,13 +455,13 @@ func (wsc *wsConnection) writeRoutine(ctx context.Context) {
 				return
 			}
 		case msg := <-wsc.writeChan:
-			jsonBytes, err := json.MarshalIndent(msg, "", "  ")
+			data, err := json.Marshal(msg)
 			if err != nil {
-				wsc.Logger.Error("Failed to marshal RPCResponse to JSON", "err", err)
+				wsc.Logger.Error("Failed to marshal RPCResponse to JSON", "msg", msg, "err", err)
 				continue
 			}
-			if err = wsc.writeMessageWithDeadline(websocket.TextMessage, jsonBytes); err != nil {
-				wsc.Logger.Error("Failed to write response", "err", err, "msg", msg)
+			if err = wsc.writeMessageWithDeadline(websocket.TextMessage, data); err != nil {
+				wsc.Logger.Error("Failed to write response", "msg", msg, "err", err)
 				return
 			}
 		}

test/app/kvstore_test.sh

@@ -57,7 +57,7 @@ echo "... testing query with /abci_query 2"
 
 # we should be able to look up the key
 RESPONSE=`curl -s "127.0.0.1:26657/abci_query?path=\"\"&data=$(toHex $KEY)&prove=false"`
-RESPONSE=`echo $RESPONSE | jq .result.response.log`
+RESPONSE=`echo $RESPONSE | jq .response.log`
 
 set +e
 A=`echo $RESPONSE | grep 'exists'`
@@ -70,7 +70,7 @@ set -e
 
 # we should not be able to look up the value
 RESPONSE=`curl -s "127.0.0.1:26657/abci_query?path=\"\"&data=$(toHex $VALUE)&prove=false"`
-RESPONSE=`echo $RESPONSE | jq .result.response.log`
+RESPONSE=`echo $RESPONSE | jq .response.log`
 set +e
 A=`echo $RESPONSE | grep 'exists'`
 if [[ $? == 0 ]]; then