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

Update Actors error handling #722

Merged
merged 21 commits into from
Oct 1, 2020
Merged

Update Actors error handling #722

merged 21 commits into from
Oct 1, 2020

Conversation

austinabell
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • This all stems from the gas charging blockstore (and some from gas charging syscalls). In the go implementation, they panic exactly where the error is encountered and that error is caught outside of their vm. Since we are (and should) bubble the errors up instead of panicking, that error exit code cannot be lost. This is done through the error downcasting. The reason for the trait implementation is because Amt and Hamt errors, as well as dynamic errors can downcast into a ActorError because of SysErrOutOfGas from blockstore interactions within those structures.
    • I've checked every mapped error and conversion (there is over 500) and this is what I've come up with

The only time this can ever be a difference is if the go implementation uses (ExitCode) Wrapf or rt.Abort, an out of gas error was not hit, and there was a different actor error was hit that would be overwritten by this. After checking all usages of this (I could be missing a case and was just skimming, there was ~300 of these as well) I'm semi confident all of them do not fit this criteria, or when they do the go implementation would just fatal error (so it doesn't matter)

Reference issue to close (if applicable)

Closes #720

Other information and links

@timvermeulen
Copy link
Contributor

Good stuff! The ActorDowncast trait looks like a good approach to me.

e
e.downcast_default(
ExitCode::ErrIllegalState,
format!("failed to replaces sector expirations at {:?}", key),
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional/Nit: 'replace' instead of replaces

@austinabell austinabell merged commit 630b54f into main Oct 1, 2020
@austinabell austinabell deleted the austin/iplderrhandle branch October 1, 2020 17:07
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.

Allow Amt and Hamt to forward actor errors through blockstore usage
3 participants