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

Using IStringLocalizer<T> leads to Lifestyle Mismatch in ASP.NET Core #711

Closed
chrisgl opened this issue May 15, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@chrisgl
Copy link

commented May 15, 2019

If you use IStringLocalizer<T> in your ASP.Net application together with the current Integration Guide for Version 4.6 (including Cross-Wiring) you get a "VerificationException" when the container is verified:

MyClassUsedInController (Async Scoped) depends on IStringLocalizer<MyClassUsedInController> (Transient).

As I see it, the IStringLocalizer got the wrong Lifestyle, when it was auto-crosswired.

Expected behavior
I expect to get no Exception. The Lifestyle should be Scoped, as this is the default-lifestyle of my container. In fact, I can't use Transient in ASP.NET Core, as every controller is Async Scoped.

Actual behavior
This is the Exception:

configuration is invalid. The following diagnostic warnings were reported:
-[Lifestyle Mismatch] QueryManagerInteractor (Async Scoped) depends on IStringLocalizer<QueryManagerInteractor> (Transient).
See the Error property for detailed information about the warnings. Please see https://simpleinjector.org/diagnostics how to fix problems and how to suppress individual warnings."

Additional context
.NET Core Version 2.2
SimpleInjector Version 4.6.0

I just quickly checked the CrossWire Implementation and it seems that the function ToLifestyle is the problem. It returns Transient as default for the switch statement.

@chrisgl chrisgl added the question label May 15, 2019

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

TLDR;

Add the following registration to Simple Injector:

container.RegisterConditional(typeof(IStringLocalizer),
    c => typeof(StringLocalizer<>).MakeGenericType(c.Consumer.ImplementationType),
    Lifestyle.Singleton,
    c => true);

VLTR (Very Long To Read):

it seems that the function ToLifestyle is the problem. It returns Transient as default for the switch statement.

Changing the default lifestyle in Simple Injector should not influence how framework components are cross wired. Simple Injector's Transient lifestyle is, therefore, the most appropriate lifestyle for registration of cross-wired ASP.NET components with the "transient" lifetime. Let me explain why this is.

By the use of cross wiring, the Simple Injector integration packages try to bridge the gap that ASP.NET Core’s Conforming Container model causes. Unfortunately, this hole can never be completely patched. That’s why, for instance, collections are not cross wired by the integration packages. Collections are constructed in a completely different and incompatible way, and injecting them by default could lead to confusing, unexpected, or even incorrect behavior.

In part, this holds as well when it comes to how both containers define what a Transient component is:

  • Simple Injector considers Transient components to be "lasting for only a short time; temporary", i.e. short lived and not reused. For that reason, Simple Injector prevents the injection of Transient components into Scoped and Singleton consumers as they are expected to be longer lived, which would otherwise result in Captive Dependencies.
  • The .NET Core DI Container (MS.DI) considers “transients” to be "lightweight, stateless services." As they are assumed stateless, it is safe to inject them into consumers with another lifetime, as long as they don’t have any stateful (sub) dependencies of their own. “Stateful,” in the context of MS.DI, typically means a scoped dependency. MS.DI's definition of transient is identical to what Autofac calls Instance Per Dependency. IMO, Autofac's naming is more correct, because conceptually, there is a big difference between the two definitions of transient. Unfortunately, in MS DI, this particular definition of transient is weakly held, because I can point you to numerous transiently registered components that actually do contain state and would cause major problems when injected into a singleton consumer. And we also see framework registrations, made with the transient lifetime, that arguably should have been singleton in the first place.

And this is where the trouble starts, because the StringLocalizer<T> component is registered as transient by Microsoft, likely because it is stateless. It can, in fact, safely be cached indefinitely by singleton consumers. But this is not something that Simple Injector can detect.

This is amplified by the fact that even stateless (transient) components can have stateful dependencies or sub dependencies. Say for instance the default (singleton) ResourceManagerStringLocalizerFactory is replaced with a transient DatabaseStringLocalizerFactory, which in its turn depends on a scoped StringResourcesDbContext. In that case it isn’t safe any longer to inject a transient StringLocator<T> into a Singleton consumer (as StringLocalizer<T> depends on IStringLocalizerFactory).

There is, unfortunately, no way for Simple Injector to look into the StringLocator<T>’s object graph to see whether this holds or not. This would mean that, in case Simple Injector would cross wire transient MS.DI components as Singleton, it would cause the infamous "Cannot resolve scoped service […] from root provider" exception when the instance is resolved from MS.DI. Or—at least—in case the IServiceProvider is built with the validateScopes set to true. When validateScopes set to false, however, the object graph will get happily constructed by MS.DI, while causing horrific—and possibly hard to track down—concurrency bugs.

And even if you’re willing to experience those dreadful "Cannot resolve scoped service […] from root provider" exceptions, it might not be easy—or obvious—at all to rewire those registrations.

This means that, from perspective of Simple Injector, it is only safe and sensible to cross wire those transient framework components as Transient—even when you have a different default lifestyle configured in Simple Injector.

Of course, in the context of ASP.NET Core, the difference between Scoped and Transient isn’t that big, considering the duration of a single web request. This means that in most cases, it would likely be safe to inject a Transient into a Scoped, even though in theory a Scoped is longer lived.

Thing is, however, that there is no way for Simple Injector to see if it’s safe. A user can decide to have a single scope for a long-running (background) operation, which makes that background-Scoped instance much longer lived than a web request Scope. Simple Injector can’t distinguish between scopes of a short-lived web request and a long-lived background operation, making ignoring the potential Lifestyle Mismatch between the Scoped consumer with its Transient dependency dangerous, even if you decide that Scoped is the default lifestyle for your application components.

I hope that the above gives an idea of why changing the lifestyle of cross-wired framework components is not a good idea. But the question now remains: how to proceed from here?

When it comes to IStringLocalizer, there are a few options. First of all, you can easily override the default IStringLocator<T> registration in the ServiceCollection and make it a singleton, as this is clearly what it should have been anyway:

services.AddSingleton(typeof(IStringLocalizer<>), typeof(StringLocalizer<>));

This would solve the problem, because in this case, Simple Injector will detect this registration to be a singleton and will, as well, cross wire it as Singleton in the container.

Another, and my preferred, solution is to make the non-generic IStringLocalizer registration directly into Simple Injector. This has an interesting benefit, because Simple Injector allows registrations to be contextual. This allows you to let application components depend on the non-generic IStringLocalizer, instead of the generic IStringLocalizer<T>, while still ensuring that Simple Injector injects the correct StringLocalizer<T> into an application component, where the T will be replaced with the type of the consuming component. The following registration shows how to do this:

container.RegisterConditional(typeof(IStringLocalizer),
    c => typeof(StringLocalizer<>).MakeGenericType(c.Consumer.ImplementationType),
    Lifestyle.Singleton,
    c => true);

This means that in your case, you can redefine your QueryManagerInteractor’s constructor to the following:

// Note the non-generic IStringLocalizer here
public QueryManagerInteractor(IStringLocalizer localizer) { … }

Still, because of the RegisterConditional registration, Simple Injector will construct the following object graph for you:

QueryManagerInteractor( // Async Scoped
    StringLocalizer<QueryManagerInteractor>( // Singleton
        IStringLocalizerFactory())) // Singleton

NOTE: This graph was visualized using Simple Injector's diagnostic services. For more information, see this.

This elegantly solves the problem while even simplifying your application code because of the removal of redundant generic typing.

This solution, however, is not very repeatable. It works very well with StringLocalizer<T>, but not with other framework services that can't be registered as Singleton.

Say, for instance, that your application requires the use of ASP.NET Core's ObjectPoolProvider. This service's default component, DefaultObjectPoolProvider, is registered as transient. It might, or might be reusable—we don't know. And even if we know this is the case, this might change in the future.

With framework services such as IStringLocalizer, you can assume that they are, and always will be thread-safe and reusable. Too much framework and application code depends on this (implicit) behavior. For other framework services, you might want to be more careful. Microsoft (or other third-party developers) might change its mind in a future version. Such change is still a breaking change, but I think it's safe to assume that these changes will happen nonetheless.

In such case, it's safer to inject a factory delegate that is able to resolve that component instead. Using the ObjectPoolProvider, for instance, that might look as follows:

// NOTE: IServiceScope is automatically registered when you call .UseSimpleInjector()
container.RegisterInstance<Func<ObjectPoolProvider>>(
    () => container.GetInstance<IServiceScope>().ServiceProvider
        .GetRequiredService<ObjectPoolProvider>());

This allows your application component(s) to depend on Func<ObjectPoolProvider> instead:

public QueryManagerInteractor(Func<ObjectPoolProvider> provider)

In case you know that a certain transient framework registration can be cached in a scope without causing any trouble, you could also decide to make the registration as follows:

container.Register(
    () => container.GetInstance<IServiceScope>().ServiceProvider
        .GetRequiredService<ObjectPoolProvider>(),
    Lifestyle.Scoped);

This, again, allows your application component(s) to depend on ObjectPoolProvider directly.

But please note my usual warning: According to the Dependency Inversion Principle, you should strive to let application components only depend on application-specific abstractions. From that perspective it might not make sense to inject framework-defined abstractions directly into our application components. Instead you wish to let the application declare an abstraction and create an adapter implementation—an infrastructural component—inside your Composition Root that delegates or transforms the call to the framework service. That adapter, however, could obviously still be injected with the framework service, a Func<T>, or an IServiceScope if required.

I hope this helps.

@chrisgl

This comment has been minimized.

Copy link
Author

commented May 17, 2019

Thank you for your detailed and very useful explaination.
I added your suggestion and it works fine for me.

@chrisgl chrisgl closed this May 17, 2019

@dotnetjunkie dotnetjunkie changed the title Using IStringLocalizer<T> leads to LifestyleMismatch in ASP.Net Core Using IStringLocalizer<T> leads to Lifestyle Mismatch in ASP.NET Core May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.