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

Make Celluloid.new link by default if possible. #145

Closed
wants to merge 2 commits into from

Conversation

cararemixed
Copy link
Contributor

I've added an experimental change that makes new link by default if called within an actor's context. spawn remains unlinked by default and the new_link method will still require an actor context to link to, unlike new.

I've also added some minimal spec coverage but we might want to clean that up if we can decide on a clearer style to write these specs with proper actor cleanup.

@tarcieri
Copy link
Member

tarcieri commented Jan 8, 2013

Some useful background on this is how Akka works:

http://doc.akka.io/docs/akka/snapshot/general/supervision.html

Akka implements a specific form called “parental supervision”. Actors can only be created by other actors—where the top-level actor is provided by the library—and each created actor is supervised by its parent. This restriction makes the formation of actor supervision hierarchies implicit and encourages sound design decisions. It should be noted that this also guarantees that actors cannot be orphaned or attached to supervisors from the outside, which might otherwise catch them unawares. In addition, this yields a natural and clean shutdown procedure for (sub-trees of) actor applications.

Seems like a good idea to me.

@cararemixed
Copy link
Contributor Author

I should probably expand on the documentation if Akka is the sort of example we want to follow.

@tarcieri
Copy link
Member

tarcieri commented Jan 8, 2013

Seems good to me ;)

@fujin
Copy link

fujin commented Jan 8, 2013

Yeah, this is cool. I like. I wasn't aware of how Akka works, but we'd been building stuff like this into nearly all of our actor supervision trees anyway. 👍

@PragTob
Copy link

PragTob commented Jan 8, 2013

I like it 👍

@cararemixed
Copy link
Contributor Author

I've tried to provide some comments as guidance for the three behaviors. We can probably update the wiki to reflect this once merged and released if everything looks fine.

@benlangfeld
Copy link
Member

I like this. I would, though, urge caution in releasing it (RC with sufficient time for testing) since it may have unexpected consequences in existing apps.

@cararemixed
Copy link
Contributor Author

We could probably add logging to warns when implicit links and redundant links are added. The question might then be if that should be released separately first or if we simply should make it configurable with the intent to deprecate the old behavior.

@cararemixed
Copy link
Contributor Author

It also seems like Travis is reporting errors that my build doesn't show locally. The test suite might have some timing bugs. Can anyone else reproduce the problems?

@tarcieri
Copy link
Member

tarcieri commented Jan 9, 2013

@strmpnk I'm definitely able to reproduce this sporadically on both JRuby and rbx. Here's the failure:

Failures:

  1) Celluloid::SyncCall aborts with ArgumentError when a method is called with too many arguments
     Failure/Error: actor.should be_alive
       expected alive? to return true, got false

As you presume, the existing tests may make some assumptions about ordering that you're changing. Let me take a look...

@halorgium
Copy link
Contributor

I would be tempted to not split up the #new and #spawn methods.

Would it be better to consider this change along with supervision? See #148.
For me, I would really like a simple way to do supervision and linking as one thing.

@halorgium
Copy link
Contributor

I started a discussion about this at https://groups.google.com/d/msg/celluloid-ruby/Rd2AurcPNpo/xejk2OT3zwEJ

@halorgium halorgium closed this Apr 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants