riker-rs / riker

Easily build efficient, highly concurrent and resilient applications. An Actor Framework for Rust.
https://riker.rs
MIT License
1.02k stars 69 forks source link

Remove actor id #128

Closed hardliner66 closed 4 years ago

hardliner66 commented 4 years ago

Removes the actor ID because it isn't used anywhere except to check if an actor is the root. Also if riker is used on systems where usize is 32 bit or smaller, there is a chance that the actor id wraps around and multiple actors could end up with the same id.

The old is_root checks if the actor id is 0. In case of an overflow this could happen more than once.

The new is_root function uses the actor path, because it is already guaranteed that there can't be a second actor with the same path.

leenozara commented 4 years ago

If I remember, this is used to differentiate between instances of the same actor at the same path. This occurs during supervision events such as Restart. When an actor is restarted, it occupies the same path as the failed actor but is given a new ID. This assists in debugging and also allows users to be able to make decisions on versions of the actor.

igalic commented 4 years ago

Good grief, i really like to forget sometimes, that there are — relatively common! — platforms without 64 bit atomics

hardliner66 commented 4 years ago

@leenozara I just wrote a simple test and it seems the actor id's aren't working as you described:

use riker::actors::*;

#[derive(Debug)]
struct ActorIds {
    fail_on_start: bool,
    fail_on_recv: bool,
}

impl ActorFactoryArgs<(bool, bool)> for ActorIds {
    fn create_args((fail_on_start, fail_on_recv): (bool, bool)) -> Self {
        ActorIds {
            fail_on_start,
            fail_on_recv,
        }
    }
}

// implement the Actor trait
impl Actor for ActorIds {
    type Msg = i32;

    fn pre_start(&mut self, ctx: &Context<Self::Msg>) {
        println!("Started: {:?}", ctx.myself.uri());
    }

    fn post_start(&mut self, _ctx: &Context<Self::Msg>) {
        if self.fail_on_start {
            panic!("OMG");
        }
    }

    fn recv(&mut self, _ctx: &Context<Self::Msg>, _msg: Self::Msg, _sender: Sender) {
        if self.fail_on_recv {
            panic!("OMG2");
        }
    }
}

fn main() {
    let sys = ActorSystem::new().unwrap();

    let fail_on_start = true;
    let fail_on_recv  = true;

    let a = sys.actor_of_args::<ActorIds, _>("asd", (fail_on_start, fail_on_recv)).unwrap();
    a.tell(1, None);

    while sys.user_root().has_children() {
        std::thread::sleep(std::time::Duration::from_millis(50));
    }
}

If the actor panics in post_start, it is restarted with the same actor id as the actor before (103). If the actor panics in recv, it isn't restarted at all.

I think it would be better to log the panic when an actor crashes/restarts. This should be enough to debug what happened.

I don't know how often it is needed to differentiate on the number of restarts an actor has made. If you just need to know if an actor has restarted, we could provide a on_restart callback on the Actor trait, so that a user can decide what the actor needs to do in case of a restart.

If there is a good use-case for the number, it would probably be better to keep a number per actor, so you can see which generation this actor is and not just if the current number is different than some number the actor had before.

leenozara commented 4 years ago

If there is a good use-case for the number, it would probably be better to keep a number per actor, so you can see which generation this actor is and not just if the current number is different than some number the actor had before.

That makes sense to me and I like the term "generation" to describe it too. This is how Akka and other systems have done this.

The case for checking the generation would be be used by other actors when subscribing to ActorRestarted system event, or used in clustering, but that is best addressed separately in an RFC.

Thanks for clarifying! I'll merge it.