actor upgrade error refactor #1910
Conversation
Add a general-purpose ControlFlow syscall return value and use it.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## actor-upgrades #1910 +/- ##
===================================================
- Coverage 75.91% 28.46% -47.45%
===================================================
Files 153 114 -39
Lines 15220 10581 -4639
===================================================
- Hits 11554 3012 -8542
- Misses 3666 7569 +3903
|
| @@ -201,9 +201,6 @@ where | |||
| events_root, | |||
| } | |||
| } | |||
|
|
|||
| Err(ExecutionError::Abort(e)) => return Err(anyhow!("actor aborted: {}", e)), | |||
There was a problem hiding this comment.
This was one of the cases I was worried about. Aborting with a fatal error here isn't really correct.
| /// The helper trait used by `BindSyscall` to convert kernel results with execution errors into | ||
| /// results that can be handled by wasmtime. See the documentation on `BindSyscall` for details. | ||
| #[doc(hidden)] | ||
| pub trait IntoSyscallResult: Sized { | ||
| pub trait IntoControlFlow: Sized { |
There was a problem hiding this comment.
I wish we could just use Into<ControlFlow<T>> instead of having to define a new trait but... the same type could implement Into<ControlFlow<T>> and IntoControlFlow<U>>. So we need a trait with an associated type (i.e., like this one).
| if exit_code.is_success() { | ||
| ControlFlow::Abort(Abort::Exit(exit_code, String::new(), block_id)) | ||
| } else { | ||
| ControlFlow::Return(sys::out::send::Send { | ||
| exit_code: exit_code.value(), | ||
| return_id: block_id, | ||
| return_codec: block_stat.codec, | ||
| return_size: block_stat.size, | ||
| }) | ||
| } |
There was a problem hiding this comment.
plus having it here, especially with the explicit naming makes it very easy to read and maintain
| fn into(self) -> Result<Result<Self::Value, SyscallError>, Abort> { | ||
| fn into_control_flow(self) -> ControlFlow<Self::Value> { | ||
| match self { | ||
| Ok(value) => Ok(Ok(value)), | ||
| Ok(value) => ControlFlow::Return(value), | ||
| Err(e) => match e { | ||
| ExecutionError::Syscall(err) => Ok(Err(err)), | ||
| ExecutionError::OutOfGas => Err(Abort::OutOfGas), | ||
| ExecutionError::Fatal(err) => Err(Abort::Fatal(err)), | ||
| ExecutionError::Abort(e) => Err(e), |
| // TODO: Check error cases. At a minimum, we could run out of gas here! | ||
| Some(block) => (block.stat(), self.blocks.put_reachable(block)?), |
There was a problem hiding this comment.
hmm, I don't see put_reachable ever checking if we run out of gas
There was a problem hiding this comment.
Hm. You're right... It looks like the only failure case is the LimitExceeded case which... likely shouldn't fail.
But I'd avoid casting the error if possible.
Add a general-purpose ControlFlow syscall return value and use it. This way we don't need to add an
::Abortcase to theExecutionErroritself.