ServiceProviderProps and Props Changes #4858
Conversation
…ctorProducer dependencies more explicit
Aaronontheweb
changed the title
|
Should be good to go now. |
|
Looks like these changes blew up Akka.Cluster.Sharding.... |
…ronontheweb/akka.net into fix/4853-ServiceProviderProps
| ///// <typeparam name="T">The type of actor to instantiate.</typeparam> | ||
| ///// <param name="producer">The delegate used to create a new instance of your actor type.</param> | ||
| ///// <returns>A new <see cref="Props"/> instance which uses DI internally.</returns> | ||
| //public Props Props<T>(Func<IServiceProvider, T> producer) where T : ActorBase |
There was a problem hiding this comment.
Commented out method - just removed it.
| @@ -74,7 +59,7 @@ public static ServiceProvider For(ActorSystem actorSystem) | |||
| /// <returns>A new <see cref="Akka.Actor.Props"/> instance which uses DI internally.</returns> | |||
| public Props Props<T>(params object[] args) where T : ActorBase | |||
| { | |||
| return new ServiceProviderProps<T>(Provider, args); | |||
| return Akka.Actor.Props.CreateBy(new ServiceProviderActorProducer<T>(Provider, args)); | |||
There was a problem hiding this comment.
Use the new Props.CreateBy method with takes an IIndirectActorProducer reference directly.
| /// <typeparam name="TActor">The type of the actor to create.</typeparam> | ||
| internal class ServiceProviderProps<TActor> : Props where TActor : ActorBase | ||
| /// <typeparam name="TActor">the actor type</typeparam> | ||
| internal sealed class ServiceProviderActorProducer<TActor> : IIndirectActorProducer where TActor:ActorBase |
There was a problem hiding this comment.
Refactor the ServiceProviderProps into an IIndirectActorProducer that can be used directly inside the Props class.
| { | ||
| return new ServiceProviderProps<TActor>(_provider, Arguments) |
There was a problem hiding this comment.
no-op for this class - scopes are meant to be managed directly by the users.
| @@ -1408,8 +1408,11 @@ namespace Akka.Actor | |||
| public static Akka.Actor.Props Create<TActor>(Akka.Actor.SupervisorStrategy supervisorStrategy) | |||
| where TActor : Akka.Actor.ActorBase, new () { } | |||
| public static Akka.Actor.Props Create(System.Type type, params object[] args) { } | |||
| [System.ObsoleteAttribute("Do not use this method. Call CreateBy(IIndirectActorProducer, params object[] arg" + | |||
There was a problem hiding this comment.
All public API changes made to the Props class - kept it to a bare minimum for compatibility reasons.
| /// </summary> | ||
| /// <typeparam name="TActor">The type of the actor to create.</typeparam> | ||
| /// <param name="args">The arguments needed to create the actor.</param> | ||
| /// <returns>The newly created <see cref="Akka.Actor.Props" />.</returns> | ||
| public static Props Create<TActor>(params object[] args) where TActor : ActorBase | ||
| { | ||
| return new Props(typeof(TActor), args); | ||
| return new Props(new ActivatorProducer(typeof(TActor), args), DefaultDeploy, args); |
There was a problem hiding this comment.
Use the ActivatorProducer directly, rather than use reflection to create it.
| /// </summary> | ||
| /// <typeparam name="TProducer">The type of producer used to create the actor.</typeparam> | ||
| /// <param name="args">The arguments needed to create the actor.</param> | ||
| /// <returns>The newly created <see cref="Akka.Actor.Props" />.</returns> | ||
| [Obsolete("Do not use this method. Call CreateBy(IIndirectActorProducer, params object[] args) instead")] | ||
| public static Props CreateBy<TProducer>(params object[] args) where TProducer : class, IIndirectActorProducer |
There was a problem hiding this comment.
This method sucks - marked as Obsolete.
| /// <param name="producer">The actor producer that will be used to create the underlying actor..</param> | ||
| /// <param name="args">The arguments needed to create the actor.</param> | ||
| /// <returns>The newly created <see cref="Akka.Actor.Props" />.</returns> | ||
| public static Props CreateBy(IIndirectActorProducer producer, params object[] args) |
There was a problem hiding this comment.
Take an explicit IIndirectActorProducer - the ServiceProvider extension now calls this method directly.
| /// <returns>The newly created <see cref="Akka.Actor.Props" />.</returns> | ||
| public static Props CreateBy(IIndirectActorProducer producer, params object[] args) | ||
| { | ||
| return new Props(producer, DefaultDeploy, args); |
There was a problem hiding this comment.
even though the args presumably get passed intro the IIndirectActorProducer, we still have to pass them into the Props constructor because they're used for things like remote deployments.
| } | ||
|
|
||
| [Obsolete("we should not be calling this method. Pass in an explicit IIndirectActorProducer reference isntead.")] | ||
| private static IIndirectActorProducer CreateProducer(Type type, object[] args) |
There was a problem hiding this comment.
Source of most of the weirdness I identified in #3599 stem from this method, which conflates accepting a Type for an actor with a Type for an IIndirectActorProducer, which has the effect of making it really difficult to reason about what is happening internally and performing reflection twice for all actor invocations.
|
I think I have the test suite under control now - going to run some benchmarks next. |
|
With this PR:
Dev:
Looks like we got a small improvement by stripping out some of the extra reflection. |
|
Eh, performance difference looks about the same actually - probably no negative or positive impact from this change. |
close #4853
close #3599
API approvals and
ServiceProviderPropschanges aren't done yet - just wanted to kick off the test suite to make sure I haven't massively blown it on introducing these changes safely.