-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JSON RPC Client doesn't handle RPC errors. #672
Comments
The current behavior was by design, was it not? |
Not really, no. The errors returned in the response body should have been surfaced via the endpoint. |
I was thinking of this discussion, which doesn't carry the intent I thought it did. |
I think the biggest issue here is the payload sent to the decode response func of the client. It indeed should not be fed with However diverting it to an endpoint error is not a good idea either as it might incorrectly trip retry logic, circuit breakers and such. Idiomatic to Go kit would be to pass along the entire payload rpcRes to the decode response func and have the implementor of a Go kit client parse how the error should be handled. Is it a real failure / misbehaving error needing to be seen by endpoint middleware like circuit breakers, retry logic, etc. or is it an error that is specific to the business logic and is only understood by the function consumer. |
Thanks @basvanbeek, that makes more sense to me, too. I've updated the PR. |
* Handle JSON RPC errors. Resolves issue go-kit#672. * Refactor decode func to receive JSON RPC response. Remove error func. * Comment tweaks.
If the server responds with a valid JSON RPC response, containing an error (e.g. see below), the client does not recognise this, and continues to attempt to decode the empty result field.
{ "jsonrpc": "2.0", "error": { "code": -32603, "message": "JWT Token is expired" } }
On receipt of an error like this, I'd like the endpoint to return a nil result (the JSON RPC spec says that Error and Result cannot co-exist in a response) and an error. It might also be useful to have a server option that can allow the error to be mapped to a different error type, or perhaps swallowed altogether.
The text was updated successfully, but these errors were encountered: