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

DIActorContextAdapter uses typeof().Name instead of AssemblyQualifiedName #833

Closed
Danthar opened this issue Apr 10, 2015 · 14 comments
Closed

Comments

@Danthar
Copy link
Member

Danthar commented Apr 10, 2015

The Akka.DI.Core.DIActorContextAdapter passes typeof(TActor).Name to the Props object.
I do not think this is correct. As this would effectively bars a user from having more then 1 actor with the same type name, but in different namespaces, in the same ActorSystem.

First I figured it should be .FullName but then you cant reference Actors from different assemblies. So AssemblyQualifiedName it is.

@jcwrequests
Copy link

@Danthar thanks for the input and I will makes the adjustments. Do you have an example?

@jcwrequests
Copy link

Nevermind about the example its just an impulse to asked for examples to recreate the issue. I am just a big believer in documentation. Cheers

@nvivo
Copy link
Contributor

nvivo commented Apr 10, 2015

Just an opinion, but I think this issue should be fixed here.

There shouldn't be a way to create an invalid props if you already have the Type object, so DIExt.Props could accept the Type directly and get the qualified name there.

I'd dare to go further and ask why pass strings at all if you already have the type? Couldn't the DIActorProducer hold the type object instead? It would be more performant than creating string qualified names, as those are not interned, so you will end up with a bunch of different references to the same value in memory.

@jcwrequests
Copy link

@nvivo the strings are an Akka thing not something that I personally prefer. This part is from the JVM side. I was just working within the provided constraints. But I certainly agree with your opinion.

@nvivo
Copy link
Contributor

nvivo commented Apr 10, 2015

Yeah, this "must be equal to the JVM version" thing keeps biting me every day. =)

@Danthar
Copy link
Member Author

Danthar commented Apr 10, 2015

Just found the same issue in the AutofacDependencyResolver.

  public Props Create<TActor>() where TActor : ActorBase
    {
        return system.GetExtension<DIExt>().Props(typeof(TActor).Name);
    }

I realise my PR was way to naive. There are lots of other places where this problem occurs.

@rogeralsing
Copy link
Contributor

Some thoughts.
In the DIActorProducer the field containing the typename is actorName this is confusing as it is then used to get concrete types via actorName.GetType(), so it's not the name, its the FQN of the actor.
Also, why do we store the actor type as a string internally? if we want to use a type, why dont we store it as a type?

@jcwrequests
Copy link

@rogeralsing I believe that you are right. Thinking back that was something that I decided based on my initial understanding of the process which has changed considerably since then. Thanks for your suggestion it's really appreciated.

@jcwrequests
Copy link

@rogeralsing I have a working model based on your suggestion which makes more sense. I will have to see if I can reproduce @nvivo @Danthar Issues after the changes. Thanks so much.

@nvivo
Copy link
Contributor

nvivo commented Apr 10, 2015

If the type or the qualified name is used there can't be any problems. My issue was exactly that the type couldn't be found just with the class name.

@jcwrequests
Copy link

@nvivo Then I think my solution will solve that issue.

@jcwrequests
Copy link

@Danthar @nvivo Please verify that PR #835 resolves this issue. Thanks.

@jcwrequests
Copy link

@nvivo hey I was reading through the suggestions and realized that your suggestion was the same as @rogeralsing. When I was responding I was only on my phone and did not read it through clearly. Sorry for that.

@Aaronontheweb
Copy link
Member

Resovled per #835

This was referenced Apr 28, 2015
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

No branches or pull requests

5 participants