Closed hardliner66 closed 4 years ago
@hardliner66
Greatly appreciate this. I'm going to give this a deeper review since it touches on some of the key concepts of actor representation and what is probably the most significant developer interaction, i.e. actor creation.
I personally like your idea, just let me see if there's any downside to adopting this. I remember TypeSafe depricating this capability a while back for Akka and standardizing all actor creation through the use of Props
. But then that may well have been due to Scala specific reasons.
@hardliner66 The approach in general is sound - I like it. The only remaining thought I have is is implementing the different variants of actor creation (e.g. actor_of
, actor_of_args
, etc) on System
and Context
etc better than making them part of Props
?
If all actor creation decisions are made through Props
it means:
ActorRefFactory
Props
is needed and a call of actor_of
and that contains everything required to create the actor. A common pattern is to pass Props
as an argument to another actor so that actor may use it for creation of children. This is why Akka consolidated actor “setup” around Props
In later versions.sys.actor_of
Without needing a Props
first is lost. I personally see that convenience really only applying to small projects and an inconvenience to have to learn a new way and rewrite code as the project grows beyond small.E.g.
// start the system and create an actor
fn main() {
let sys = ActorSystem::new().unwrap();
Iet props1 = Props::new::<MyActor>();
Iet props2 = Props::new_args::<MyActor, _>(10_i32);
Iet props3 = Props::new_args::<MyActor, _>(20_i64);
let my_actors = vec![
sys.actor_of(props1, "my-actor-1").unwrap(),
sys.actor_of(props2, "my-actor-2").unwrap(),
sys.actor_of(props3, "my-actor-3").unwrap(),
];
my_actors
.iter().enumerate()
.for_each(|(i, actor)| actor.tell(format!("Hello {}!", i + 1), None));
std::thread::sleep(Duration::from_millis(500));
}
What are your thoughts?
I definitely see the benefit of using Props
in some cases, thats the reason I renamed the functions instead of removing them. But I think, most of these are advanced use cases. Also, when you are a beginner, you shouldn't need to think about Props
. You shouldn't have to think about how you're actor is created until you actually need it.
Take Erlang for example. In Erlang an actor is simply a function which receives messages. You do not have to care how the function is constructed, you just spawn it and you're done. It even works with supervision out of the box. If you need more advanced stuff, you have to write more advanced code.
I think that this is a good design. Make the simple case simple and make the advanced stuff possible. Because if you need the advanced stuff, you probably already know what you are doing.
So my suggestion would be to provide a simple interface, if you just need to spawn an actor. And provide an advanced interface, if you need the added flexibility. This way you still retain the flexibilty of Props
, while also giving the user a way to choose something simpler for everthing else.
Also, there is a benefit in understandabilty. Similar to for-loop vs. iterator. A for-loop is certainly more flexible, but using iterators with the corresponding functions like map or reduce it's easier to convey intent. I think the same goes for actor creation.
With the following you see exactly which actor use the simple spawn behaviour and which doesn't
let my_actors = vec![
sys.actor_of::<MyActor>("my-actor-1").unwrap(), // simple case
sys.actor_of_args::<MyActor, _>("my-actor-2", 10_i32).unwrap(), // simple, with args
sys.actor_of_args::<MyActor, _>("my-actor-3", 20_i64).unwrap(), // simple, with args
sys.actor_of_props("my-actor-4", props).unwrap(), // maybe a more advanced case
];
Btw. I don't think that making all of this part of Props
is a bad idea, if it removes code duplication. All the functions I made still use Props ander the hood.
Maybe we could combine both approaches, because still I think that we should provide both options and let the user decide what he wants/needs to use.
We do have Erlang developers using Riker so you have a good point on having some similarity with that. By combining as you’ve suggest do you mean making this possible?:
// creating actors without props
let a1 = sys.actor_of::<MyActor>("my-actor-1").unwrap();
let a2 = sys.actor_of_args::<MyActor, _>("my-actor-2", 10_i32).unwrap();
// getting props
let props1 = Props::new::<MyActor>();
let props2 = Props::new_args::<MyActor, _>(20_i64);
// creating actors using props
let a3 = sys.actor_of_props("my-actor-3", props1).unwrap();
let a4 = sys.actor_of_props("my-actor-4", props2).unwrap();
This works well in my view. How would multiple constructor arguments work?
For multiple arguments you can use a tuple.
I changed my example from above to include a multi parameter constructor:
use riker::actors::*;
use std::time::Duration;
// Actors with the ActorFactory or Default trait
// can be created with the actor_of function
#[derive(Default)]
struct MyActor {
value: i32,
msg: String,
}
// Actors with the ActorFactoryArgs<T> trait
// can be created with the actor_of_args function
impl ActorFactoryArgs<i32> for MyActor {
fn create_args(value: i32) -> Self {
MyActor {
value,
msg: "".to_string(),
}
}
}
// Actors with the ActorFactoryArgs<T> trait
// can be created with the actor_of_args function
impl ActorFactoryArgs<String> for MyActor {
fn create_args(value: String) -> Self {
MyActor {
value: -1,
msg,
}
}
}
// Actors with the ActorFactoryArgs<T> trait
// can be created with the actor_of_args function
impl ActorFactoryArgs<(i32, String)> for MyActor
{
fn create_args((value, msg): (i32, String)) -> Self {
MyActor {
value,
msg,
}
}
}
// implement the Actor trait
impl Actor for MyActor {
type Msg = String;
fn recv(&mut self, ctx: &Context<String>, msg: String, _sender: Sender) {
println!(
"[{}] :: Received: \"{}\", self.value: {}, [optional] self.msg: {}",
ctx.myself().name(),
msg,
self.value,
self.msg,
);
}
}
// start the system and create an actor
fn main() {
let sys = ActorSystem::new().unwrap();
let my_actors = vec![
sys.actor_of::<MyActor>("my-actor-1").unwrap(),
sys.actor_of_args::<MyActor, _>("my-actor-2", 10).unwrap(),
sys.actor_of_args::<MyActor, _>("my-actor-3", "test".to_string()).unwrap(),
sys.actor_of_args::<MyActor, _>("my-actor-4", (20, "test".to_string())).unwrap(),
];
my_actors
.iter().enumerate()
.for_each(|(i, actor)| actor.tell(format!("Hello {}!", i + 1), None));
std::thread::sleep(Duration::from_millis(500));
}
Very elegant. I want to merge but before I do let’s have a plan for Props
for consistency. Does my example above make sense for you and if so would you like to make the changes yourself?
Sounds like a good idea. Yeah, your examples make sense. If you already have something on your mind you can go ahead and implement it, if not I will look into it.
If you can go ahead and implement I imagine we can get it done faster since you’ve already done it for actor_of
, plus having another person intimately familiar with this part of the code is an advantage. Doesn’t have to be perfect I’d say, we just want to have consistency between actor setup (Props
) and actor creation (actor_of
) APIs. Any shortcuts taken in the code we can revisit later.
In the meantime I can make use of the time to look at the latest Rust futures.
Let me know if that is ok with you. This will be a major improvement in API design.
Yeah, thats okay for me.
This is the new example with the updated functions:
use riker::actors::*;
use std::{time::Duration};
// Actors with the ActorFactory or Default trait
// can be created with the actor_of function
#[derive(Default)]
struct MyActor {
value: i32,
}
// Actors with the ActorFactoryArgs<T> trait
// can be created with the actor_of_args function
impl ActorFactoryArgs<i32> for MyActor {
fn create_args(value: i32) -> Self {
MyActor { value }
}
}
// implement the Actor trait
impl Actor for MyActor {
type Msg = String;
fn recv(&mut self, ctx: &Context<String>, msg: String, _sender: Sender) {
println!(
"[{}] :: Received: \"{}\", self.value: {}",
ctx.myself().name(),
msg,
self.value,
);
}
}
// start the system and create an actor
fn main() {
let sys = ActorSystem::new().unwrap();
// new props implementation
let p1 = Props::new::<MyActor>();
let p2 = Props::new_args::<MyActor, _>(30);
// old props implementation (renamed)
let p3 = Props::new_from(MyActor::create);
let p4 = Props::new_from_args( MyActor::create_args, 20);
let my_actors = vec![
sys.actor_of::<MyActor>("my-actor-d1").unwrap(),
sys.actor_of_args::<MyActor, _>("my-actor-d2", 10).unwrap(),
sys.actor_of_props("my-actor-p1", p1).unwrap(),
sys.actor_of_props("my-actor-p2", p2).unwrap(),
sys.actor_of_props("my-actor-p3", p3).unwrap(),
sys.actor_of_props("my-actor-p4", p4).unwrap(),
];
my_actors
.iter()
.enumerate()
.for_each(|(i, actor)| actor.tell(format!("Hello {}!", i + 1), None));
std::thread::sleep(Duration::from_millis(500));
}
I renamed Props::new
and Props::new_args
to Props::new_from
and Props::new_from_args
respectively and implemented Props::new
and Props::new_args
to work with the traits ActorFactory
and ActorFactoryArgs
.
The two new functions use the renamed Props::new_from
and Props::new_from_args
internally.
The actor_of
and actor_of_args
functions now also use the new functions.
Finally, I added #[inline]
to some functions, so most of the calls should be optimized away.
Looks good to me :) 👍
I’ll merge this now . Tomorrow I’ll run a few additional checks myself and update website docs to reflect this change in API. We should then be good to publish.
EPIC
This pull request introduces two new traits: ActorFactory and ActorFactoryArgs to create an actor without and with parameters respectively.
This has a few benefits: 1) a standardized way to create an actor instead of relying on naming conventions 2) a blanket implementation for structs which implement the
Default + Actor
traitThis simplifies the default case for actor creation, because you only need to know the type of your Actor instead of the name of the "constructor" function.
Here is a small example:
For more complex scenarios, one can still manually create Props and use the new
actor_of_props
function.