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

Props methods for propagating and setting IActorProducer are pretty weird #3599

Closed
Aaronontheweb opened this issue Sep 25, 2018 · 1 comment · Fixed by #4858
Closed

Props methods for propagating and setting IActorProducer are pretty weird #3599

Aaronontheweb opened this issue Sep 25, 2018 · 1 comment · Fixed by #4858

Comments

@Aaronontheweb
Copy link
Member

Akka.NET Version: v1.3.9

This is how we set the ActorProducer inside Props today when we're using dependency injection:

public Props(Deploy deploy, Type type, params object[] args)
{
Deploy = deploy;
inputType = type;
Arguments = args;
producer = CreateProducer(inputType, Arguments);
}

It's indirect, and we repeat this process of using reflection on the input type to figure out if we should use a custom ActorProducer whenever someone calls any of the fluent interface methods on Props, all of which end up calling the Props.Copy() method:

protected virtual Props Copy()
{
return new Props(Deploy, inputType, Arguments) { SupervisorStrategy = SupervisorStrategy };
}

I'm not sure why we designed it this way, but it's gross, relies on magic, and not all that extensible. What irks me most about it is that the Type we feed into Props can mean different things other than the actor's implementation. I'd rather change the signatures on Props to make it 100% explicit "I AM USING THIS PRODUCER" and not let the arguments of various Props constructors take on different meanings for the same input.

How we do this without breaking 100% of existing DI plugins for Akka.NET is an interesting question. I guess a reasonable way of handling that is just "upgrade all of them" since this change doesn't have a meaningful impact on end-user code today, but it would break binary compatibility since fixing this issue might end up changing either the signatures of existing Props methods or changing their meanings. That doesn't seem like a big deal.

@Aaronontheweb
Copy link
Member Author

Going to put this into the v1.4.0 branch, since this will probably require an update to all of the Akka.DI.* plugins.

@Aaronontheweb Aaronontheweb modified the milestones: 1.4.0, 1.4.1 and Later Feb 11, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.2, 1.4.3, 1.4.4 Mar 13, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.4, 1.4.5 Mar 31, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.5, 1.4.6 Apr 29, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.6, 1.4.7 May 12, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.7, 1.4.8 May 26, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.8, 1.4.9 Jun 17, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.9, 1.4.10 Jul 21, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.10, 1.4.11 Aug 20, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.11, 1.4.12 Nov 5, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.12, 1.4.13 Nov 16, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.13, 1.4.14 Dec 16, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.14, 1.4.15 Dec 30, 2020
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.15, 1.4.16 Jan 20, 2021
@Aaronontheweb Aaronontheweb modified the milestones: 1.4.17, 1.4.18 Mar 11, 2021
Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this issue Mar 17, 2021
Arkatufus pushed a commit that referenced this issue Mar 18, 2021
* full reformat of Props class

* refactor DefaultProducer

* close #3599 - cleaned up Props methods to make IIndirectorActorProducer dependencies more explicit

* close #4854 - removed and replaced ServiceProviderProps

* API approval

* added comment to clarify important backwards-compat change

* fix regression on Props.Create<T>

* eliminate one more source of reflection

* fixed SupervisorStrategy copying

* added specs to validate that Akka.DependencyInjection Props propagate values correctly

* fixed Props copy error
This was referenced Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant