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 ActorRef an interface or The breaking-just-about-everything PR #762

Merged
merged 3 commits into from
Mar 30, 2015

Conversation

HCanber
Copy link
Contributor

@HCanber HCanber commented Mar 29, 2015

Fixes #480 Make ActorRef an interface
Related to #633 Fix interface naming conventions
and #652 Prefix user-facing interfaces with "I" to match .NET Convention

The current implementation

The hierarchy for an actor ref looks like this:

Akka.Actor.ActorRef                (abstract class)
+ Akka.Actor.InternalActorRef      (abstract class)
|  + Akka.Actor.ActorRefWithCell
|  |  + Akka.Actor.LocalActorRef
|  |  |  + Akka.Actor.RootGuardianActorRef
... and so on

Every actor ref, except Akka.Actor.NoSender, has to inherit from InternalActorRef as the core framework casts actor refs to InternalActorRef to get access to internal members.

The new implementation

This PR introduces two new interfaces: IActorRef and IInternalActorRef. Every concrete class has to implement these two interfaces, but does not otherwise have to inherit from any base classes.

What's been done

The existing abstract class ActorRef has been renamed to ActorRefBase and the new interface IActorRef is used everywhere instead of ActorRef.
The same thing has been done to InternalActorRef: It has been renamed to InternalActorRefBase and the new interface IInternalActorRef is used.

Since IActorRef is an interface the operators == != has been removed.
ActorRef.Nobody and ActorRef.NoSender has been moved to ActorRefs.
Tell(object message) is now an extension method.

The commits and reviewing

This PR consists of three commits (oldest to newest):

  • The first renames ActorRef to ActorRefBase and introduces an interface ActorRef and other things that had to change due to ActorRef being an interface.
  • The second renames InternalActorRef to InternalActorRefBase and introduces a interface InternalActorRef
  • The third only renames the interface ActorRef to IActorRef and InternalActorRef to IInternalActorRef

This should make it a bit easier to review as renaming from ActorRef to IActorRef modifies 206 files and has been lifted out to its own commit.
The first two commits contains the real code changes and needs reviewing.

Breaking changes to the public API

  • Renamed:
    • ActorRef --> IActorRef
    • ActorRef.Nobody --> ActorRefs.Nobody
    • ActorRef.NoSender --> ActorRefs.NoSender
  • ActorRef's operators == and != has been removed. This means all expressions like actorRef1 == actorRef2 must be replaced with Equals(actorRef1, actorRef2)
  • Tell(object message), i.e. the implicit sender overload, has been moved
    to an extension method, and requires using Akka.Actor; to be accessible.
  • Implicit cast from ActorRef to Routee has been replaced with Routee.FromActorRef(actorRef)

HCanber added 3 commits March 29, 2015 12:45
BREAKING CHANGES:
ActorRef == and != has been removed. This means all
actorRef1 == actorRef2 must be replaced with Equals(actorRef1, actorRef2)

Tell(object message), ie the implicit sender overload, has been moved
to an extension method, and requires a "using Akka.Actor;"

ActorRef.NoSender -> ActorRefs.NoSender
ActorRef.Nobody  -> ActorRefs.Nobody

Implicit cast from ActorRef to Routee has been replaced with Routee.FromActorRef(actorRef)

You might need to add a
using Akka.Actor;
to be able to use Tell with implicit sender.
From now on, an ActorRef implementation need to implement
both ActorRef and InternalActorRef interfaces.

Before this was the ActorRef hiearchy. Every ActorRef (except NoSender) had to inherit from InternaActorRef
Akka.Actor.ActorRef
+ Akka.Actor.InternalActorRef
|  + Akka.Actor.ActorRefWithCell
|  |  + Akka.Actor.LocalActorRef
|  |  |  + Akka.Actor.RootGuardianActorRef
|  |  |  + Akka.TestKit.TestActorRef<T>
|  |  + Akka.Actor.RepointableActorRef
... (other classes removed)
+ Akka.Actor.NoSender

Now ActorRef and InternalActorRef are interfaces and we have two new base
classes: ActorRefBase : ActorRef and InternalActorRefBase : ActorRefBase, InternalActorRef and the hierarchy is:
Akka.Actor.ActorRef
 + Akka.Actor.ActorRefBase
 |  + Akka.Actor.InternalActorRefBase
 |  |  + Akka.Actor.ActorRefWithCell
 |  |  |  + Akka.Actor.LocalActorRef
 |  |  |  |  + Akka.Actor.RootGuardianActorRef
... (other classes removed)

However there is no need for ActorRefs to inherit from the base classes.
Implementing InternalActorRef is a requirement though.
@rogeralsing
Copy link
Contributor

cc @Aaronontheweb we need to decide what PR of this and yours we merge first, what cause the least friction?

@Aaronontheweb
Copy link
Member

@HCanber what's the reasoning behind putting Tell into an extension method?

@Aaronontheweb
Copy link
Member

@rogeralsing @HCanber doesn't matter to me which one goes first, but we should do all of the smaller PRs first before we do either of these big ones. Thinking of @jcwrequests's DI one in particular (needs to be rebased on dev.)

Either I way, I volunteer myself to clean up the mess when we try to merge both of these in :p

@Aaronontheweb
Copy link
Member

Actually, we should probably do mine first - I don't think mine has any breaking API changes. @HCanber's can be done safely on top of mine I think.

@jcwrequests
Copy link

@HCanber @rogeralsing @Aaronontheweb if there is anything that I can do with my pull request to help I am certainly open. I am just not sure if I rebase my PR if there will be any problems. In my day job I am usually the only dev committing so I don't run into issues requiring squashing.

@Aaronontheweb
Copy link
Member

@jcwrequests here's what you should do:

  • switch to your dev branch and git fetch upstream then git merge --ff-only upstream/dev
  • switch to your feature branch and then check out a new version of it - git checkout -b feature-branch-rebase
  • do a git rebase dev from that branch. Deal with any merge conflicts.
  • then do a git rebase -i dev and squash all commits except for your first commit.

That way there are no merge conflicts with dev and it's easy to review. Does that make sense?

@HCanber
Copy link
Contributor Author

HCanber commented Mar 29, 2015

@HCanber what's the reasoning behind putting Tell into an extension method?

It's only the Tell with implicit sender that's been moved, i.e. Tell(message). The other Tell, i.e. Tell(message, sender) exists in ICanTell interface.

So the alternatives would have been:
a. Move it to ICanTell. Which would have been a breaking change, and means it would have had to be implemented in a few more classes.
b. Add it to IActorRef. Since we're finding more and more problems with having implicit context & sender, I didn't want to "legitimize" it by putting it in the interface when we probably want to remove it completely in the next major version.

And, since it can be implemented as a extension method, and it really is a "convenience" method, I thought that was the best alternative.

@jcwrequests You can find more information on how to handle these kind of issues here: Contributing.md
Sometimes when I'm having a large PR that needs to be rebased, I do not rebase at all. Instead I rename my branch, and create a new branch off dev with the same name as the one that was renamed, and then I cherry pick all the commits. That way you still have everything the way they were before you started, and can easily switch back and forth to compare.
Don't forget you need to force push after you've rebased a pushed branch.

@Aaronontheweb
Copy link
Member

@HCanber thanks for the explanation - that makes perfect sense.

@jcwrequests
Copy link

@HCanber thanks for the info.

@Aaronontheweb Aaronontheweb mentioned this pull request Mar 30, 2015
@Aaronontheweb Aaronontheweb merged commit 6ca2095 into akkadotnet:dev Mar 30, 2015
Aaronontheweb added a commit that referenced this pull request Mar 30, 2015
@HCanber HCanber deleted the actorref-interface-480 branch March 31, 2015 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ActorRef an interface
4 participants