-
Notifications
You must be signed in to change notification settings - Fork 79
Scoped IMapper is bad idea #81
Comments
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 ***@***.***> wrote:
Related to AutoMapper/AutoMapper#2569
<AutoMapper/AutoMapper#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
<#81>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGYMrJUCPfZwMjYXK1qShx3goJ633QUks5vIwdhgaJpZM4acgiT>
.
|
I don't understand what scope need for Mapper? |
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 ***@***.***> 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
<#81 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGYMlQ8kJvwhQvCpcw2Mv8e51gEOl47ks5vIy4cgaJpZM4acgiT>
.
|
@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. |
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 ***@***.***> 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
<#81 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAZQMTQFLKRIJNQ7UTJOLDPRYJUHANCNFSM4GTSBCJQ>
.
|
By the way, DbContext's default lifetime is scoped: 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. |
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. |
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 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 The default lifetime of these dependencies is So I have an object - 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. |
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 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: Default use being ASP.NET Core The design of the MS Ext DI package is for ASP.NET Core
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 So in summary, I don't feel that the reasons provided really mandate or support the assertion that this should be scoped. |
I'll add there are few good reasons to support IMapper being single-instance: Performance Memory Utilization 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. |
Let's look at scope options one-by-one then: TransientNew object created every single request from the container. The default scope for registering dependencies. Best for lightweight, stateless services. Transients can depend on singletons Safest default ScopedScoped 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 SingletonSingleton services are the same object for each and every request. Singletons cannot depend on scoped Choosing a lifetimeAutoMapper 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 CoreRegardless 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 outcomesTo allow for usage outside of ASP.NET Core, I can:
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 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? |
I, for one, completely agree with the scoped default :) |
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. |
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 OrderValueResolver : IValueResolver<Guid, Order> <- THIS IS THE PROBLEM AREA Pseudo-code calling
Since 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 SingletonWhy are you using this library then???? AutoMapper already provides assembly scanning for
vs
You might have a strong case for |
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 If we want to talk about the vast majority of use cases, how many people use the 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 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 My $.02 |
This is immaterial - singleton would prevent this scenario. It is a supported use case for 10 years now.
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!
Done! |
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. |
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. |
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 |
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. |
Related to AutoMapper/AutoMapper#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 ?
The text was updated successfully, but these errors were encountered: