Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actor improvements #66

Merged
merged 6 commits into from
May 7, 2014
Merged

Actor improvements #66

merged 6 commits into from
May 7, 2014

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented May 7, 2014

@jdantonio Is there a branch/notes I can look at?
I gave it a quick look, to identify what is missing:

  • use pooled executor in SimpleActorRef
  • make sure only one message is processed at a time. Add proxy class before any executor which will ensure that only one block is being executed at a time for a given key (e.g. actor/agent object) (use this also for agents)
  • remove @executor.shutdown from #shutdown
  • Unnecessary method lookup cache dropping, see source, originaly Unnecessary method lookup cache dropping #67
  • make sure all IVars are resolved when actor is shutdown before processing the message
  • split Ref to Ref and Core ?
  • merge actress into concurrent-ruby

Later

  • un/become ?
  • dead letter routing ?

@jdantonio
Copy link
Member

I have a bunch of notes in a branch on my other computer. I'll post those here tomorrow. I was considering a new ActorRef subclass. Mainly because of restart strategies. But I haven't committed to any specific design yet so I'm open to suggestion.

Akka supports two restart strategies (one for one, all for one) whereas Erlang supports three (same as Akk plus 'rest for one'). I was considering supporting the same two as Akka with 'one for one' being the default. (In this implementation I referred to it as 'reset' rather than 'restart' because I was concerned that 'restart' might imply shutting down and restarting the thread pool).

I've also been debating whether we need a true mailbox. Erlang processes have persistent mailboxes as do Akka actors. But neither of those supports a true pool of actors (Akka's pools are really a router actor load balancing across children actors). Not having a real message mailbox makes our actors much simpler (especially when you take into consideration "dead letter" routing) so I'm partial to that approach, but I'm concerned that we may be missing an important feature if we go that route.

What are your thoughts on restart strategies and message mailboxes?

@pitr-ch
Copy link
Member Author

pitr-ch commented May 7, 2014

Depends on #65, relevant commit here is 199314f.

@pitr-ch pitr-ch changed the title Use pools in ActorContext instead of Thread per actor. Actor improvements May 7, 2014
@pitr-ch
Copy link
Member Author

pitr-ch commented May 7, 2014

I've submitted a commit using pool instead of thread per actor.

@pitr-ch
Copy link
Member Author

pitr-ch commented May 7, 2014

I am thinking that we should start with having solid implementation of actors running on a pool first including simple supervision without strategies. Then I would start adding callbacks, children-parent relation, restart strategies and un/become.

Restart strategies sounds good to me. rest for one seems less important to me so +1 for just one for one and all for one from the start.

I would start without real mailboxes, we can ensure though that every sent massage's IVar will be rejected when actor is shutdown without processing it. That should reduce the need for DeadLetterRouting I think. Also DeadLetterRouting is really hard, AFAIK even Akka does not guarantee that all undelivered messages will end up there.

Event based actors do not have stack like Erlang actors so we should support something like un/become or build in Statemachine in future, what do you think?

I would suggest splitting Ref into just a Ref and Core. That way we can later create RemoteRef talking to core in another process. I'll look into taking actress and incorporating it into concurrent-ruby (ditching my implementation of Future and similar), it already has this split.

@jdantonio
Copy link
Member

@pitr-ch I like the incremental approach you suggest and I agree with all your suggestions. You and I are very aligned in our thinking.

I haven't given any thought to un/become or state machines yet. I'm definitely open to exploring those features once we get basic actors working.

There are some very good ideas in your Actress gem so thank you very much for offering to implement those ideas here. I'm going to merge this PR so you can continue your excellent work.

I've given you commit access to this gem. Thank you for you help and welcome to the team!

jdantonio added a commit that referenced this pull request May 7, 2014
@jdantonio jdantonio merged commit 7c643cc into ruby-concurrency:master May 7, 2014
@pitr-ch
Copy link
Member Author

pitr-ch commented May 8, 2014

Thanks for the warm welcome :)

@pitr-ch pitr-ch mentioned this pull request May 9, 2014
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants