From 610cf02c4a821f16ffecec68c633d7294fb6dc79 Mon Sep 17 00:00:00 2001 From: Nicholas Date: Fri, 9 Sep 2022 05:03:23 -0700 Subject: [PATCH] rpc: improve error codes for internal server errors (#25678) This changes the error code returned by the RPC server in certain situations: - handler panic: code -32603 - result marshaling error: code -32603 - attempt to subscribe via HTTP: code -32001 In all of the above cases, the server previously returned the default error code -32000. Co-authored-by: Nicholas Zhao Co-authored-by: Felix Lange --- rpc/client_test.go | 8 ++++++-- rpc/errors.go | 18 +++++++++++++++++- rpc/handler.go | 6 ++++-- rpc/json.go | 5 ++--- rpc/server_test.go | 2 +- rpc/service.go | 3 +-- rpc/testdata/internal-error.js | 7 +++++++ rpc/testservice_test.go | 14 ++++++++++++++ 8 files changed, 52 insertions(+), 11 deletions(-) create mode 100644 rpc/testdata/internal-error.js diff --git a/rpc/client_test.go b/rpc/client_test.go index 04c847d0d..51df76f7f 100644 --- a/rpc/client_test.go +++ b/rpc/client_test.go @@ -82,11 +82,15 @@ func TestClientErrorData(t *testing.T) { } // Check code. + // The method handler returns an error value which implements the rpc.Error + // interface, i.e. it has a custom error code. The server returns this error code. + expectedCode := testError{}.ErrorCode() if e, ok := err.(Error); !ok { t.Fatalf("client did not return rpc.Error, got %#v", e) - } else if e.ErrorCode() != (testError{}.ErrorCode()) { - t.Fatalf("wrong error code %d, want %d", e.ErrorCode(), testError{}.ErrorCode()) + } else if e.ErrorCode() != expectedCode { + t.Fatalf("wrong error code %d, want %d", e.ErrorCode(), expectedCode) } + // Check data. if e, ok := err.(DataError); !ok { t.Fatalf("client did not return rpc.DataError, got %#v", e) diff --git a/rpc/errors.go b/rpc/errors.go index 4c06a745f..9a19e9fe6 100644 --- a/rpc/errors.go +++ b/rpc/errors.go @@ -54,9 +54,15 @@ var ( _ Error = new(invalidRequestError) _ Error = new(invalidMessageError) _ Error = new(invalidParamsError) + _ Error = new(internalServerError) ) -const defaultErrorCode = -32000 +const ( + errcodeDefault = -32000 + errcodeNotificationsUnsupported = -32001 + errcodePanic = -32603 + errcodeMarshalError = -32603 +) type methodNotFoundError struct{ method string } @@ -101,3 +107,13 @@ type invalidParamsError struct{ message string } func (e *invalidParamsError) ErrorCode() int { return -32602 } func (e *invalidParamsError) Error() string { return e.message } + +// internalServerError is used for server errors during request processing. +type internalServerError struct { + code int + message string +} + +func (e *internalServerError) ErrorCode() int { return e.code } + +func (e *internalServerError) Error() string { return e.message } diff --git a/rpc/handler.go b/rpc/handler.go index cd95a067f..22ad98149 100644 --- a/rpc/handler.go +++ b/rpc/handler.go @@ -48,7 +48,6 @@ import ( // if err := op.wait(...); err != nil { // h.removeRequestOp(op) // timeout, etc. // } -// type handler struct { reg *serviceRegistry unsubscribeCb *callback @@ -354,7 +353,10 @@ func (h *handler) handleCall(cp *callProc, msg *jsonrpcMessage) *jsonrpcMessage // handleSubscribe processes *_subscribe method calls. func (h *handler) handleSubscribe(cp *callProc, msg *jsonrpcMessage) *jsonrpcMessage { if !h.allowSubscribe { - return msg.errorResponse(ErrNotificationsUnsupported) + return msg.errorResponse(&internalServerError{ + code: errcodeNotificationsUnsupported, + message: ErrNotificationsUnsupported.Error(), + }) } // Subscription method name is first argument. diff --git a/rpc/json.go b/rpc/json.go index 6b2ac2d52..1064939ff 100644 --- a/rpc/json.go +++ b/rpc/json.go @@ -104,15 +104,14 @@ func (msg *jsonrpcMessage) errorResponse(err error) *jsonrpcMessage { func (msg *jsonrpcMessage) response(result interface{}) *jsonrpcMessage { enc, err := json.Marshal(result) if err != nil { - // TODO: wrap with 'internal server error' - return msg.errorResponse(err) + return msg.errorResponse(&internalServerError{errcodeMarshalError, err.Error()}) } return &jsonrpcMessage{Version: vsn, ID: msg.ID, Result: enc} } func errorMessage(err error) *jsonrpcMessage { msg := &jsonrpcMessage{Version: vsn, ID: null, Error: &jsonError{ - Code: defaultErrorCode, + Code: errcodeDefault, Message: err.Error(), }} ec, ok := err.(Error) diff --git a/rpc/server_test.go b/rpc/server_test.go index d09d31634..c9abe53e5 100644 --- a/rpc/server_test.go +++ b/rpc/server_test.go @@ -45,7 +45,7 @@ func TestServerRegisterName(t *testing.T) { t.Fatalf("Expected service calc to be registered") } - wantCallbacks := 10 + wantCallbacks := 12 if len(svc.callbacks) != wantCallbacks { t.Errorf("Expected %d callbacks for service 'service', got %d", wantCallbacks, len(svc.callbacks)) } diff --git a/rpc/service.go b/rpc/service.go index bef891ea1..cfdfba023 100644 --- a/rpc/service.go +++ b/rpc/service.go @@ -18,7 +18,6 @@ package rpc import ( "context" - "errors" "fmt" "reflect" "runtime" @@ -199,7 +198,7 @@ func (c *callback) call(ctx context.Context, method string, args []reflect.Value buf := make([]byte, size) buf = buf[:runtime.Stack(buf, false)] log.Error("RPC method " + method + " crashed: " + fmt.Sprintf("%v\n%s", err, buf)) - errRes = errors.New("method handler crashed") + errRes = &internalServerError{errcodePanic, "method handler crashed"} } }() // Run the callback. diff --git a/rpc/testdata/internal-error.js b/rpc/testdata/internal-error.js new file mode 100644 index 000000000..2ba387401 --- /dev/null +++ b/rpc/testdata/internal-error.js @@ -0,0 +1,7 @@ +// These tests trigger various 'internal error' conditions. + +--> {"jsonrpc":"2.0","id":1,"method":"test_marshalError","params": []} +<-- {"jsonrpc":"2.0","id":1,"error":{"code":-32603,"message":"json: error calling MarshalText for type *rpc.MarshalErrObj: marshal error"}} + +--> {"jsonrpc":"2.0","id":2,"method":"test_panic","params": []} +<-- {"jsonrpc":"2.0","id":2,"error":{"code":-32603,"message":"method handler crashed"}} diff --git a/rpc/testservice_test.go b/rpc/testservice_test.go index 253e26328..8454a4019 100644 --- a/rpc/testservice_test.go +++ b/rpc/testservice_test.go @@ -70,6 +70,12 @@ func (testError) Error() string { return "testError" } func (testError) ErrorCode() int { return 444 } func (testError) ErrorData() interface{} { return "testError data" } +type MarshalErrObj struct{} + +func (o *MarshalErrObj) MarshalText() ([]byte, error) { + return nil, errors.New("marshal error") +} + func (s *testService) NoArgsRets() {} func (s *testService) Echo(str string, i int, args *echoArgs) echoResult { @@ -114,6 +120,14 @@ func (s *testService) ReturnError() error { return testError{} } +func (s *testService) MarshalError() *MarshalErrObj { + return &MarshalErrObj{} +} + +func (s *testService) Panic() string { + panic("service panic") +} + func (s *testService) CallMeBack(ctx context.Context, method string, args []interface{}) (interface{}, error) { c, ok := ClientFromContext(ctx) if !ok {