DependencyInjection Tests?

[rogeralsing]

Don't we have any tests for the dependency injection parts?
if not, we should make some asap.

Related #941

Seems like there might be some conflicts between stashing and DI plugins.

[Aaronontheweb]

Yes, we should absolutely add some.

@Danthar, @jcwrequests, @nvivo - do any of you have interest in working on this? If so, how can we best support you?

[jcwrequests]

@Aaronontheweb I can certainly pitch in. My only thing is do you have any documentation on what testing framework you use and does this need to follow the Akka Testkit and if so what documentation do you have on best practices doing so.

[jcwrequests]

@rogeralsing @Aaronontheweb The reason I asked my question is that all Akka tests are around a spec convention using xunit. Is this a requirement? Do you have any guidelines? Should the tests be located with the contrib code? What specs do you want and test for? @Danthar @nvivo I am more then willing to pitch in just let me know where I can help.

[Aaronontheweb]

Great questions @jcwrequests!

1. Yep, we are standardized on XUnit. About to be XUnit 2.0 when I pull in Test runner improvements. Upgrade to XUnit 2.0 #888
2. We have little documentation on the Akka TestKit unfortunately, although that's part of Reorganization of Akka.NET docs for v1 getakka.net#7 - so some of what you'll need to do is take a look at existing specs and work backwards from there.
3. Best recommendation I can give is to make extensive use of the TestActor - he's your buddy for being able to see what messages actually get delivered. You basically have to design your tests so the TestActor ends up with the correct message with the correct content within a given timeframe.

[jcwrequests]

@Aaronontheweb I guess I could look at the Props spec and go from there since they are closely related.

[Danthar]

If you google on akka testkit you will find some blog posts by @HCanber explaining some stuff about the testkit. I found them to be a great intro if you need one.

-----Original Message-----
From: "jcwrequests" [email protected]
Sent: ‎07/‎05/‎2015 02:53
To: "akkadotnet/akka.net" [email protected]
Cc: "Arjen Smits" [email protected]
Subject: Re: [akka.net] DependencyInjection Tests? (#942)

@Aaronontheweb I guess I could look at the Props spec and go from there since they are closely related.
—
Reply to this email directly or view it on GitHub.

[rogeralsing]

We should port this http://doc.akka.io/docs/akka/snapshot/scala/testing.html

[rogeralsing]

BTW. regarding tests for DI. we should totally do a spec for DI and then let different DI impls inherit that and run the same tests using the same types and actors.

To verify that each impl behaves the same way.

[jcwrequests]

@rogeralsing I agree. Where do we begin?

[jcwrequests]

@Danthar Thanks I found the post. @rogeralsing @Aaronontheweb I will proceed with coming up with a DIExt Core test project and put it under contrib along with all the rest of the DI stuff. As I commit stuff to my personal repo I will link back here for feedback.

[jcwrequests]

I have added a new project on my repo and will start getting it all setup tonight.

[jcwrequests]

Just documented Aaron's comments from gitter
I think so - basically try to cover all of the weird edge cases where DI can get used
on actors with stashes, etc
on actors that restart
child actors that need DI

[jcwrequests]

Here is a link https://github.com/jcwrequests/akka.net/tree/DI-Core-Testing/src/contrib/dependencyInjection/Tests/Akka.DI.Core.Tests
to what I have so far. @HCanber @Aaronontheweb @rogeralsing @nvivo @Danthar Can you take a look and make sure everything looks good so far. If everyone could start sending spec ideas my way I will be sure to add them to my project. If you have any questions or comments that you need to talk about just hit me up on gitter private chat. Thanks.

[Danthar]

For spec idea's. I think it could be really simple. The DI integration only goes as deep as the Props object.
Because that is basically what the DI does. It creates a Props object and thats it. Akka takes care of the rest.
So until such time that the DI integration goes beyond that. We only need to verify that the various ways of DI usage always result in a valid and proper Props instance.

So that means if you have that down. You don't need to write specs for all the various Actor invariants as they will be covered by existing tests.

[rogeralsing]

@Danthar there is a bit more to it e.g. when actor terminates it can callback to the container to dispose/release dependencies with different lifecycle managers.
Right?

[Danthar]

Ah your right. The DIActorProducer does let the DI container instantiate the actor and also calls Release.

But even then all of Akka's initialisation is based on the type definition in the Props instance. So if you verify that a correct Props instance is created you still have alot of scenario's covered. Right?

[jcwrequests]

@nvivo @Danthar @akkadotnet/owners So how do you want and spec this out? Has anyone had a chance to check out my shell project just to be sure at least from a reference point of view everything is correct? I will see what I can come up with and just add it to the project. Just send me private gitter messages with any questions. Cheers.

[Danthar]

Project structure looks good to me. However I would drop the Tests subfolder and follow the same folder structure as in the rest of the project.
So you would have a
Akka.DI.Core and
Akka.DI.Core.Tests
In the same folder.

[jcwrequests]

@Danthar Thanks. I had a few moments just now at lunch and made the changes.

[jcwrequests]

@nvivo @Danthar @akkadotnet/owners I have continued with project. Went through some strange issues with building the project with XUnit and the old Xunit test runner but got that resolved. I also started adding some test cases. Until I get more feedback I will at least implement all of the prop tests then move onto to some of the things @Aaronontheweb was saying. @rogeralsing I have not had a chance to look at http://doc.akka.io/docs/akka/snapshot/scala/testing.html but it's on my todo. I looking forward to the feedback and getting this done PR done.

[jcwrequests]

I found something interesting and am not sure if it's worth extending DI for this case but I will leave it you guys. Here is the example:

        var strategy = new OneForOneStrategy(_ => Directive.Stop);
        using (var system = ActorSystem.Create("MySystem"))
        {
            var propsResolver =
                new TestActorResolver(system);

            var props = propsResolver.Create<DITestActor>(strategy);

            Assert.Equal(strategy, props.SupervisorStrategy);
        }

Thanks.

[jcwrequests]

@nvivo @Danthar @akkadotnet/owners Still looking for feedback. Also what you guys think about this PR #966? Since you guys know Akka best could someone explain the benefit and the particular problem it solves?

[nvivo]

As we discussed previously, I don't like the current API of DI, it's one of those places where it would be a good idea to diverge from jvm, so I don't have much to say about which scenarios it should be tested against.

I see the benefit from that proposal in that currently its quite hard to use DI and add settings to props. The requirement of explicitly calling the PropsResolver sometimes makes it a pain to use.

But that PR #966 seems to just mask the issue a little bit. IMO, the solution should be to get rid of the DI() extension and allow the existing api of Props and ActorOf to take advantage of DI in a transparent way, so we don't need to duplicate all the api.

[Aaronontheweb]

I'll do a full review of this on Thursday / Friday. Don't have time to give this the attention it deserves until then.

[nvivo]

You said 2 things here that caught my attention:

"DI is the exception for creating actors, not the rule. It should be used rarely."
(...)
Having Sys.DI.Props() is fairly transparent

This only makes sense is if you don't want to use DI. When you use DI, you can't make it the exception and use it rarely, it just doesn't work that way. And this is the problem with the current API in my opinion, it's made by people that don't like DI in a way to make it painful to use.

The issue is not typing 4 extra characters, but the fact that you have to decide when you need it or not. If I have some code with Props.Create<Actor>() and I start requiring a dependency in that actor, I need to go everywhere I start that actor and change my code to be Di().Props<Actor>() everywhere.

The only way this change is reasonable is if you don't actually use DI and think of it as a helper you call sometimes when you don't want to type much - that's not the idea. The whole idea of DI is to avoid changes in application code when dependencies change.

The solution you said yourself:

now that I think about it - this is just a detail of how the Props is created, isn't it?

So, if it's all about creating Props, what I end up doing is to have my own Props factory that abstract the decision in a way that makes sense:

1. Optionally call PropsFactory.SetProducer(...) to set the DI framework

2. PropsFactory.Create<Foo>() will:

   a. If DI is registered, call the DI producer to create the Props
   b. If it's not registered, call Props.Create as usual

3. PropsFactory.Create<Foo>(a, b, c) and PropsFactory.Create<Foo>(new Foo(a, b, c)) will always delegate to Props.Create with the same signatures.

This way I have a single API to code for and I never have to change application code if I add dependencies. This is not forcing DI anywhere, and is not affecting remote deployment in any way - because I can still create props manually if I need to.

DI is used only when I don't specify parameters to Props, which makes it clear in code I don't care about dependencies. And if I want all my props to be remote deployable, a different producer can be used that always produce serializable props.

I'm not saying this is perfect, but beats the hell out of the current API and makes it usable in real world.

[rkuhn]

The impression I get from pondering this discussion is that we are conflating two things that should rather be kept separate: creating an Actor means spawning a new isolated and possibly distributed independent agent of computation, this act is about delegating responsibility. The parent Actor’s intention and responsibility is to define exactly the task that is to be delegated. If it so happens that the execution of the task requires a dependency that is independent of the parent—meaning that the parent does not or cannot care about the details of this dependency—then it would be best to keep this dependency completely out of the Actor creation, i.e. to keep it out of the Props. The exemplary database connection has no business in the Actor infrastructure if its selection is not the parent’s responsibility, instead the constructor of the Actor should perform the necessary lookups to obtain all dependencies before beginning to perform its designated function.

This thought was triggered by @nvivo’s remark that the invocation sites of ActorOf should not care about those dependencies that are managed externally. If the parent needs to explicitly choose whether or not to supply constructor arguments then this principle is already violated.

Is a solution conceivable that completely hides the dependencies within the Actor such that the parent (and Akka) does not need to know about them at all?

Concretely, I’m thinking of using the DI extension within PreStart to obtain either the individual dependencies or to have a small (and shallow) dependency container object be injected by the DI framework. This would leave the orthogonal concerns of “unit of computation” (Actor) and “managed dependencies” unentangled and unencumbered by each other. In other words, I see no reason why the Actor instance also needs to be the business logic instance; as so often, it seems favorable to prefer composition over inheritance.

[jcwrequests]

I agree with Natan. If my memory serves me correctly NServiceBus handled the problem the same way as Natan suggests. At least in the open source version. Still I would recommend talking to Mark Seemann if I had a twitter account I would do it myself. Sorry for being annoying about that but he did write the book. :)

https://github.com/Particular/NServiceBus/tree/develop/src/NServiceBus.Core/ObjectBuilder

[nvivo]

The parent Actor’s intention and responsibility is to define exactly the task that is to be delegated (...) If it so happens that the execution of the task requires a dependency that is independent of the parent (...) then it would be best (...) to keep it out of the Props

@rkuhn, If I understand you correctly, you mean that since the only parameters in an actor constructor should be the ones related to the task it needs to accomplish, and the parent MUST know exactly which task the child should perform, then Props should always be explicitly declared. Is that right?

And in that case, there would be no place for DI to create Props. That raises the question on why do the current DI extensions do exactly that?

You made good points about separation of concerns. Maybe you can enlighten me, but I don't see how could this be enforced using the semantics you described. Separating what is a "dependency of the code" and a "dependency of the task that code should perform" is quite hard to tell and the answer will in most cases be blurry.

Another thing that concerns me is that constructor injection is a well known and well supported pattern, it works fine. Diverging from it in favor of a service locator in PreStart would confuse a lot of people, and the only benefit would be some "sense of accomplishment" at the cost of flexibility and control.

Is a solution conceivable that completely hides the dependencies within the Actor such that the parent (and Akka) does not need to know about them at all?

I think so. I'm achieving this today by separating concerns this way:

• for "service actors" - that is, always on actors that are there to handle requests, constructors usually require parameters that can be resolved by DI
• for "worker actors" - that is, actors created for a specific task (and that includes anything remotely deployable), constructors usually require only some data that is easily serializable like strings, actor refs, configuration, etc - and for those I never use any DI and always create props manually

So, whenever I need DI, it's always hidden. When I don't need it, I just don't use it. To me this works fine today, with the exception that I have to ignore the official DI methods and roll my own which bothers me a little bit but doesn't hurt me.

I like the idea of separating concerns, and I'd support moves to improve DI. I just I don't know if exposing the DI kernel on PreStart would be a good thing. Maybe some of the interested folks like @jcwrequests, @Danthar and @thomaslazar can post their opinions.

[Aaronontheweb]

The parent Actor’s intention and responsibility is to define exactly the task that is to be delegated. If it so happens that the execution of the task requires a dependency that is independent of the parent—meaning that the parent does not or cannot care about the details of this dependency—then it would be best to keep this dependency completely out of the Actor creation, i.e. to keep it out of the Props. The exemplary database connection has no business in the Actor infrastructure if its selection is not the parent’s responsibility, instead the constructor of the Actor should perform the necessary lookups to obtain all dependencies before beginning to perform its designated function.

I agree with this in concept, but in practice I think this is more of an aspirational standard. When a parent provides constructor arguments to the Props of a child it's delegating some task to, what's the test for distinguishing whether or not those arguments are really independent of the actor's ability to carry out that task or not? Isn't that just an implementation detail that's ultimately up to the user and not the framework? Developers are going to write what they're going to write.

That way I've personally used DI for database connections is as you describe - I resolve them using the DI container inside the actor's OnReceive or PreStart methods, because the lifecycle of the actor is almost always going to be longer than that of the DB connection (FWIW: connections are usually implemented as transient, pooled objects that don't hold onto physical TCP connections and are recycled at configurable time intervals.)

Concretely, I’m thinking of using the DI extension within PreStart to obtain either the individual dependencies or to have a small (and shallow) dependency container object be injected by the DI framework. This would leave the orthogonal concerns of “unit of computation” (Actor) and “managed dependencies” unentangled and unencumbered by each other. In other words, I see no reason why the Actor instance also needs to be the business logic instance; as so often, it seems favorable to prefer composition over inheritance.

Practically speaking, I could see people being resistant to this because it's different than how most .NET developers consume DI containers today. Constructor injection is the typical way of doing it and has the added benefit of being easier to test (you don't need to "mock" a DI container as part of your tests this way.)

This only makes sense is if you don't want to use DI. When you use DI, you can't make it the exception and use it rarely, it just doesn't work that way.

This hasn't been true in my experience; Akka.NET is fundamentally different from ASP.NET MVC / NancyFX in the sense that you're directly responsible for creating DI-ed objects in Akka.NET, whereas a factory does it for you on invocation in those other contexts. There's nothing about the DI or IoC patterns, AFAIK, that requires containers usage to be an invisible detail of the calling system.

I actually like the way you made DI transparent inside PropsFactory - that's a good idea, but doing that inside the core library would be tricky. We'd have to rewrite the way Props<TActor> currently works.

That's why I think we're kind of stuck using an extension method or a factory to make it explicit. Unless we added the DIExtension class to the core library, it's tough to bolt on these capabilities through an external module in a way that's this transparent.

The way Akka.Remote and Akka.Cluster achieve their transparent actor creation changes is through overriding the IActorRefProvider implementation, but doing that isn't an option for DI - you'd eliminate the ability to use both DI + Remoting or DI + Clustering.

[jcwrequests]

That is basically the same conclusion I came to as well. Nothing is perfect and you will never be able to please everyone.The way it currently works is a very practical. Perhaps it might be interesting to create an experimental Akka for testing new ideas like having DI be a core principal. Again I know everyone is busy but it could be interesting.

[stefansedich]

@jcwrequests one thing if we are making some changes that I really dislike at the moment and that confused me the other day.

In the constructor for say the AutofacDependencyResolver we wire up the DI extension, so my initial config looked like:

var system = ActorSystem.Create("import", config);
var resolver = new AutoFacDependencyResolver(container, system);
system.AddDependencyResolver(resolver);

Looking at the doco I did not see much mention of setting the resolver, and when I went digging I realized that what I was doing is not really required but you could instead do something like:

var system = ActorSystem.Create("import", config);
new AutoFacDependencyResolver(container, system);

Could just be me but that just reads and feels strange, what would we think of the ability to just call a static Init method that could wire up the resolver:

var system = ActorSystem.Create("import", config);
AutoFacDependencyResolver.Initialize(container, system);

Just "feels" nicer to me, feel free to throw the suggestion away :).

Cheers

[nvivo]

I think it's more than the syntax of creating the dependency resolver. Currently, the way to use DI is:

ActorSystem.DI().Props<Foo>() or
Context.DI().Props<Foo>()

The real issue to me is that the DI system has nothing to do with the ActorSystem or with the actor context, but is tied there for reasons that are no longer valid.

If Props can be created independently of actor systems and a Props instance can used with any actor system, the whole idea that DI should be an actor system extension should be reexamined.

[stefansedich]

Ok some updates,

I have pushed a bunch of PRs to @Aaronontheweb fork that fixes our issues, what was done for the various containers:

1. Windsor by default registers dependencies using a Singleton lifetime, changed test setup to register as a transient instance.
2. NInject was changed to use a child container so that we could handle deterministic dispose of all resolved dependencies for that particular actor instance.
3. Structuremap has a tiny bug in the Release code where we were disposing the main container and not the created child container.
4. Unity was changed to use child container for the actor instance, and also requires that you register using a HierarchicalLifetimeManager, it seems as though Unity will not call Dispose when using the default Transient lifetime manager. (http://www.tomdupont.net/2013/12/undestanding-unity-lifetime-managers.html) I am not overly familiar with Unity though so someone might want to comment here.

It looks like what we should do is update the documentation for Windsor to warn about registering using a transient lifetime, and perhaps add some info the the Unity documentation. It looks like Windsor, Structuremap and NInject should just work as expected with happy defaults.

Cheers

[jcwrequests]

@stefanedich Nice!

[Aaronontheweb]

I'll leave this and #1139 open for a bit to entertain the discussion around getting DI as it is right now to work as expected and to get clear on what those expectations are for the next couple of days.

Spent some time with @nvivo today discussing some possible options for making DI more transparent in Akka.NET in the future, which would require something much more radical than what's currently being proposed. Any additional discussions about that should be moved to a new thread.

The immediate goal of #1139 is just to spell out the assumptions we make about using DI in the context of actors and to test each implementation against those assumptions with the future benefit of regression testing.

TL;DR; I just wanted to improve test coverage and behavioral consistency for DI in the short run, and we should be pushing the bug fixes and recommended container configuration settings out there with great haste.

[stefansedich]

For those playing at home I just merged in the PR that gets all of @Aaronontheweb DI tests passing, my next goal is to update the DI documentation spelling out a few things for the various containers that require specific configuration when registering actors and add some more information to help anyone starting out with Akka.Net and DI to ensure they have the smoothest possible experience.

[nvivo]

@Aaronontheweb, is there a specific issue you will create to discuss the DI stuff, or can I create one?

[Aaronontheweb]

@nvivo I think we should have a discussion issue around how to make DI more transparent. including the stuff you and I talked about on Skype (maybe merging Akka.DI.Core into the main assembly.)

[Aaronontheweb]

cc @stefansedich still planning on documenting all of the recommending scoping / config options for each DI container in our docs? Once that's done we can close this issue out.

[Aaronontheweb]

Did this a long time ago, and moved everything out of the core repo.