Skip to content
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

feat: API: support typed errors over RPC #9061

Merged
merged 6 commits into from
Sep 23, 2022
Merged

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Jul 19, 2022

Related Issues

This is work towards, but does not finish #9041

Proposed Changes

  • Updates go-jsonrpc.
  • Introduces API errors, with only one case right now -- OutOfGas.
  • Demo test.

Additional Info

  • Needs Support error codes / typed errors go-jsonrpc#36 & tag
  • The chief limitation here is that wrapped errors do not get picked up by the RPC code, which relies on reflection. This is a non-trivial issue, although it is not a blocker. I'd like to discuss this with @magik6k, but it doesn't preclude landing this PR.

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@arajasek arajasek requested a review from a team as a code owner July 19, 2022 17:33
@arajasek arajasek changed the title feat: support typed errors over RPC feat: API: support typed errors over RPC Jul 19, 2022
@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

❗ No coverage uploaded for pull request base (release/v1.18.0@37be576). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 30bd0ed differs from pull request most recent head 3e81c56. Consider uploading reports for the commit 3e81c56 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##             release/v1.18.0    #9061   +/-   ##
==================================================
  Coverage                   ?   41.02%           
==================================================
  Files                      ?      708           
  Lines                      ?    78898           
  Branches                   ?        0           
==================================================
  Hits                       ?    32365           
  Misses                     ?    41014           
  Partials                   ?     5519           

@magik6k
Copy link
Contributor

magik6k commented Aug 1, 2022

The chief limitation here is that wrapped errors do not get picked up by the RPC code, which relies on reflection.

We could make go-jsonrpc recursively errors.Unwrap the error until it hits a registered error (with some depth limit just in case), then have it return a "wrapped" version of that error:

  • What the error code would be in that case is a good question, we can go one of a few routes:
    • Pre-register the 'wrapped' error type, which would marshal into the {errorMsg, subErrCode, subErr} - probably closest how errors are wrapped in Go, but weird in non-Go apps - you can't tell the error type just by looking at the error code in the jsonrpc response
    • Register wrapped error types for each wrapable error type - you can tell the error just by looking at the error code, but now there are 2 error codes per error type.
    • Mix of the above - when registering an error specify if it's wrapable - if it is, always marshal it as wrapped json, if it is not, keep current behavior.

@arajasek arajasek force-pushed the asr/rpc-errors branch 2 times, most recently from 9f475ba to af56893 Compare September 20, 2022 20:27
@arajasek arajasek changed the base branch from master to release/v1.18.0 September 21, 2022 15:17
@arajasek arajasek force-pushed the asr/rpc-errors branch 2 times, most recently from 10e6284 to f7ddb86 Compare September 21, 2022 18:03
"github.com/filecoin-project/lotus/api"
lapi "github.com/filecoin-project/lotus/api"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate import

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@magik6k
Copy link
Contributor

magik6k commented Sep 22, 2022

Tests seem to be broken because of this really robust error check: https://github.com/filecoin-project/lotus/blob/master/itests/kit/blockminer.go#L194 (which we can now make actually robust with this PR)

Comment on lines 14 to 27
func TestRetryErrorIsInTrue(t *testing.T) {
errorsToRetry := []error{&jsonrpc.RPCConnectionError{}}
require.True(t, ErrorIsIn(&jsonrpc.RPCConnectionError{}, errorsToRetry))
require.True(t, api.ErrorIsIn(&jsonrpc.RPCConnectionError{}, errorsToRetry))
}

func TestRetryErrorIsInFalse(t *testing.T) {
errorsToRetry := []error{&jsonrpc.RPCConnectionError{}}
require.False(t, ErrorIsIn(xerrors.Errorf("random error"), errorsToRetry))
require.False(t, api.ErrorIsIn(xerrors.Errorf("random error"), errorsToRetry))
}

func TestRetryWrappedErrorIsInTrue(t *testing.T) {
errorsToRetry := []error{&jsonrpc.RPCConnectionError{}}
require.True(t, ErrorIsIn(xerrors.Errorf("wrapped: %w", &jsonrpc.RPCConnectionError{}), errorsToRetry))
require.True(t, api.ErrorIsIn(xerrors.Errorf("wrapped: %w", &jsonrpc.RPCConnectionError{}), errorsToRetry))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically those tests should also be moved to api/

@@ -191,7 +192,7 @@ func (bm *BlockMiner) MineBlocksMustPost(ctx context.Context, blocktime time.Dur
reportSuccessFn := func(success bool, epoch abi.ChainEpoch, err error) {
// if api shuts down before mining, we may get an error which we should probably just ignore
// (fixing it will require rewriting most of the mining loop)
if err != nil && !strings.Contains(err.Error(), "websocket connection closed") {
if err != nil && !strings.Contains(err.Error(), "websocket connection closed") && !api.ErrorIsIn(err, []error{&jsonrpc.RPCConnectionError{}}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supernit:

Suggested change
if err != nil && !strings.Contains(err.Error(), "websocket connection closed") && !api.ErrorIsIn(err, []error{&jsonrpc.RPCConnectionError{}}) {
if err != nil && !strings.Contains(err.Error(), "websocket connection closed") && !api.ErrorIsIn(err, []error{new(jsonrpc.RPCConnectionError)}) {

@dirkmc
Copy link
Contributor

dirkmc commented Sep 23, 2022

Another use case where this would be helpful:
https://github.com/filecoin-project/boost/blob/a58ea5a92a4237a018f9e7dc700936a5cebb0688/cmd/booster-bitswap/remoteblockstore/remoteblockstore.go#L97-L102

@hunjixin
Copy link
Contributor

hunjixin commented Oct 10, 2022

@magik6k sorry for these questions after the code merge

Recently venus intended to submit some of venus changes and bug fixes for rpc to the upstream, but when pushing it, it found many conflicts with TypeError

In PR, I think the current implementation of TypeError is a bit complicated.

I'm not sure if users really need the MarshalJson/UnMarshalJson functionality, but I'm looking at lotus/boost usage and it's enough to determine the type of error, and in venus requirements it's enough to determine the type of error.
in addition, I have the impression that adding meta fields may cause errors in the jsonrpc library in rust, which does not allow additional fields. meta is not a in standard jsonrpc protocol.

ErrorCode is a good idea, but maybe more better to define ErrorCode as true error type like below code

type ErrorCode int

func (e ErrorCode) Error() string {
    return fmt.Sprintf("error code %d", e)
}

for rpc server , user fmt.Error to return specific error

if res.MsgRct.ExitCode == exitcode.SysErrOutOfGas {
        return -1, rpc.ErrorCode(exitcode.SysErrOutOfGas)
}

or convert all exitcode to rpc error

if res.MsgRct.ExitCode != exitcode.Ok {
        return -1, xerrors.Errorf("message execution failed: exit %w, reason: %s", rpc.ErrorCode(res.MsgRct.ExitCode), res.Error)
}

for rpc client, use this code to check error type

    require.True(t, xerrors.Is(err, rpc.EOutOfGas))

If write it this way, not need to add the Register function in Rpc, and the Rpc implementation can be simpler without adding the meta field in Respnse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants