diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index b08af302e..1609e9755 100644 --- a/CHANGELOG_PENDING.md +++ b/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) diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go index a66e61933..035793d93 100644 --- a/rpc/jsonrpc/server/http_json_handler.go +++ b/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...) } } diff --git a/rpc/jsonrpc/server/http_server.go b/rpc/jsonrpc/server/http_server.go index a9a5ca0b8..c0c2a834f 100644 --- a/rpc/jsonrpc/server/http_server.go +++ b/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) } } diff --git a/rpc/jsonrpc/server/http_server_test.go b/rpc/jsonrpc/server/http_server_test.go index 7c114e62a..9fd45c1e4 100644 --- a/rpc/jsonrpc/server/http_server_test.go +++ b/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)) } diff --git a/rpc/jsonrpc/server/http_uri_handler.go b/rpc/jsonrpc/server/http_uri_handler.go index fcdfc707a..808da16e2 100644 --- a/rpc/jsonrpc/server/http_uri_handler.go +++ b/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) -} diff --git a/rpc/jsonrpc/server/ws_handler.go b/rpc/jsonrpc/server/ws_handler.go index 845180d62..818e66512 100644 --- a/rpc/jsonrpc/server/ws_handler.go +++ b/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 } } diff --git a/test/app/kvstore_test.sh b/test/app/kvstore_test.sh index 034e28878..305a2926c 100755 --- a/test/app/kvstore_test.sh +++ b/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