rpc/jsonrpc/server: return an error in WriteRPCResponseHTTP(Error) (b…

…p #6204) (#6230) * rpc/jsonrpc/server: return an error in WriteRPCResponseHTTP(Error) (#6204) instead of panicking Closes #5529 (cherry picked from commit 00b9524) # Conflicts: # CHANGELOG_PENDING.md # rpc/jsonrpc/server/http_json_handler.go # rpc/jsonrpc/server/http_server.go # rpc/jsonrpc/server/http_server_test.go # rpc/jsonrpc/server/http_uri_handler.go * resolve conflicts * fix linting * fix conflict Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Marko Baricevic <marbar3778@yahoo.com>
KYVENetwork · Mar 17, 2021 · 4e25703 · 4e25703
1 parent d004a58
commit 4e25703
Show file tree

Hide file tree

Showing 12 changed files with 76 additions and 66 deletions.
diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md
@@ -15,6 +15,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 - P2P Protocol
 
 - Go API
+    - [rpc/jsonrpc/server] \#6204 Modify `WriteRPCResponseHTTP(Error)` to return an error (@melekes)
 
 - Blockchain Protocol
 

diff --git a/blockchain/v2/reactor_test.go b/blockchain/v2/reactor_test.go
@@ -59,19 +59,22 @@ func (mp mockPeer) TrySend(byte, []byte) bool { return true }
 func (mp mockPeer) Set(string, interface{}) {}
 func (mp mockPeer) Get(string) interface{}  { return struct{}{} }
 
-//nolint:unused
+// nolint:unused // ignore
 type mockBlockStore struct {
 	blocks map[int64]*types.Block
 }
 
+// nolint:unused // ignore
 func (ml *mockBlockStore) Height() int64 {
 	return int64(len(ml.blocks))
 }
 
+// nolint:unused // ignore
 func (ml *mockBlockStore) LoadBlock(height int64) *types.Block {
 	return ml.blocks[height]
 }
 
+// nolint:unused // ignore
 func (ml *mockBlockStore) SaveBlock(block *types.Block, part *types.PartSet, commit *types.Commit) {
 	ml.blocks[block.Height] = block
 }

diff --git a/crypto/hash.go b/crypto/hash.go
@@ -6,6 +6,6 @@ import (
 
 func Sha256(bytes []byte) []byte {
 	hasher := sha256.New()
-	hasher.Write(bytes) //nolint:errcheck // ignore error
+	hasher.Write(bytes)
 	return hasher.Sum(nil)
 }
diff --git a/crypto/merkle/proof_value.go b/crypto/merkle/proof_value.go
@@ -80,13 +80,13 @@ func (op ValueOp) Run(args [][]byte) ([][]byte, error) {
 	}
 	value := args[0]
 	hasher := tmhash.New()
-	hasher.Write(value) //nolint: errcheck // does not error
+	hasher.Write(value)
 	vhash := hasher.Sum(nil)
 
 	bz := new(bytes.Buffer)
 	// Wrap <op.Key, vhash> to hash the KVPair.
-	encodeByteSlice(bz, op.key) //nolint: errcheck // does not error
-	encodeByteSlice(bz, vhash)  //nolint: errcheck // does not error
+	encodeByteSlice(bz, op.key) // nolint: errcheck // does not error
+	encodeByteSlice(bz, vhash)  // nolint: errcheck // does not error
 	kvhash := leafHash(bz.Bytes())
 
 	if !bytes.Equal(kvhash, op.Proof.LeafHash) {

diff --git a/mempool/clist_mempool_test.go b/mempool/clist_mempool_test.go
@@ -662,7 +662,7 @@ func newRemoteApp(
 }
 func checksumIt(data []byte) string {
 	h := sha256.New()
-	h.Write(data) //nolint: errcheck // ignore errcheck
+	h.Write(data)
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
 

diff --git a/p2p/conn/secret_connection.go b/p2p/conn/secret_connection.go
@@ -275,7 +275,6 @@ func (sc *SecretConnection) Read(data []byte) (n int, err error) {
 }
 
 // Implements net.Conn
-// nolint
 func (sc *SecretConnection) Close() error                  { return sc.conn.Close() }
 func (sc *SecretConnection) LocalAddr() net.Addr           { return sc.conn.(net.Conn).LocalAddr() }
 func (sc *SecretConnection) RemoteAddr() net.Addr          { return sc.conn.(net.Conn).RemoteAddr() }

diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go
@@ -941,6 +941,6 @@ func (a *addrBook) hash(b []byte) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
-	hasher.Write(b) //nolint:errcheck // ignore error
+	hasher.Write(b)
 	return hasher.Sum(nil), nil
 }
diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go
@@ -21,14 +21,12 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 	return func(w http.ResponseWriter, r *http.Request) {
 		b, err := ioutil.ReadAll(r.Body)
 		if err != nil {
-			WriteRPCResponseHTTPError(
-				w,
-				http.StatusBadRequest,
-				types.RPCInvalidRequestError(
-					nil,
-					fmt.Errorf("error reading request body: %w", err),
-				),
+			res := types.RPCInvalidRequestError(nil,
+				fmt.Errorf("error reading request body: %w", err),
 			)
+			if wErr := WriteRPCResponseHTTPError(w, http.StatusBadRequest, res); wErr != nil {
+				logger.Error("failed to write response", "res", res, "err", wErr)
+			}
 			return
 		}
 
@@ -48,13 +46,10 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 			// next, try to unmarshal as a single request
 			var request types.RPCRequest
 			if err := json.Unmarshal(b, &request); err != nil {
-				WriteRPCResponseHTTPError(
-					w,
-					http.StatusInternalServerError,
-					types.RPCParseError(
-						fmt.Errorf("error unmarshalling request: %w", err),
-					),
-				)
+				res := types.RPCParseError(fmt.Errorf("error unmarshaling request: %w", err))
+				if wErr := WriteRPCResponseHTTPError(w, http.StatusInternalServerError, res); wErr != nil {
+					logger.Error("failed to write response", "res", res, "err", wErr)
+				}
 				return
 			}
 			requests = []types.RPCRequest{request}
@@ -108,7 +103,9 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han
 		}
 
 		if len(responses) > 0 {
-			WriteRPCResponseHTTP(w, responses...)
+			if wErr := WriteRPCResponseHTTP(w, responses...); wErr != nil {
+				logger.Error("failed to write responses", "res", responses, "err", wErr)
+			}
 		}
 	}
 }

diff --git a/rpc/jsonrpc/server/http_server.go b/rpc/jsonrpc/server/http_server.go
@@ -89,30 +89,32 @@ func ServeTLS(
 	return err
 }
 
-// WriteRPCResponseHTTPError marshals res as JSON and writes it to w.
+// WriteRPCResponseHTTPError marshals res as JSON (with indent) and writes it
+// to w.
 //
-// Panics if it can't Marshal res or write to w.
+// source: https://www.jsonrpc.org/historical/json-rpc-over-http.html
 func WriteRPCResponseHTTPError(
 	w http.ResponseWriter,
 	httpCode int,
 	res types.RPCResponse,
-) {
+) error {
+	if res.Error == nil {
+		panic("tried to write http error response without RPC error")
+	}
+
 	jsonBytes, err := json.MarshalIndent(res, "", "  ")
 	if err != nil {
-		panic(err)
+		return fmt.Errorf("json marshal: %w", err)
 	}
 
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(httpCode)
-	if _, err := w.Write(jsonBytes); err != nil {
-		panic(err)
-	}
+	_, err = w.Write(jsonBytes)
+	return err
 }
 
-// WriteRPCResponseHTTP marshals res as JSON and writes it to w.
-//
-// Panics if it can't Marshal res or write to w.
-func WriteRPCResponseHTTP(w http.ResponseWriter, res ...types.RPCResponse) {
+// WriteRPCResponseHTTP marshals res as JSON (with indent) and writes it to w.
+func WriteRPCResponseHTTP(w http.ResponseWriter, res ...types.RPCResponse) error {
 	var v interface{}
 	if len(res) == 1 {
 		v = res[0]
@@ -122,13 +124,12 @@ func WriteRPCResponseHTTP(w http.ResponseWriter, res ...types.RPCResponse) {
 
 	jsonBytes, err := json.MarshalIndent(v, "", "  ")
 	if err != nil {
-		panic(err)
+		return fmt.Errorf("json marshal: %w", err)
 	}
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(200)
-	if _, err := w.Write(jsonBytes); err != nil {
-		panic(err)
-	}
+	_, err = w.Write(jsonBytes)
+	return err
 }
 
 //-----------------------------------------------------------------------------
@@ -166,7 +167,9 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler
 
 				// If RPCResponse
 				if res, ok := e.(types.RPCResponse); ok {
-					WriteRPCResponseHTTP(rww, res)
+					if wErr := WriteRPCResponseHTTP(rww, res); wErr != nil {
+						logger.Error("failed to write response", "res", res, "err", wErr)
+					}
 				} else {
 					// Panics can contain anything, attempt to normalize it as an error.
 					var err error
@@ -180,15 +183,12 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler
 					default:
 					}
 
-					logger.Error(
-						"Panic in RPC HTTP handler", "err", e, "stack",
-						string(debug.Stack()),
-					)
-					WriteRPCResponseHTTPError(
-						rww,
-						http.StatusInternalServerError,
-						types.RPCInternalError(types.JSONRPCIntID(-1), err),
-					)
+					logger.Error("panic in RPC HTTP handler", "err", e, "stack", string(debug.Stack()))
+
+					res := types.RPCInternalError(types.JSONRPCIntID(-1), err)
+					if wErr := WriteRPCResponseHTTPError(rww, http.StatusInternalServerError, res); wErr != nil {
+						logger.Error("failed to write response", "res", res, "err", wErr)
+					}
 				}
 			}
 

diff --git a/rpc/jsonrpc/server/http_server_test.go b/rpc/jsonrpc/server/http_server_test.go
@@ -112,7 +112,8 @@ func TestWriteRPCResponseHTTP(t *testing.T) {
 
 	// one argument
 	w := httptest.NewRecorder()
-	WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(id, &sampleResult{"hello"}))
+	err := WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(id, &sampleResult{"hello"}))
+	require.NoError(t, err)
 	resp := w.Result()
 	body, err := ioutil.ReadAll(resp.Body)
 	_ = resp.Body.Close()
@@ -129,9 +130,10 @@ func TestWriteRPCResponseHTTP(t *testing.T) {
 
 	// multiple arguments
 	w = httptest.NewRecorder()
-	WriteRPCResponseHTTP(w,
+	err = WriteRPCResponseHTTP(w,
 		types.NewRPCSuccessResponse(id, &sampleResult{"hello"}),
 		types.NewRPCSuccessResponse(id, &sampleResult{"world"}))
+	require.NoError(t, err)
 	resp = w.Result()
 	body, err = ioutil.ReadAll(resp.Body)
 	_ = resp.Body.Close()
@@ -159,9 +161,11 @@ func TestWriteRPCResponseHTTP(t *testing.T) {
 
 func TestWriteRPCResponseHTTPError(t *testing.T) {
 	w := httptest.NewRecorder()
-	WriteRPCResponseHTTPError(w,
+	err := WriteRPCResponseHTTPError(
+		w,
 		http.StatusInternalServerError,
 		types.RPCInternalError(types.JSONRPCIntID(-1), errors.New("foo")))
+	require.NoError(t, err)
 	resp := w.Result()
 	body, err := ioutil.ReadAll(resp.Body)
 	_ = resp.Body.Close()

diff --git a/rpc/jsonrpc/server/http_uri_handler.go b/rpc/jsonrpc/server/http_uri_handler.go
@@ -25,8 +25,10 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit
 	// Exception for websocket endpoints
 	if rpcFunc.ws {
 		return func(w http.ResponseWriter, r *http.Request) {
-			WriteRPCResponseHTTPError(w, http.StatusNotFound,
-				types.RPCMethodNotFoundError(dummyID))
+			res := types.RPCMethodNotFoundError(dummyID)
+			if wErr := WriteRPCResponseHTTPError(w, http.StatusNotFound, res); wErr != nil {
+				logger.Error("failed to write response", "res", res, "err", wErr)
+			}
 		}
 	}
 
@@ -39,14 +41,12 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit
 
 		fnArgs, err := httpParamsToArgs(rpcFunc, r)
 		if err != nil {
-			WriteRPCResponseHTTPError(
-				w,
-				http.StatusInternalServerError,
-				types.RPCInvalidParamsError(
-					dummyID,
-					fmt.Errorf("error converting http params to arguments: %w", err),
-				),
+			res := types.RPCInvalidParamsError(dummyID,
+				fmt.Errorf("error converting http params to arguments: %w", err),
 			)
+			if wErr := WriteRPCResponseHTTPError(w, http.StatusInternalServerError, res); wErr != nil {
+				logger.Error("failed to write response", "res", res, "err", wErr)
+			}
 			return
 		}
 		args = append(args, fnArgs...)
@@ -56,11 +56,17 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit
 		logger.Debug("HTTPRestRPC", "method", r.URL.Path, "args", args, "returns", returns)
 		result, err := unreflectResult(returns)
 		if err != nil {
-			WriteRPCResponseHTTPError(w, http.StatusInternalServerError,
-				types.RPCInternalError(dummyID, err))
+			if err := WriteRPCResponseHTTPError(w, http.StatusInternalServerError,
+				types.RPCInternalError(dummyID, err)); err != nil {
+				logger.Error("failed to write response", "res", result, "err", err)
+				return
+			}
+			return
+		}
+		if err := WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(dummyID, result)); err != nil {
+			logger.Error("failed to write response", "res", result, "err", err)
 			return
 		}
-		WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(dummyID, result))
 	}
 }
 

diff --git a/statesync/snapshots.go b/statesync/snapshots.go
@@ -32,9 +32,9 @@ type snapshot struct {
 func (s *snapshot) Key() snapshotKey {
 	// Hash.Write() never returns an error.
 	hasher := sha256.New()
-	hasher.Write([]byte(fmt.Sprintf("%v:%v:%v", s.Height, s.Format, s.Chunks))) //nolint:errcheck // ignore error
-	hasher.Write(s.Hash)                                                        //nolint:errcheck // ignore error
-	hasher.Write(s.Metadata)                                                    //nolint:errcheck // ignore error
+	hasher.Write([]byte(fmt.Sprintf("%v:%v:%v", s.Height, s.Format, s.Chunks)))
+	hasher.Write(s.Hash)
+	hasher.Write(s.Metadata)
 	var key snapshotKey
 	copy(key[:], hasher.Sum(nil))
 	return key
-Original file line number
+Diff line change
@@ Expand Up @@
     - P2P Protocol
     - Go API
+        - [rpc/jsonrpc/server] \#6204 Modify `WriteRPCResponseHTTP(Error)` to return an error (@melekes)
     - Blockchain Protocol
@@ Expand Down @@