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

Added Extensions methods to ActorSystem and ActorContext to make DI more accessible #966

Merged
merged 3 commits into from
May 19, 2015

Conversation

thomaslazar
Copy link

Issue #965

Added a new class DIActorSystemAdapter.cs containing methods to extend the ActorSystem with an ActorOf (return an IActorRef similar to the DIActorContextApapter method) and Props (returns a Props object that can be further configure) method that use DI to create the Actor dependencies.

Added a Props method to DIActorContextAdapter.cs similar to the DIActorSystemAdapter class one.

Added a DI() extension method for ActorSystem in Extensions.cs that uses the DIActorSystemAdapter class.

With these changes it is now possible to create Props and Actors on the ActorSystem level without having to use a IDependencyResolver implementation available or through system.GetExtensions<DIExt>().Props call.

// get an Actor through DI directly from the ActorSystem
var worker = system.DI().ActorOf<TypedWorker>("worker");

// get Props through DI directly from the ActorSystem
var router = system.ActorOf(system.DI().Props<TypedWorker>().WithRouter(FromConfig.Instance), "router");

It is also possible to now create Props from within an Actor through DI and configure it further.

// get Props from within an Actor using the DI extension
var router = Context.ActorOf(Context.DI().Props<TypedWorker>().WithRouter(FromConfig.Instance), "router");

@nvivo
Copy link
Contributor

nvivo commented May 12, 2015

I completely agree we shouldn't require calling IDependencyResolver, but I think this is more a workaround then a solution.

We should allow Props.Create to use a DI in the background, so system/context.ActorOf and Props.Create can be used without api duplication.

This would require some changes to the di implementation, but would make things much cleaner. This Context.ActorOf(Context.DI().Props... is getting cumbersome.

@Aaronontheweb
Copy link
Member

Don't merge this PR. I'd like to review it and the other conversations going on around DI, but I'm not available to do it until Thursday / Friday. It's come up enough times where I want to understand what the actual issue is.

@thomaslazar
Copy link
Author

@nvivo I agree with it getting cumbersome and I wish for a more and better integrated way to do this and making it so Prop.Create() uses DI internally when a IDependencyResolver is hooked up in the ActorSystem would make it completely nice and transparent (disregarding the situation where you perhaps sometimes don't want the DI to be used when using Prop.Create(), but i don't have currently a concrete reason why you would not want to use the container on some occasions).

I was just working under the assumption that the way it is currently handled with creating child actors through DI (the example given in the documentation) would be the way to go for now. And adding to that is a fairly easy matter. This way it can be made fairly usable and then later deprecated and also rather easily refactored to the future proper way.

The biggest problem currently is if you don't use Akka.net as a first class citizen (building your App on top of it) and just pass around a reference to the ActorSystem to use in places that can benefit from it (I have it currently injected into Apache Thrift handlers) then you have to always use system.GetExtension<DIExt>().Props(...) to create the props through DI which is way more cumbersome then a couple of extra extensions or pass along an IDependencyResolver to do it with which would pollute your API.

The current DI examples are all just one shot console apps that create the ActorSystem, show how to use the IDependencyResolver and then dispose of it when you're done.

But how would you integrate it into a ASP.NET deployment scenario mentioned in the documentation? There you would probably create your Container as a static field in Global.asax, then add the ActorSystem as singleton to the container, hook both up with a DependencyResolver and then you can have the Container resolve your IHttpHandler instances or MVC Controllers and have it inject the ActorSystem into those for it to be used within.

That's btw the exact scenario i currently have to use Akka.net in and that's why i'm struggling so much with it.

@nvivo
Copy link
Contributor

nvivo commented May 12, 2015

@thomaslazar, you're assumption is correct. The way it works now is based on the existing API from JVM. So I'm using this as a hook to discuss a better API for DI, independently of the merge of this PR, as I'm not the only one that finds the current API odd and hard to use.

It seems to me DI in Akka was an afterthought. Any new framework written today takes that as a core concept. We should take into consideration that Akka is 5 years old, and at this point there are lots of legacy components that could be done better today. The Typesafe team is constantly reinventing Akka with better ideas and throwing away old assumptions, and I don't see why we can't do that too instead of just copying everything they do. Even being a port, we should be allowed to improve things every now and then.

IMO, the whole problem is this desire to expose DI explicitly, while it should be implicit. If I have this code today:

public class A : ReceiveActor {
     public A() { ... }
}

and change to this:

public class A: ReceiveActor {
    public A(IDependency dep) {...}
}

It shouldn't cause changes to the application code. The app should still work with system.ActorOf<A>() or Context.ActorOf<A>() inside actors.

If we look at the Props today, it's already a kind of manual DI system that builds instances based on dependencies registered in its constructor. Props already uses a IIndirectActorProducer to actually build instances, so it seems it's a small step to allow DI to work transparently.

  1. We could have a new overload here:
public virtual ActorBase NewActor(IIndirectActorProducer producer) { ... }

This would be called by the ActorSystem and actors when a DI was registered, passing the DI producer automatically.

To solve the problem of creating Props using DI (the purpose of this PR), the IActorRefFactory could include a Props method similar to the static Props.Create overloads, so both ActorSystems and actors would allow props creation using DI:

// from actorSystem
var props = system.Props<ActorWithDependency>();

// from inside an actor
var props = Context.Props<ActorWithDependency>();

Considering that ActorOf<T> is just a shortcut for ActorOf(Props.Create<T>), modifying it to be ActorOf(factory.Props<T>()) should be simple.

Also, note this API is independent of DI. It works exactly as Props.Create by default.

  1. Or we could allow the producer to be set in the props directly, instead of using an actorsystem extension.
Props.DefaultProducer = someDIproducer;

This would be much simpler, but would set it statically for the whole system. I'm not sure if this would cause any problems though, as I doubt anyone using a DI container would be using 2 in the same app.

I'm up for any other solution, as long as we don't have to expose to the application that a DI system is being used like today.

cc @jcwrequests

@nvivo nvivo mentioned this pull request May 12, 2015
@thomaslazar
Copy link
Author

@nvivo I am all in for your idea to make DI usage more transparent. My proposal is really just a quick fix/addition based on already implemented code to at least make that stuff a little less of a pain.

The other thing is I'm just starting out with Akka.net and the Akka way of life in general. So extensions I can handle. Changing Akka.net on a level you proposing I currently can't ;)

@Aaronontheweb
Copy link
Member

Had a conversation with @rkuhn about DI this morning. What's in this PR is already pretty close to what he and I discussed. Will share some details tomorrow but I think we're close to a working solution that should make DI easier to use.

@jcwrequests
Copy link

@Aaronontheweb Sounds good to me.

@rogeralsing
Copy link
Contributor

@Aaronontheweb can you share any details on what you and Roland talked about?

@Aaronontheweb
Copy link
Member

sorry not getting around to this sooner!

The high-level points are this:

  • All of the DI methods should always return a usable Props object. You shouldn't have an ActorOf method directly available through the DI plugins.
  • The syntax in this PR:
 var router = system.ActorOf(system.DI().Props<TypedWorker>().WithRouter(FromConfig.Instance), "router1");

Should be supported.

So if we can clean this PR up to remove the additional ActorOf methods off of the DI object and just focus on returning props instead we should be good to go.

@nvivo
Copy link
Contributor

nvivo commented May 18, 2015

It makes sense to make DI return props. But removing this now will break the 1.0 api.

@Danthar
Copy link
Member

Danthar commented May 18, 2015

@Aaronontheweb is this the net result of your conversion with @rkuhn ? Or are there more fundamental changes for the DI system coming up ?

@Aaronontheweb
Copy link
Member

@Danthar that's the net result of the conversation. There's some other interesting ideas that came up (remotely deploying an actor who gets DI'd on the remote system.)

But removing this now will break the 1.0 api.

Meant removing them from THIS PR. For everything that's already in the 1.0 API, we have the follow the normal procedure. Mark as obsolete and then remove in the next major version.

@thomaslazar
Copy link
Author

I can make the necessary changes to the PR to only keep the Props creating methods. Will see that i can do these changes tomorrow.
@Aaronontheweb shall i just mark the already existing ActorOf method as obsolete too when i'm doing the other changes?

But this doesn't solve one problem i'm currently having. You can't add additional arguments to the Prop creating method when using DI.

Without DI you can do something like this:
Props.Create(() => new MyDoSomethingWithDBActor(_sessionFactory, someId))
If you want to create a bunch of those actors all with different Ids you can just create them.

With DI you'd probably want that _sessionFactory (i'm working with NHibernate currently) be injected but the second parameter still be "variable". I know you can pass along an IDictionary instance to Castle Windsors Resolve() method and have that be used for dependencies that the container can't serve on his own. Don't really know how this works with other DI frameworks though.

It would be nice to be able to write something like this:
system.DI().Props<MyDoSomethingWithDBActor>(new Dictionary<string,object>{{"Id", 5},{"someOtherArg","blah"}})
But this would have to be so generic so it can be used with all the supported DI frameworks. Don't really know how that could be done yet though.

This example looks rather crude though with the dictionary.

Castle Windsor has a nice extension method for IDictionary that lets you add items to a dictionary with a fluent syntax.
It then could look like this:
system.DI().Props<MyDoSomethingWithDBActor>(new Dictionary<string,object>().Insert("Id", 5).Insert("someOtherArg", "blah"))

http://docs.castleproject.org/Windsor.Arguments.ashx#_Insert_extension_method_4

This extension could be added to the DI Core project.

@Aaronontheweb
Copy link
Member

@thomaslazar

@Aaronontheweb shall i just mark the already existing ActorOf method as obsolete too when i'm doing the other changes?

Yes, that would be great - thank you!

You can't add additional arguments to the Prop creating method when using DI.

Mixing and matching Props arguments with DependencyInjection... Doing that in a generic way that works across all supported containers might be hairy.

In my opinion, I'd try to tackle that problem in its own Issue + PR and have a proper design discussion about it first. Whatever we come up with for solving that problem needs to work across all of the supported DI containers.

Would that work?

@thomaslazar
Copy link
Author

That will do fine. I will get on cleaning up the PR tomorrow and add another Issue regarding the other stuff.

@nvivo
Copy link
Contributor

nvivo commented May 18, 2015

Just to get this straight:

  1. Wasn't the current design based on the JVM implementation? Is the JVM deprecating the api on the next version as well?

  2. And is this new design is being implemented by typesafe on scala akka too?

@Aaronontheweb
Copy link
Member

@nvivo

The .NET implementation is divergent from JVM's in a few respects. The JVM has the benefit of the implicit keyword which makes their DI stuff fairly invisible to the caller. The CLR has to be explicit. I haven't reviewed any of the JVM code - this is just a rehash of the conversation I had with Roland, where I showed him the code in this PR.

We took some liberties with the API and added the ActorOf convenience methods in a couple of areas where we shouldn't have. We're bringing our API back inline with the JVM's now. Basically we shouldn't try to hide Props, even in the name of being convenient, as it ends up being a limiting factor in a lot of scenarios (like putting a router on top of DI'd actors.)

So this isn't a "new" design - it's a minor change to our current one.

@rkuhn
Copy link

rkuhn commented May 19, 2015

Thanks for the discussion and for listening to the advice! To fill in some background, we have arrived at this solution (with just two actorOf variants—with and without a name) due to painful previous experiences. In Akka 1.x there was a plethora of actorOf convenience methods and that poses quite some challenges when it comes to user education as well as library evolution and maintenance. Therefore we switched to a scheme where all the variability is encapsulated in a dedicated type (Props) and the main API method is meant to remain unchanged “forever”.

Concerning DI the two CLR and the JVM have rather different ecosystems so we might not see full convergence on the design and feature set of exposing DI in Akka / Akka.NET, but I think that is absolutely okay. The beautiful thing about focusing entirely on Props is that the difference will be encapsulated within this bounded area.

@nvivo
Copy link
Contributor

nvivo commented May 19, 2015

@rkuhn Thanks for your time and explanation.

I agree it's a great idea to focus on Props.

Now, I really don't care much about convenience methods of ActorOf. What bothers me is the fact that currently DI is explicit in user code. Things like Context.DI().Props(...) instead of just Context.Props(...) requires changes to user code if you start using DI, while DI should be more transparent.

If I my code has Props<Foo> and now Foo requires a dependency, I shouldn't be required to change my entire application to explicitly call DI to create that props.

I have been looking at the Java implementation, but couldn't find much about it. Do you have any thoughts on that?

@rkuhn
Copy link

rkuhn commented May 19, 2015

@nvivo My personal opinion is that all essential aspects should be visible—if an essential aspect is hidden from view then that counts as “magic”, that’s exactly how illusionists operate. The fact that Props<Foo> needs more information should therefore not be hidden in my opinion, it is an extension or configuration point that should be visible in the code. And the fact that these dependencies are injected and therefore not picked by the calling code directly is also an interesting fact that should be documented, but again that might just be my opinion.

If there is a universal and ubiquitous DI mechanism on the CLR that can always be required to exist then it might make sense to teach Props how to invoke it, meaning that there could be convenient ways of marking the references that will be injected. But if DI is to remain flexible and non-opinionated then I don’t see a way around invoking it explicitly. Everything else would amount to creating another DI framework within Akka.NET.

@nvivo
Copy link
Contributor

nvivo commented May 19, 2015

Thanks @rkuhn. That's the kind of answer I was looking for.

@stefansedich
Copy link
Contributor

I agree with @rkuhn here,

Personally I prefer that once I use DI for a top level actor that using it for child actors becomes an explicit choice, I already have many cases where I want to control the creation of the child actor I do not want it to magically continue to use DI because I created the parent using DI as this is not always what I want to do.

@nvivo
Copy link
Contributor

nvivo commented May 19, 2015

@stefansedich Just to make it clear, it's not about one or the other. You can always create props manually if you want, and what I said don't prevent that in any way. My view was simply that Akka.NET should be more like the rest of the platform, and that would make it easier for people to use it.

There are things that are personal preferences and beliefs, and have nothing to do with what you can or cannot do. I don't agree with the choices made, but I'm glad to know what is the reasoning behind this architecture, as I didn't see any before.

@stefansedich
Copy link
Contributor

@nvivo

What do you mean with "more like the rest of the platform"?
On 19 May 2015 9:30 pm, "Natan Vivo" notifications@github.com wrote:

@stefansedich https://github.com/stefansedich Just to make it clear,
it's not about one or the other. You can always create props manually if
you want, and what I said don't prevent that in any way. My view was simply
that Akka.NET should be more like the rest of the platform, and that would
make it easier for people to use it.

There are things that are personal preferences and beliefs, and have
nothing to do with what you can or cannot do. I don't agree with the
choices made, but I'm glad to know what is the reasoning behind this
architecture, as I didn't see any before.


Reply to this email directly or view it on GitHub
#966 (comment).

@nvivo
Copy link
Contributor

nvivo commented May 19, 2015

@stefansedich In DNX (and this includes the normal .NET 4.6 about to be released), DI is a core concept, not an add-on.

From the docs: "Core Concepts: Dependency Injection through the entire stack. DI is a core part of the KRuntime, and all libraries we build on top of it."

This makes DI automatically available and transparent to any new official .NET framework, including ASP.NET MVC, Entity Framework, SignalR, OWIN middlewares and even console apps. NancyFX does it out of the box too.

But again, this is all about personal preferences and I can understand that.

@nvivo
Copy link
Contributor

nvivo commented May 19, 2015

Just want to make it clear about the previous comment: the point is that no library requires a different syntax to use DI. This can be achieved in Akka and doesn't prevent any scenarios or creates any technical issues, as it doesn't in any other framework. DI doesn't prevent you from manually creating objects, it only makes the default path to not have to. This is the direction the platform is going.

It's only about preferences, not limitations.

- removed ActorOf method
- added non generic Props method
DIActorContextAdapter.cs
- obsoleted ActorOf method
- added non generic Props method
Examples
- updated Examples to use the new syntax
Readme.md
- updated Examples with new syntax
@thomaslazar
Copy link
Author

Cleaned up the pull request.
I added non generic versions of the Props methods, removed/obsoleted the ActorOf methods and updated the Examples as well as the Readme to reflect the new changes.

@nvivo
Copy link
Contributor

nvivo commented May 19, 2015

@thomaslazar +300 for DIActorSystemAdapter!

@Aaronontheweb
Copy link
Member

Great job @thomaslazar. This is great!

Aaronontheweb added a commit that referenced this pull request May 19, 2015
Added Extensions methods to ActorSystem and ActorContext to make DI more accessible
@Aaronontheweb Aaronontheweb merged commit 85d3d0c into akkadotnet:dev May 19, 2015
@jcwrequests
Copy link

@thomaslazar Thumbs Up Here.

@thomaslazar thomaslazar deleted the di_extension branch May 29, 2015 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants