IStringLocalizerFactory.Create(type) Invocation Strangeness - n + 1 times, then never again

[bgribaudo]

If an IStringLocalizerFactory is registered and an action returns a view (return new View(new SomeModel());) where the view's cshtml sets its model class (@model SomeModel) then uses a field from that model in a form:

• The first time the view is returned, IStringLocalizerFactory.Create(typeof(SomeModel)) is invoked at least n + 1 times, where n is the number of properties in the model. For example, if SomeModel has 3 properties, then Create(typeof(SomeModel)) will be called 4+ times.
• After this initial round of invocations, IStringLocalizerFactory.Create(typeof(SomeModel)) is never invoked again. When the site processes future requests that involve return new View(new SomeModel());, the IStringLocalizer previously returned by Create(typeof(SomeModel)) is used.

This behavior is puzzling. :-)

The first time the view is returned, it seems like Create(typeof(SomeModel)) should be called one time, regardless of the number of fields in SomeModel. Creating n + 1 or more instances seems wasteful.

Then, each subsequent time the view is returned, it seems advantageous for Create(typeof(SomeModel)) to be invoked one time to get the IStringLocalizer to use for that request (that is, don't cache the IStringLocalizer used for the previous request; instead, use the factory to create a new one just for this request). This would enable the factory to create a string localizer that's appropriate for the request (for example, if the localizer is database-powered, maybe the localizer should be loaded fresh from the database on each request) and avoid the need to keep every string localizer that's ever been used cached in memory.

To sum it up, it seems like the IStringLocalizer for SomeModel isn't cached when it should be (the initial return) but then is cached when maybe it shouldn't be (subsequent returns). :-)

Is this behavior expected/intended? Is there a chance it could be changed to 'use Create() to create one instance per action invocation' instead of 'use Create() to create a lot of instances then cache one forever'?

Demo - https://github.com/bgribaudo/StringLocalizerFactoryInvocationDemo

[mkArtakMSFT]

Thanks for contacting us, @bgribaudo.
@kichalla, can you please look into this? Thanks!

[kichalla]

This is by design as far as I can tell. We construct metadata of the model (the container type) + individual properties (which are themselves considered models too) and this is cached for further use.

[bgribaudo]

Hi @kichalla, thank you for looking into this. Is there any chance this behavior could be changed?

If I have a class that has 15 properties, it seems strange/wasteful for 16+ instances of the localizer to be created when one instance should work just fine. :-)

It would also be really neat if the factory were invoked for each request. This would align its behavior with how IViewLocaizer calls the factory once per request (or, more accurately, probably once per view per request).

On the second item: I'm working with a custom localizer factory that factors in the current request when it builds the localizer. This works fine for localizing text on pages, because IViewLocalizer doesn't cache localizers across requests. However, localization of field validation error messages doesn't work because of the caching (the request-specific localizer is cached across requests). If this behavior were changed to invoking the factory once per request, the localizer setup I'm using would work and ASP.Net Core would avoid keeping potentially every localizer used by the site cached for the lifetime of site's hosting processes.

[mkArtakMSFT]

@bgribaudo, can you please describe the scenario you're having trouble with current behavior? We would like to understand the main problem, as there may be some other approach you should take.
For example, what specifically from the request do you need and how does it affect localization?

[bgribaudo]

Hi @mkArtakMSFT! Sure thing!

Context: I'm working with a custom JSON-powered string localizer and localizer factory.

Scenarios:

1. ASP.Net Core allows views to be edited without requiring a site restart. That is, changes made to a view are reflected as soon as the view's cshtml file is saved and the page is reloaded. This is handy for rapid development.

   The same 'change live as soon as the edit is saved' also applies to localizer string edits pertaining to view localization (e.g. a cshtml file containing @inject IViewLocalizer). Since a fresh instance of the localizer is created via the localizer factory for each request, if I edit the JSON file used to provide localized strings to a given view, those edits are reflected as soon as I refresh the view. Again, the fact that no restart is required is handy for rapid development.

   However, 'save changes -> immediately live (no restart required)' does not hold true for the translation of DataAnnotation strings. Since the localizer used to translate a given view model's DataAnnotations is cached during the first request then reused for subsequent requests, the site must be restarted (to clear the cache) in order for translation file edits to be noticed by the system.

2. The fact that IStringLocalizers are cached and reused across requests means that the localizer instance needs to both know how to translate strings for a given language and also how to load and cache the translation strings for any supported language (because future requests handled by the localizer could be for any of those languages).

   If the factory created a new instance of the localizer per request, this could be simplified. The factory would have the option factor in the request's current UI culture when building the localizer instance used for that request. Since that localizer wouldn't be used across requests, it wouldn't need to be built to load and cache the various translations.

   This is the approach our custom localizer uses. It seemed to work fine until we discovered that DataAnnotation translations are cached. :-/ We could rework it to handle caching but this would make the localizer responsible for multiple concerns. The relevant unit tests illustrate this: the localizer factory's tests verify that it returns a correctly initialized localizer, given the context of the current request; the localizer's tests verify that it returns the correct strings, given the single language it was initialized to handle. However, when both sets of responsibilities are merged into the localizer, its tests need to cover that correct strings are returned times multiple languages. That "times" factor seems to suggest that the class is stepping beyond the Single Responsibility Principle--but since that design is forced upon the localizer by the caching, there doesn't seem to be much I can do about it unless the caching is changed/removed.

Hope this helps! Thank you for being willing to chat about this. Hope you have a great day!

[mkArtakMSFT]

Thanks for all the details, @bgribaudo.
This spun up a good discussion in our team. To assist us with this investigation, could you also provide a minimalistic repro project emphasizing the behavior you're observing?

[bgribaudo]

Hi @mkArtakMSFT! You're most welcome! Are you looking for a demo that shows the caching of the DataAnnotation-related localizers vs. how a fresh view localizer is created for each request or are you looking for something more in-depth than that?

[kichalla]

@bgribaudo We are specifically interested in the part where you see the problem of caching of DataAnnotation related localizers and since you are using a custom localizer factory with json file etc.

[bgribaudo]

@kichalla & @mkArtakMSFT, does https://github.com/bgribaudo/JsonStringLocalizerDemo provide what you're looking for?

It contains a basic (demo-grade) implementation of a JSON-powered string localization setup. The readme file gives instructions for how to observe the seemingly wasteful creation of localizers on the first request and the difference in behavior between how IStringLocalizerFactory's two Create() methods are called. The localizer/localizer factory code + unit tests should give an idea of how creating on a per-request basis (vs. forcing the localizer to handle the extra responsibility of working in a cross-request cached environment) simplifies the work required to implement these classes.

[mkArtakMSFT]

Thanks, @bgribaudo. Sorry for the delay. @ryanbrandenburg, please look into the provided repro so we can discuss it later.

[ryanbrandenburg]

Correct me if I'm wrong but the following are your problems:

1. Factory.Create is called multiple times for the same type.
2. You are concerned that re-using localizers causes additional caching concerns, specifically across requests which might have different languages.

My thoughts are as follows:

1. It is true that Factory.Create is called multiple times, but this needn't be a concern. The "default" IStringLocalizerFactory implementation caches localizers to prevent them from being created needlessly. So, rather than attempting to cache IStringLocalizer's itself, our code just trusts that if the factory wants localizers cached, it will do that itself.
2. IStringLocalizer implementations should in general return values from this[string] based on the CurrentUICulture at the time this[] is called, not based on the CurrentUICulture when the IStringLocalizer was created. I know that we have some code that might make you think otherwise, but we do not advise that people store the culture in their IStringLocalizer unless they have a specific reason. This issue actually inspired me to file Obsolete ResourceManagerWithCultureStringLocalizer and WithCulture interface member dotnet/aspnetcore#3324, which I hope would clarify this position.

Does that address all your concerns? If not let me know where I'm off-base.

[ryanbrandenburg]

I'm going to close this issue as by design, if you still have problems feel free to re-open with more details.

[bgribaudo]

Hi @ryanbrandenburg,

Thanks for looking into this! I was traveling so I'm a bit delayed in responding. :-( I'd reopen but apparently I don't have rights to do that.

In regards to my caching concerns, maybe it would help to describe a couple scenarios then contrast them with how ASP.Net Core 2.1 works.

Scenario 1

Factory.Create is called each time a localizer is needed. No caching is provided by ASP.Net Core. If caching is desired, implementing it is the responsibility of the localization developer.

Pro: Developer has full control over caching (yes cache/no cache/cache lifespan).
Con: If caching is desired, developer is responsible to implement.

Scenario 2

Factory.Create is called the first time a particular localizer is needed; the output returned is cached by ASP.Net Core. Any future requests for the same localizer are satisfied by the framework-provided cache; Factory.Create is not invoked.

Pro: Developer doesn't need to implement caching.
Con: Developer has no control over caching (such as disabling it or limiting the lifespan of cached objects). If the framework's caching strategy differs from what the developer wants, tough.

ASP.Net Core 2.1 (real life)

Both of the above scenarios apply. The developer has all of the cons of both but only gets the intersection of their benefits. :-(

During the first client request that triggers the need for a given localizer, Factory.Create may be called multiple times with a request for that localizer. If caching is important, the localizer developer must implement it. However, after that first client request, localizer caching is provided by the framework—whatever caching logic the developer may have put in Factory.Create is bypassed.

The developer needed to put the effort into implementing caching (the con of scenario 1) but because the framework’s caching then takes over the developer is unable to manage/control that caching (the con of scenario 2).

---

Any chance the current design could be changed? :-)

Maybe when a localizer factory is registered, an argument could be passed specifying the scope of caching to apply to localizer requests.

If this cache setting were respected on both the initial request and all subsequent requests, then we'd have the best of both worlds:

• A developer that wants localizers cached until the site is restarted simply needs to register the factory asking for singleton localizer caching. ResourceManager's StringLocalizationFactory, which exhibits this behavior, could probably be simplified because it could rely completely on the framework to provide all caching functionality.
• If, instead, the developer wants the localization creation process to somehow or another factor in the current request (for example, to reload translation strings from source on each request to align with Razor's ‘edit -> save -> changes are reflected live without requiring a restart’ behavior), the factory would be registered asking for per request (or transient) caching of localizers.

[bgribaudo]

Related: #7636