FromActorRef<TFrom> which allows to send messages convertible to original TMessage

[anabyv042]

Adding FromActorRef<TFrom> - a wrapper over ActorRef<TMessage> that allows to send messages of type TFrom to the actor if TFrom: Into<TMessage>. This allows to create some isolation in the actor system as discussed in #305.

[anabyv042]

Created a draft PR to validate the initial approach. I will expand it if the approach is correct.

I have some concerns/questions about the current implementation:

1. Naming - I couldn't come up with a better name myself so would be grateful for any suggestions :)
2. TFrom: Clone - I needed to enforce the clonability of the message so that I wouldn't break the MessagingErr. Would there be a better way to do this?
3. What API do we want to expose on the wrapper? should it be a subset of ActorRef methods or it should be identical?

[codecov]

Codecov Report

Attention: Patch coverage is 83.69565% with 15 lines in your changes missing coverage. Please review.

Project coverage is 82.19%. Comparing base (3c94f0f) to head (d8e14f8).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ractor/src/actor/derived_actor.rs	60.00%	14 Missing ⚠️
ractor/src/actor/tests/mod.rs	98.24%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
+ Coverage   82.17%   82.19%   +0.01%     
==========================================
  Files          60       61       +1     
  Lines       11032    11124      +92     
==========================================
+ Hits         9066     9143      +77     
- Misses       1966     1981      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[slawlor]

This is a great start! I think however we can improve upon the API, and potentially drop the boxed converter, just relying on type constraints for functionality implementation. Let me know if my comments make sense!

[slawlor]

Hmm I feel like the clone should be avoidable. What about a dual implementation, where we have

FromActorRef<TFrom: TryFrom<TMessage>, TMessage: From<TFrom>>

And we panic if we can't deconvert the TryFrom. That way you can avoid the clone, by capturing the original

MessagingErr::SendErr(message) => MessageErr::SendErr(msg.from_into().expect("Failed to convert message back to base type"))

I don't love the clone restriction, as it has a lot of downstream consequences (think like a tcp stream, etc) and vastly restricts the usage. I'd rather utilize the expect and rely on the fact that a messaging error means the send failed, and therefore we're going to get back the same datatype, and can safely unwrap it back.

However it does require the user to implement both patterns of conversion.

[slawlor]

nit: The type should also be added to the public re-exports at the crate base.

[slawlor]

nit: This isn't documented in the tests super well, but use of periodic_check was all over the place prior to the addition of the drain() functionality. Now we have the ability to drain messages, and wait for the actor cleanup, so you can just drain, wait for stop, and then read the counter without polling it.

It will yield more effecient tests, since you don't have a polling with a timeout/etc. I need to refactor a lot of the tests to account for this tbh.

[anabyv042]

This is a great start! I think however we can improve upon the API, and potentially drop the boxed converter, just relying on type constraints for functionality implementation. Let me know if my comments make sense!

Thanks for the review! Regarding the boxed converter, this was added to get rid of TMessage on FromActorRef definition. As I understand, the suggestion is to parametrize FromActorRef by both TFrom and TMessage and store ActorRef internally:

pub struct FromActorRef<TFrom, TMessage> {
    inner: ActorRef<TMessage>,
    _phantom: PhantomData<TFrom>,
}

then the send message implementation will look like this:

impl<TFrom: TryFrom<TMessage>, TMessage: From<TFrom> + Message> FromActorRef<TFrom, TMessage>
where
    <TFrom as TryFrom<TMessage>>::Error: std::fmt::Debug,
{
    /// Casts the message to the target message type of [ActorCell] and sends it
    ///
    /// * `message` - The message to send
    ///
    /// Returns [Ok(())] on successful message send, [Err(MessagingErr)] otherwise
    pub fn send_message(&self, message: TFrom) -> Result<(), MessagingErr<TFrom>> {
        match self.inner.send_message(message.into()) {
            Ok(_) => Ok(()),
            Err(MessagingErr::SendErr(msg)) => Err(MessagingErr::SendErr(
                msg.try_into()
                    .expect("Failed to convert message back to base type"),
            )),
            Err(MessagingErr::ChannelClosed) => Err(MessagingErr::ChannelClosed),
            Err(MessagingErr::InvalidActorType) => Err(MessagingErr::InvalidActorType),
        }
    }
}

however this will force us to have the original message as part of the FromActorRef type which gets in the way of hiding TMessage:

let actor: ActorRef<AB> = init(); 
let from_a: FromActorRef<A, AB> = actor.clone().from_ref();

what do you think? Boxed converter is the only thing I found to hide TMessage from FromActorRef but let me know if there's a better way :) thanks!

[slawlor]

@anabyv042

however this will force us to have the original message as part of the FromActorRef type which gets in the way of hiding TMessage:

Ahh ok I see your goal. Let me try something, I think I have a workaround.

[slawlor]

OK I couldn't find a way around the Box, you're right. We need to propagate the original message type, without enforcing the type constraint on the original struct. I did however apply most of the other PR comments. Let me know what you think!

I think this is clean enough at this stage, but we might just need a little more cleanup in spots and maybe an example usage in the doc comments would be good to also be covered by doctests.

[anabyv042]

sounds good, thanks! added some doctests + added a SendError scenario to the ut to test reverse conversion

[slawlor]

Can you make sure the signals are green? I'm guessing the doc tests are failing

[anabyv042]

Fixed the formatting + added import which was failing the test. Hopefully it will pass this time

[slawlor]

Looks great to me, thanks for the additions!