Add ITranslationProvider.LoadTranslationsAsync() #15886
Conversation
📝 WalkthroughWalkthroughThe updates across OrchardCore's localization modules primarily focus on transitioning from synchronous to asynchronous methods. This shift enhances the system's efficiency by supporting non-blocking operations, especially beneficial when loading translations from various data stores. Key interfaces and classes within the localization framework have been refactored to include async methods, with corresponding deprecations of their synchronous counterparts. Changes
Assessment against linked issues
The changes effectively address the primary objectives outlined in the linked issue #11356 by incorporating asynchronous methods across the localization interfaces and their implementations. This refactoring ensures that the system can handle translation loading operations asynchronously, thereby avoiding blocking calls and enhancing performance when interacting with various data storage solutions. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| ? GetAllStringsFromCultureHierarchyAsync(culture) | ||
| : GetAllStringsAsync(culture).ToEnumerable(); |
There was a problem hiding this comment.
Consider maintaining asynchronous flow in GetAllStrings.
- : GetAllStringsAsync(culture).ToEnumerable();
+ : GetAllStringsAsync(culture);This change avoids potential performance bottlenecks by not blocking the thread while converting IAsyncEnumerable to IEnumerable.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ? GetAllStringsFromCultureHierarchyAsync(culture) | |
| : GetAllStringsAsync(culture).ToEnumerable(); | |
| ? GetAllStringsFromCultureHierarchyAsync(culture) | |
| : GetAllStringsAsync(culture); |
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Show resolved
Hide resolved
| return includeParentCultures | ||
| ? GetAllStringsFromCultureHierarchyAsync(culture).GetAwaiter().GetResult() | ||
| : GetAllStringsAsync(culture).ToEnumerable(); |
There was a problem hiding this comment.
Consider maintaining asynchronous flow in GetAllStrings.
- return includeParentCultures
- ? GetAllStringsFromCultureHierarchyAsync(culture).GetAwaiter().GetResult()
- : GetAllStringsAsync(culture).ToEnumerable();
+ return includeParentCultures
+ ? await GetAllStringsFromCultureHierarchyAsync(culture).ToListAsync()
+ : await GetAllStringsAsync(culture).ToListAsync();This change avoids potential performance bottlenecks by not blocking the thread while converting IAsyncEnumerable to IEnumerable.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return includeParentCultures | |
| ? GetAllStringsFromCultureHierarchyAsync(culture).GetAwaiter().GetResult() | |
| : GetAllStringsAsync(culture).ToEnumerable(); | |
| return includeParentCultures | |
| ? await GetAllStringsFromCultureHierarchyAsync(culture).ToListAsync() | |
| : await GetAllStringsAsync(culture).ToListAsync(); |
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectHtmlLocalizer.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Since the ultimate consumers of these services, PortableObjectHtmlLocalizer and PortableObjectStringLocalizer retrieve the localization values synchronously, I doubt we get a performance benefit but there is a chance of deadlocks.
Please provide end-to-end benchmarks for these services so we can see if we gain something.
src/OrchardCore/OrchardCore.Localization.Abstractions/ITranslationProvider.cs
Outdated
Show resolved
Hide resolved
| /// <inheritdoc /> | ||
| public async Task<CultureDictionary> GetDictionaryAsync(CultureInfo culture) | ||
| { | ||
| var cachedDictionary = _cache.GetOrCreate(CacheKeyPrefix + culture.Name, k => new Lazy<Task<CultureDictionary>>(async () => |
There was a problem hiding this comment.
This now caches not the dictionary but a Task that produces the dictionary. While this looks working at a first glance, this is not what should really happen. Rather, the dictionary itself needs to be cached. I don't think the Lazy makes sense here anyway.
| <ItemGroup> | ||
| <PackageReference Include="System.Linq.Async" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
It's necessary to convert from IAsyncEnumerable to Enumberable
There was a problem hiding this comment.
It still builds if I remove it.
There was a problem hiding this comment.
Really!! I got 4 errors, how the IAsyncEnumerable extension methods be there without referencing a proper package?
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PoParser.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PoParser.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Zoltán Lehóczky <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
src/OrchardCore/OrchardCore.Localization.Core/LocalizationManager.cs (1)
39-62: Consider caching the dictionary directly instead of the task to avoid potential issues with multiple concurrent requests.
This comment was marked as off-topic.
This comment was marked as off-topic.
This reverts commit 93a802e.
There was a problem hiding this comment.
If the other changes in the PR prove to be suitable, then ILocalizationFileLocationProvider can be made async too.
There was a problem hiding this comment.
Why, it just retrieve a set of locations
| /// <inheritdoc /> | ||
| public CultureDictionary GetDictionary(CultureInfo culture) | ||
| { | ||
| var cachedDictionary = _cache.GetOrCreate(CacheKeyPrefix + culture.Name, k => new Lazy<CultureDictionary>(() => |
There was a problem hiding this comment.
The Lazyness here is important as it prevents r-entrancy. Without it multiple threads (concurrent requests?) can get into the body and run. The MemoryCache is not blocking on the factory body. With the Lazy, all concurrent requests can potentially create a Lazy instance, but only one will be added to the cache and executed.
| /// <inheritdoc /> | ||
| public async Task<CultureDictionary> GetDictionaryAsync(CultureInfo culture) | ||
| { | ||
| var cachedDictionary = await _cache.GetOrCreateAsync(CacheKeyPrefix + culture.Name, async (e) => |
There was a problem hiding this comment.
Sicne we don't want entries to be revoked here (we don't, right? even on memory pressure), we can use a keyed Dictionary and remove the prefix on the key (because there can't be conflicts anymore). I did it recently, I am sure you will recall.
There was a problem hiding this comment.
Can we use ConccurentDictionary?
There was a problem hiding this comment.
Yes, a ConcurrentDictionary actually since a culture could be read while another one is being added to it. But still a keyed service so we don't have to use IMemoryCache. We should only use it when we need expiration rules which we don't want here.
There was a problem hiding this comment.
Please also add a comment in the code to remember why we need a ConcurrentDictionary (I will forget myself)
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PoParser.cs
Show resolved
Hide resolved
| { | ||
| private static readonly Dictionary<char, char> _escapeTranslations = new() |
There was a problem hiding this comment.
Perfect candidate for FrozenDictionary.
Or just remove this dictionary and use a switch the compiler is clever enough to use a dictionary of ifs based on the number of items for best performance, so we don't have to think about it too much.
There was a problem hiding this comment.
I will create another PR for that because it might need extra testing and benchmarking
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
@hishamco for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment. |
| { | ||
| translationProvider.LoadTranslations(culture.Name, dictionary); | ||
| } | ||
| var dictionary = new CultureDictionary(culture.Name, rule ?? _defaultPluralRule); |
There was a problem hiding this comment.
The LoadTranslationsAsync should be move to a private AwaitedLoadtranslationAsync method with the async/await keywords such that we don't need it in the GetDictionaryAsync.
| @@ -41,7 +41,7 @@ public override LocalizedHtmlString this[string name] | |||
| if (_localizer is IPluralStringLocalizer pluralLocalizer && arguments.Length == 1 && arguments[0] is PluralizationArgument) | |||
| { | |||
| // Get an unformatted string and all non plural arguments (1st one is the plural count). | |||
| var (translation, argumentsWithCount) = pluralLocalizer.GetTranslation(name, arguments); | |||
| var (translation, argumentsWithCount) = pluralLocalizer.GetTranslationAsync(name, arguments).GetAwaiter().GetResult(); | |||
There was a problem hiding this comment.
All this PR to come back to this pitfall, for every single invokation of the localizer. It was better before, even if it was not reading the file asynchronously once)
There was a problem hiding this comment.
Maybe because we didn't do it the right way from the beginning :)
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
Fixes #11356
No breaking changes