AutoMapper / AutoMapper.Extensions.Microsoft.DependencyInjection

MIT License
258 stars 79 forks source link

Tests showing how bad singleton is #100

Closed jbogard closed 5 years ago

jbogard commented 5 years ago

In the first two tests, the services are correctly resolved.

In the third test, one of the dependencies is scoped, and you silently get a different instance than was originally resolved. The scoped instance is different than the one used by the IMapper instance.

jbogard commented 5 years ago

@lbargaoanu these are tests that would fail by changing the default to Singleton. We have to make a scoped dependency and resolve it inside a scope.

lbargaoanu commented 5 years ago

OK, but we want to resolve dependencies from the current scope when calling Map. So there is no way to do that besides http specific ways?

jbogard commented 5 years ago

You can, just don't configure your Mapper as singleton. The first two can successfully resolve scoped services. If you configure as singleton, you can't/shouldn't have any scoped/transient dependencies.

lbargaoanu commented 5 years ago

IMapper doesn't have any dependencies, it just uses a factory. If that factory can resolve things from the current scope, then the problem is solved. IMapper doesn't keep any references to objects, only Map needs them. So it's not a question of principle. It's rather whether the .Net Core DI allows you to access the current scope or not.

jbogard commented 5 years ago

The factory is a dependency, though, and which factory it gets depends on if it's resolved from the root service provider or from a scope.

Even though the mapper is resolved from a scope, if the mapper is singleton, the factory is the root service provider.

The purpose of these tests is to highlight we can't have Singleton as the default, since we'd be getting the "wrong" factory method in ASP.NET Core, and any scoped dependencies eventually resolved would be the wrong ones.

lbargaoanu commented 5 years ago

And I agree. I was saying that seems to be more of a limitation of .Net Core DI. The scope exists when calling Map. If I had an API to return that scope, I would be able to resolve scoped services from our global factory. We only need those services while calling Map, so there's no reason why that wouldn't work. But the only way to get the current scope seems to be through IHttpContextAccessor.

jbogard commented 5 years ago

It's also different than other containers - in StructureMap/Lamar for example, transients get elevated to "single-instance-per-scope" when resolved inside a scope.

lbargaoanu commented 5 years ago

Well, it makes more sense now. But it does seem a little simplistic as an DI engine. Maybe by design, but still...

jbogard commented 5 years ago

It is quite simplistic lol. I have open PRs to make it better but....they've been open for years.

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.