RFC: Include intended recipient name in SendError

[strohel]

Add test for SendError

We're going to touch it, the tests will demonstrate the changes.

Include intended recipient name in SendError

This aids with debugging send errors. Thanks to type decoupling made by
Recipient, senders may not even know the actual receiving actor, so
senders cannot replicate this behaviour fully on their own.

Should help with debugging downstream issues like tonarino/portal#1153.

Caveats:

• Increases the size of Recipient, Addr, SendError by one pointer two pointers (one &str).
• Introduces API-breaking changes to SendError and SendResultExt::on_full().
  • Specifically, SendError, DisconnectedError can no longer be constructed by downstream code. That feels correct now, but can be fixed in non-API-breaking manner if needed. - no longer true after the last commit.

---

Is it worth it?

[skywhale]

I agree that this is useful for debugging actor channel bloat issues.

Did you consider including the intended message instead, like crossbeam_channel does? Some pros and cons that I can think of:

Pros:

• We can inspect the content of the message that is being dropped in case of error
• Particularly useful for actors that take an enum message

Cons:

• Messages need to implement Debug + Send?
• There are cases multiple actors take the same message type e.g. MediaFrame.

[skywhale]

I remember having this conversation in some other PR, and we said we usually prefer making the fields pub instead of adding getters unless changing a value breaks an invariant?

[strohel]

The conversation was probably this one: #53 (comment) (+ related #56).

I still don't have any clear opinion on this. Likely this case would fit to "compound, passive data structure" per Rust API guidelines?

Public fields are most appropriate for struct types in the C spirit: compound, passive data structures. Otherwise, consider providing getter/setter methods and hiding fields instead.

This type being an error type, I think I was subconsciously affected by https://matklad.github.io/2020/10/15/study-of-std-io-error.html and its praise of the std::io::Error.

Thinking about it more, I think I slightly lean towards making the fields public. The stability guarantee is much less of a concern for us than for std.

[skywhale]

Yeah, io::Error type comes with accessors, and I was also wondering whether it's where you sought for consistency. I don't feel strongly either way, but I wanted to ask your opinion, so I can start forming my habits.

[strohel]

I ended up making the fields pub.

Rationale: if we ever want to contain original message M as a payload in the error, it would be more handy if it is pub so that users can move the message out of the error (without introducing special methods for it).

[strohel]

Did you consider including the intended message instead, like crossbeam_channel does? Some pros and cons that I can think of:

Pros:

• We can inspect the content of the message that is being dropped in case of error
• Particularly useful for actors that take an enum message

Cons:

• Messages need to implement Debug + Send?
• There are cases multiple actors take the same message type e.g. MediaFrame.

That's a very valid remark. Agreed that seeing the message is even more useful than seeing the recipient.

The approach is made tricky by our ability to create Recipient<N> out of Addr<M> if N can be converted to M, introduced in #38.

1. SendError cannot simply contain M as Recipient<N> must have the M type erased.
2. If we also require that M is convertible back to N, then SendError can contain N. But such a requirement feels weird and may be impossible to satisfy.
3. SendError could contain Box<dyn Any> (Any being M), but Any doesn't unfortunately allow "try downcasting to Debug trait". It only allows trying downcasting to a particular type, M in this case. That would make the whole M -> N type erasure a second class citizen.
4. SendError could contain Box<dyn Debug>, which would indeed require adding the Debug bound to Actor::Message (it is already Send) - and possibly also Sync (see below).

The ideal API would probably be: provide Option<Box<dyn Debug>> in SendError which would be Some if M: Debug and None otherwise. Maybe specialization would allow this one day, but I see no way how to achieve that in current stable rust (it looks like one can "require" a bound, but cannot "detect optional bound being satisfied").

Anyways, option (4) looks viable, so I've submitted and alternate PR #60 but it suffers from its own problems.

It is possible that we've closed a lot of doors with #38 - it could be feasible to touch some of its core ideas. For example we may try to Box the messages and convert then later on the receiving side. That could allow option (1) above.

[skywhale]

Thank you for all the considerations. The PR is still worth merging as is, but I would also love to hear other Rust experts thoughts @bschwind @mcginty @efyang @slightknack

[mcginty]

This looks good! The only thing that felt sus is that the internal trait's try_send returns a SendErrorReason instead of a SendError, but I understand the logic behind it.

The layout is a lot like what we had to do in innernet for IO error contexts, so makes sense to me.