Scoped IMapper is bad idea

[ZOXEXIVO]

Related to https://github.com/AutoMapper/AutoMapper/issues/2569 Scopes lifetime leaks into application services and it very annoying behaviour. Turns out, that mapper change behaviour of our services. It's just mapper, not HttpContext, DbContext or other contexts. Maybe Automapper takes on too much ?

[jbogard]

Singleton is the worst idea and can lead to easy bugs.

Transient is OK? I chose what the other context related dependencies are, which is scoped.

On Thu, Jan 31, 2019 at 3:10 PM Artemov Ivan notifications@github.com wrote:

Related to AutoMapper/AutoMapper#2569 https://github.com/AutoMapper/AutoMapper/issues/2569 Scopes lifetime flows into application services and it very annoying behaviour. Turns out, that mapper change behaviour of our services. It's just mapper, not HttpContext, DbContext or other contexts. Maybe Automapper takes on too much ?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/issues/81, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMrJUCPfZwMjYXK1qShx3goJ633QUks5vIwdhgaJpZM4acgiT .

[ZOXEXIVO]

I don't understand what scope need for Mapper? I create profile, and map with it. Which parts ot this process is dynamic?

[jbogard]

Resolvers, value converters, type converters, mapping actions. See the readme about what gets configured.

On Thu, Jan 31, 2019 at 5:55 PM Artemov Ivan notifications@github.com wrote:

I don't understand what scope need for Mapper? I create profile, and map with it. Which parts ot this process is dynamic?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/issues/81#issuecomment-459441802, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMlQ8kJvwhQvCpcw2Mv8e51gEOl47ks5vIy4cgaJpZM4acgiT .

[jholovacs]

@jbogard, single-instance (like in DI) and singleton are often confused with each other, but both can have their place, and it seems odd to find a library that's drawing some line in the sand on the matter. I think a much "worse" idea is to force a component to a scoped life cycle when there's nothing intrinsically scoped in its execution. I have to agree with @ZOXEXIVO: the scoped IMapper is definitely a bad idea.

[jbogard]

The most common DI use case with AutoMapper is to use a DbContext or RequestContext inside a Value Resolver or Type Converter.

Single instance prevents this most common scenario, and worse, would result in very nasty bugs.

Realistically it should have two modes, single instance and scoped, with scoped as the default, just like EF Core.

On Mon, Apr 22, 2019 at 2:53 PM Jeremy Holovacs notifications@github.com wrote:

@jbogard https://github.com/jbogard, single-instance (like in DI) and singleton are often confused with each other, but both can have their place, and it seems odd to find a library that's drawing some line in the sand on the matter. I think a much "worse" idea is to force a component to a scoped life cycle when there's nothing intrinsically scoped in its execution. I have to agree with @ZOXEXIVO https://github.com/ZOXEXIVO: the scoped IMapper is definitely a bad idea.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/issues/81#issuecomment-485530538, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAZQMTQFLKRIJNQ7UTJOLDPRYJUHANCNFSM4GTSBCJQ .

[jbogard]

By the way, DbContext's default lifetime is scoped:

https://github.com/aspnet/EntityFrameworkCore/blob/master/src/EFCore/EntityFrameworkServiceCollectionExtensions.cs#L65

You'll have the same problem trying to inject a DbContext. Singleton would be quite an ill-advised default, that would break so so many usages. You'd have be very careful with this mode, since all other types registered are transient.

Perhaps a better option would be do what EF Core does, and let you pass in the lifecycle with the current sensible default.

[jholovacs]

The DbContext's scoped lifetime makes sense; with the caching mechanisms built into it and how it is designed to work, it necessarily needs to have a relatively short lifespan, and a relatively narrow scope of work or else it would quickly become something that scaled very poorly.

The same cannot be said of an object mapping tool, however... IMO, there's no good reason to enforce one life cycle over another.

[jbogard]

DbContext's scope is "Scoped" not because of caching, but because it's a unit of work. And since it implements the unit of work pattern (and to a lesser extent, identity map), its lifecycle should be tied to whatever "unit of work" makes most sense for your environment.

IMapper takes a dependency on a factory method - see: https://github.com/AutoMapper/AutoMapper/blob/master/src/AutoMapper/Mapper.cs#L191

And I use this to construct every kind of dependency as part of a Map call:

https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/blob/master/src/AutoMapper.Extensions.Microsoft.DependencyInjection/ServiceCollectionExtensions.cs#L96

The default use of this package is in ASP.NET Core.

The design of the MS Ext DI package is for ASP.NET Core.

The default use case of using DI in these AutoMapper dependencies is to call into a DbContext or RequestContext.

The default lifetime of these dependencies is ServiceLifetime.Scoped.

So I have an object - Mapper - that takes a factory method in its constructor. It instantiates objects that for most usages are scoped. It then follows that the only possible lifetimes for the default use case is Scoped or Transient. Singleton would either result in runtime errors or bugs.

That's like....8 good reasons?

While you say that the same cannot be said of an object mapping tool - if you are registering it with a DI container, you're opting in to the DI behavior.

If you don't want to do that, there already exists a static/singleton implementation that scans for profiles.

[jholovacs]

Well, I'll have to disagree... If you're interested in discussing, I can explain why I don't see any of the reasons you listed as compelling:

Re: Unit of Work A unit of work does not require a scoped life cycle; it is a scope in itself, there's nothing implicit that would require a unit of work to have a forced life cycle, except for the considerations of implementation. There are quite a few UoW patterns with various ORMs that are implemented as transients for DI, and in theory it could be done as single-instance, if you didn't have to worry about caching, consistency, and/ or scalability. The reason it's scoped is because if it wasn't, it wouldn't perform very well at all. We saw this historically with hibernate/ NHibernate, and that's pretty much where the recommended "instance per request" concept was popularized. It performed much better that way, comparatively speaking.

Secondly, a DbContext is not a true UoW, as you can use the same object to perform multiple units of work, with multiple save/ commits per instance. The fact that it can be used in a UoW pattern, and is clearly intended to support a UoW pattern, doesn't change the fact that it is not actually a unit of work, so saying it's scoped because it's a unit of work doesn't make sense.

Factory Constructor: So you're injecting a factory pattern into your IMapper... again, there's nothing that requires this to be scoped in and of itself. Factories are often (usually) single-instance, so injecting a single-instance factory into another single-instance class doesn't violate any coding principles. I did see you referenced a guide from MS about how ASP.NET classes injecting factories should be scoped, but that leads directly into the next point, which is...

Default use being ASP.NET Core This assertion is mystifying to me. There's nothing intrinsically ASP.NET about it, why would there be a default use of it for anything? At the end of the day, all it's supposed to do is map properties of one object to another object, declaring it to have a default use in ASP.NET Core seems nonsensical. The default use is clearly to map properties of one object to another object... common usage should not be conflated with default usage.

The design of the MS Ext DI package is for ASP.NET Core The project page for MS Extensions DI states:

While commonly used in ASP.NET Core applications, these APIs are not coupled to the ASP.NET Core application model. They can be used in console apps, WinForms and WPF, and others.

I'm not going to attempt to divine what was in the minds of the original designers, but it's very clear they intended this to be used and usable outside of ASP.NET Core from the beginning. This seems to be in direct contradiction to your assertion.

The default use case of using DI in these AutoMapper dependencies is to call into a DbContext or RequestContext Again, this is conflating "common" usage with "default" usage, and that's just not the same thing.

So in summary, I don't feel that the reasons provided really mandate or support the assertion that this should be scoped.

[jholovacs]

I'll add there are few good reasons to support IMapper being single-instance:

Performance The Mapper does not need to be built every time it's used, and creating a new instance of it with each scope adds unnecessary overhead. While it's true that in most applications this will be insignificant and unnoticeable, on high-load application executing many scopes at the same time, this is an unnecessary burden on the CPU. It also puts more load on the GC.

Memory Utilization Making this scoped means that for every active scope in the system that is using a Mapper, there is a functionally equivalent object taking up space in memory... it's not maintaining a state, and by value each of these instances are identical, but of course by reference they are different.

I'll note again that unless you are running a lot of active scopes at the same time, these will probably be negligible, but it smells of redundant objects that aren't necessary, and that is a scalability design consideration.

[jbogard]

Let's look at scope options one-by-one then:

Transient

New object created every single request from the container. The default scope for registering dependencies. Best for lightweight, stateless services.

Transients can depend on singletons Transients can depend on scoped

Safest default

Scoped

Scoped services require a scope to be created, and are singleton for the lifetime of that scope. Scoped services that are disposable are disposed at the end of the scope (this was required for ASP.NET Core and broke every other container)

Scoped services that depend on an IServiceProvider will get the scoped service provider.

Scoped can depend on singletons Scoped can depend on transient (elevates that single dependency instance to scoped)

Singleton

Singleton services are the same object for each and every request.

Singletons cannot depend on scoped Singletons can depend on transient (elevates that single dependency instance to singleton)

Choosing a lifetime

AutoMapper utilizes a composition root to resolve handlers. For whatever application you're using it in, this composition root needs to be the same composition root as any other application dependency.

For ASP.NET Core applications, this must be the scoped service provider.

For messaging endpoints, this must be the scoped service provider (implemented as a context object like ASP.NET Core filters).

Between these, our only two real default options are "Scoped" or "Transient". Singleton has too many limitations and is fundamentally incompatible with the vast majority of use cases.

So really, maybe the default should be "Transient"? That would get rid of the assumption that you're running inside a scope.

Coupling with ASP.NET Core

Regardless of what the docs say, the MS Ext DI container was explicitly designed for ASP.NET Core. It contains zero features used outside of ASP.NET Core, and all features ASP.NET Core needed were added to this, regardless if any other container previously built supported it.

Additionally, no new features get added to the MS Ext DI container unless ASP.NET Core has an explicit need even if every other container that provides an implementation to MS Ext DI also supports this. You can find communications from the MS Ext DI team to this effect on open PRs (some opened by me).

While there may not be an explicit dependency either in code or in docs, in reality, MS Ext DI and ASP.NET Core are, as of today, inextricably linked, and will be for the foreseeable future. It is easy to see that it should be otherwise, especially given docs, but in reality, they are completely coupled.

Possible outcomes

To allow for usage outside of ASP.NET Core, I can:

• Change the scope of IMapper to transient
• Keep the scope the same, allow customizing IMapper (and I'm guessing everything else).
• Or both

If a consumer wants Singleton, then in reality not only does the mapper need to be singleton, then in reality all items registered by this extension should be singleton, since anything resolved by a singleton becomes elevated to a singleton.

This would be a bit of difference in today's behavior, since I register only IMapper as scoped and all the related services as transient. The absolute safest option, which wouldn't break the public API, would be to change the default registration to transient.

Singleton as a default is a complete non-starter, I have a half dozen clients this would immediately break with first-run exceptions.

Back to my first comment....transient then?

[lbargaoanu]

I, for one, completely agree with the scoped default :)

[jholovacs]

I think you're making a lot of assumptions here that do not reflect how people want to use this library. By doing so, you are actually artificially limiting how it can be used and reducing the scope of its efficacy... ironically, in the interest of making it compatible with ASP.NET Core, you are actually (artificially) making it incompatible with other implementations of .NET Core using different strategies and implementations, which is very strange to me.

There are also a lot of assertions that seem to be overgeneric and in some cases questionably accurate; in others, if true, seem to indicate an architecture design flaw in the library itself. Perhaps the library is trying to do too much? If it doesn't fit into a single-instance model, that indicates to me that it's maintaining some kind of state that can get confused if multiple threads are hitting it, and I don't understand how or why that could be the case for a library that is supposed to map property values from one object to another.

It seems you've got your mind made up; I won't bother you any more about it, and just look for less restrictive alternatives, or we'll just roll our own.

[jbogard]

I don't have my mind made up - I gave two clear options I'm willing to consider. All the other things are based on my experience and communications directly with the ASP.NET Core DI team.

I think you're really entirely missing the point of this library here - it's not about what AutoMapper is doing, but what the dependencies user-built AutoMapper extensions are doing.

Let me illustrate:

Mapper -- Depends on Func<Type, object>

OrderValueResolver : IValueResolver<Guid, Order> <- THIS IS THE PROBLEM AREA -- Depends on ShopContext (DbContext)

Pseudo-code calling Mapper.Map<InvoiceDto, Invoice>(invoiceDto):

var resolver =  mapper.ServiceCtor.GetService<OrderValueResolver>() // <-- KABLOOEY
invoice.Order = resolver.Resolve(invoiceDto, null, context);

Since Mapper depends on a composition root for resolving a half-dozen different potential mapping extension points, I have to super super care about the lifecycle of those potential extension points.

Singleton would prevent all users from using anything but Singleton. This is not my mind making this assertion, this is the behavior of this container. Singleton is by far the worst default option, so much so we moved away from it once we ran into numerous issues running as singleton with user-built extensions, and even stopped offering it after it became clear that it results in far too many subtle bugs with user-built extensions.

So you want to use Singleton

Why are you using this library then???? AutoMapper already provides assembly scanning for Profile types:

Mapper.Initialize(cfg => {
    cfg.AddMaps("Assembly1", "Assembly2");
});

Mapper.Map(foo, bar);

vs

services.AddAutoMapper("Assembly1", "Assembly2");

var mapper = services.BuildServiceProvider().GetService<IMapper>();

mapper.Map(foo, bar);

You might have a strong case for Transient as a default, but Singleton? No, because it breaks a very common scenario for people using MS Ext DI and AutoMapper - using Scoped items inside AutoMapper extension points, which, as I've shown, results in first-run exceptions.

[jholovacs]

I think your OrderValueResolver example would be what I consider very badly designed code; if it depends on a scoped object to resolve a value, I'd say that's a smell where the class has property values that are dynamically generated... yeah, you can theoretically do that but you're mixing things up in a way that's going to get you into trouble long before you get to configuring IoC. Translating one property to another property should not involve such heroics... but I don't think AutoMapper (or any other library) should try to prevent you from writing bad code. This is what I mean when I say it seems AutoMapper is trying to do too much. The level of sophistication in data transformation that you are describing in that example is much better suited for a (scoped) service... after all, we're not talking about mapping any more. We're talking about invoking business logic, persistence awareness, and state tracking. Mapping is almost an afterthought at that point.

It seems you're going back and forth between Singleton and single-instance, using the term Singleton for both... which is frustratingly legit, considering how MS DI is using the term, but they are very different concepts and I feel they need to be kept separate for the purposes of good communication. Static calls to the Mapper class like in your first example qualify as true Singleton pattern and I completely agree they should be avoided, as they are a hot mess to test and isolate plus a whole lot of other ugliness to which you've alluded. That being said, in your second example, if under the hood of AddAutoMapper() you allowed for the possibility of services.TryAddSingleton<IMapper>(sp=>[some instance construction]), this is single-instance, and has a lot more facility. To demonstrate, your second example would not be broken, and one could also inject the IMapper interface into other single-instance dependencies... provided you didn't so something silly like require a scoped object with state to do a translation between two objects.

If we want to talk about the vast majority of use cases, how many people use the IMapper for something significantly beyond flattening or copying property values from one object to another? How many people use multiple instances of the IMapper where they would actually need or want to use something other than the IMapper? And if they did... they should be able to register multiple IMappers as scoped, or transient, or in a factory, or some other pattern. I have to think the number of developers that would actually need or want to do that is quite small, comparatively speaking.

Consider this: Suppose I have a service class that is doing a lot of heavy lifting, and it's an expensive class to instantiate. Let's say I use this service a lot. I want to instantiate it as a thread-safe single-instance dependency that can be used across my app domain, to keep my resource utilization as low as possible. If I want to do some property mapping, I'm in a bit of a pickle if I want to use good SOLID principles with AutoMapper, because I can't inject an IMapper into it.

I think your aversion to single-instance is a bit unjustified, but if you really don't like it, I would definitely prefer to see transient, because then I could make a quick wrapper class to use across multiple scopes and register it as Singleton, inject the IMapper into that, and then inject that across my app domain as necessary. The problem I potentially see with that is that I don't think most people would assume the IMapper is transient, and that might come as a nasty shock if they heavily inject it in multiple places along a workflow.

My $.02

[jbogard]

I think your OrderValueResolver example would be what I consider very badly designed code

This is immaterial - singleton would prevent this scenario. It is a supported use case for 10 years now.

I think your aversion to single-instance is a bit unjustified

I have no aversion to single instance. In fact, the configuration is registered single instance today (and has been since the beginning). We've carefully ensured that object is thread-safe, too.

I do, however, have an extreme aversion to intentionally breaking my clients who use DI in resolvers, which is most of them. Not most maps, a very small percentage, but most of my clients use DI for scoped objects in resolvers and type converters.

Design issues aside, registering as a singleton will break people. That's a fact. And they will have no way to fix it. That's also a fact. Right now I'm preventing a scenario for you. You're asking to break applications with this change. This is why it's a complete non-starter to change the default to singleton. It will break people.

If you want singleton lifetime, but also don't use any of the extensions that this library resolves, there's a very easy fix. Do it yourself!

var mapperConfiguration = new MapperConfiguration(cfg => {
    cfg.AddMaps("Assembly1", "Assembly2");
});

services.AddSingleton<IConfigurationProvider>(mapperConfiguration);
var mapper = new Mapper(mapperConfiguration);
services.AddSingleton<IMapper>(mapper);

Done!

[jholovacs]

To be clear I was not suggesting that the default be single-instance. I was protesting that it seemed that the only way to scaffold this was as scoped, and seemed designed to break if injecting the mapper into a singleton service. If there's a means to do so, then this has just been a (perhaps excessively?) edifying conversation.

[jbogard]

You can certainly inject a transient into a singleton - it's just typically a bad idea. Singletons in a dependency tree should be the leaf nodes, since anything injected becomes a de facto singleton. A singleton as a root dependency is OK in some cases - for example ASP.NET Core registers singletons, but those singletons only depend on other singletons, and eventually create scopes per request on a method call.

[jholovacs]

I would agree it's typically a bad idea, mainly because it can get confusing and increase the chances of a bad design/ implementation, but there are cases where it makes sense to do so. I think it's generally best to allow developers to understand the life cycle of their application components and judiciously apply dependency configuration to suit their business needs. IMHO

[lock[bot]]

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.