Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

test: actor abort #118

Merged
merged 7 commits into from
Sep 10, 2020
Merged

test: actor abort #118

merged 7 commits into from
Sep 10, 2020

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Sep 4, 2020

TODO:

  • apply_message_failures in vectors - do we still want to do that? (context)
  • Some documentation for null receipts

Depends on:

Upstream fixes for broken tests (not blocking merge):

resolves #87
resolves #15
closes #49

@alanshaw alanshaw marked this pull request as ready for review September 9, 2020 17:19
@willscott
Copy link
Contributor

to confirm: on messages that fail to apply we get a null message receipt, but no other encoding within the test-vector json of the error code or other things around message failure?
My suspicion is that error codes would be a good thing to standardize and are worth including somewhere.

@alanshaw
Copy link
Member Author

to confirm: on messages that fail to apply we get a null message receipt, but no other encoding within the test-vector json of the error code or other things around message failure?

Correct. Although, looks like this may not end up being the case... filecoin-project/lotus#3697

My suspicion is that error codes would be a good thing to standardize and are worth including somewhere.

Under usual circumstances the receipt will carry the exit/error code. Actors are expected to call Runtime.Abortf to fail with a standardized exit code. null receipts cater for the very exceptional case where the actor panics without using Runtime.Abortf. This could be in an actor dependency or because of an OOM for example. This type of error will have no interoperable error code or message to record in our vectors.

})

// am.Result may still be nil if the message failed to be applied
if am.Result != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a more idiomatic way of doing this would be:

var receipt *schema.Receipt
if am.Result != nil {
    receipt = &schema.Receipt{
        ExitCode:    int64(am.Result.ExitCode),
        ReturnValue: am.Result.Return,
        GasUsed:     am.Result.GasUsed,
    }
}
b.vector.Post.Receipts = append(b.vector.Post.Receipts, receipt)

Comment on lines 153 to +157
traces := make([]types.ExecutionTrace, 0, len(msgs))
for _, msgs := range msgs {
traces = append(traces, msgs.Result.ExecutionTrace)
for _, msg := range msgs {
if msg.Result != nil {
traces = append(traces, msg.Result.ExecutionTrace)
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we want to make the traces a slice of pointers []*types.ExecutionTrace, and leave a nil element for each message that doesn't have a result?

@@ -103,7 +103,7 @@ func (b *MessageVectorBuilder) CommitApplies() {

for _, am := range b.Messages.All() {
// apply all messages that are pending application.
if am.Result == nil {
if !am.Applied {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, this is a lot more explicit.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

apply_message_failures in vectors - do we still want to do that?

I think we want to be as declarative and explicit as possible. After all, a general goal of test vectors is to remove ambiguity and uncertainty. For machines, it might be redundant to have this index, but I anticipate that this explicit acknowledgement of which messages fail will be useful for when humans have to debug something.

Related: I like what you did with ApplicableMessage#Applied and I'm thinking we should add a Failed field to ApplicableMessage, and use that instead of nil checks everywhere. A nil can mean a lot of things, but Failed: true can only mean one thing.

We can also replace the Nil() predicate by a Failed() predicate, which makes the API a lot more semantic and self-descriptive, IMO.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

This is OK and I'm happy to merge as-is. I'll open a follow-up issue to try to address the comments and the TODO items!

@raulk raulk merged commit 56e0fd0 into master Sep 10, 2020
@raulk raulk deleted the test/actor-abort branch September 10, 2020 16:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants