fix(Localizer): makes JsonStringLocalizerFactory thread-safe#7919
Merged
fix(Localizer): makes JsonStringLocalizerFactory thread-safe#7919
Conversation
This reverts commit 49f36a0.
Contributor
Reviewer's GuideMakes JsonStringLocalizerFactory thread-safe by storing the current type name in AsyncLocal and resetting it, and adds a unit test to verify fallback behavior when the type name is not set. Sequence diagram for AsyncLocal type name usage in JsonStringLocalizerFactorysequenceDiagram
participant Caller
participant JsonStringLocalizerFactory
participant TypeNameContext
participant JsonStringLocalizer
Caller->>JsonStringLocalizerFactory: GetResourcePrefix(typeInfo)
JsonStringLocalizerFactory->>JsonStringLocalizerFactory: compute typeName from typeInfo
JsonStringLocalizerFactory->>TypeNameContext: _typeName.Value = typeName
JsonStringLocalizerFactory-->>Caller: resourcePrefix
Caller->>JsonStringLocalizerFactory: CreateResourceManagerStringLocalizer(assembly, baseName)
JsonStringLocalizerFactory->>TypeNameContext: read _typeName.Value
alt typeName is not null
JsonStringLocalizerFactory->>JsonStringLocalizerFactory: typeName = _typeName.Value
else typeName is null
JsonStringLocalizerFactory->>JsonStringLocalizerFactory: typeName = baseName
end
JsonStringLocalizerFactory->>TypeNameContext: _typeName.Value = null
JsonStringLocalizerFactory->>JsonStringLocalizer: new JsonStringLocalizer(assembly, typeName, baseName, options, logger, cache, handler)
JsonStringLocalizer-->>JsonStringLocalizerFactory: instance
JsonStringLocalizerFactory-->>Caller: JsonStringLocalizer instance
Updated class diagram for JsonStringLocalizerFactory concurrency fixclassDiagram
class ResourceManagerStringLocalizerFactory
class JsonStringLocalizerFactory {
-ILoggerFactory _loggerFactory
-JsonLocalizationOptions _jsonLocalizationOptions
-ILocalizationMissingItemHandler _localizationMissingItemHandler
-AsyncLocal~string?~ _typeName
+GetResourcePrefix(typeInfo TypeInfo) string
+GetResourcePrefix(baseResourceName string, baseNamespace string) string
+CreateResourceManagerStringLocalizer(assembly Assembly, baseName string) ResourceManagerStringLocalizer
}
class JsonStringLocalizer
class ILoggerFactory
class JsonLocalizationOptions
class ILocalizationMissingItemHandler
class AsyncLocal~T~
class ResourceManagerStringLocalizer
class Assembly
class TypeInfo
JsonStringLocalizerFactory --|> ResourceManagerStringLocalizerFactory
JsonStringLocalizerFactory --> ILoggerFactory : uses
JsonStringLocalizerFactory --> JsonLocalizationOptions : uses
JsonStringLocalizerFactory --> ILocalizationMissingItemHandler : uses
JsonStringLocalizerFactory --> AsyncLocal~T~ : holds
JsonStringLocalizerFactory --> JsonStringLocalizer : creates
JsonStringLocalizerFactory --> ResourceManagerStringLocalizer : returns
JsonStringLocalizerFactory --> Assembly : parameter
JsonStringLocalizerFactory --> TypeInfo : parameter
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ArgoZhang
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new unit test reaches into the factory via reflection to invoke the non-public
CreateResourceManagerStringLocalizer, which makes the test brittle against refactors; consider instead asserting the same behavior through the publicIStringLocalizerFactoryAPI so the test validates observable behavior without depending on private implementation details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new unit test reaches into the factory via reflection to invoke the non-public `CreateResourceManagerStringLocalizer`, which makes the test brittle against refactors; consider instead asserting the same behavior through the public `IStringLocalizerFactory` API so the test validates observable behavior without depending on private implementation details.
## Individual Comments
### Comment 1
<location path="test/UnitTest/Localization/JsonStringLocalizerTest.cs" line_range="334-343" />
<code_context>
}
+ [Fact]
+ public void CreateResourceManagerStringLocalizer_UseBaseNameWhenTypeNameIsNull()
+ {
+ var factory = Context.Services.GetRequiredService<IStringLocalizerFactory>();
+ var mi = factory.GetType().GetMethod("CreateResourceManagerStringLocalizer", BindingFlags.NonPublic | BindingFlags.Instance)!;
+
+ var baseName = typeof(Foo).FullName!;
+ var localizer = Assert.IsType<IStringLocalizer>(mi.Invoke(factory, [typeof(Foo).Assembly, baseName]), exactMatch: false);
+ var result = localizer["not-found-key"];
+
+ Assert.True(result.ResourceNotFound);
+ Assert.Equal(baseName, result.SearchedLocation);
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that covers reuse of the factory to ensure the AsyncLocal type name is cleared between invocations.
To also cover the new AsyncLocal behavior and the underlying concurrency bug, please add a test that calls `CreateResourceManagerStringLocalizer` twice: once after a `GetResourcePrefix` call (so `_typeName` is set) and then again without setting `_typeName` (or after clearing it). The second call should rely on `baseName` and must not reuse the previous type name, confirming that `_typeName.Value` is correctly reset and protecting against regressions in the cleanup logic.
Suggested implementation:
```csharp
Assert.Equal(baseName, result.SearchedLocation);
}
[Fact]
public void CreateResourceManagerStringLocalizer_AsyncLocalTypeNameIsClearedBetweenInvocations()
{
var factory = Context.Services.GetRequiredService<IStringLocalizerFactory>();
var factoryType = factory.GetType();
var getResourcePrefix = factoryType.GetMethod(
"GetResourcePrefix",
BindingFlags.NonPublic | BindingFlags.Instance);
var createLocalizer = factoryType.GetMethod(
"CreateResourceManagerStringLocalizer",
BindingFlags.NonPublic | BindingFlags.Instance)!;
var fooType = typeof(Foo);
var baseName = fooType.FullName!;
// First invocation: simulate setting the AsyncLocal _typeName via GetResourcePrefix
_ = getResourcePrefix!.Invoke(factory, new object[] { fooType });
var localizer1 = Assert.IsType<IStringLocalizer>(
createLocalizer.Invoke(factory, new object[] { fooType.Assembly, baseName }),
exactMatch: false);
var firstResult = localizer1["not-found-key"];
Assert.True(firstResult.ResourceNotFound);
// Second invocation without setting _typeName again. This must rely solely on baseName
// and must not reuse the previous AsyncLocal type name.
var localizer2 = Assert.IsType<IStringLocalizer>(
createLocalizer.Invoke(factory, new object[] { fooType.Assembly, baseName }),
exactMatch: false);
var secondResult = localizer2["not-found-key"];
Assert.True(secondResult.ResourceNotFound);
Assert.Equal(baseName, secondResult.SearchedLocation);
}
```
This test assumes:
1. The factory under test exposes a non-public instance method named `GetResourcePrefix` that accepts a single `Type` (or compatible) parameter. If the actual signature differs (e.g., `TypeInfo`), adjust the `Invoke` call accordingly, for example:
- `new object[] { fooType.GetTypeInfo() }` for a `TypeInfo` parameter.
2. `using System.Reflection;` and `using Microsoft.Extensions.Localization;` are already present at the top of the file.
If the method name or binding flags differ in your implementation, update the reflection calls to match the actual factory API.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+334
to
+343
| public void CreateResourceManagerStringLocalizer_UseBaseNameWhenTypeNameIsNull() | ||
| { | ||
| var factory = Context.Services.GetRequiredService<IStringLocalizerFactory>(); | ||
| var mi = factory.GetType().GetMethod("CreateResourceManagerStringLocalizer", BindingFlags.NonPublic | BindingFlags.Instance)!; | ||
|
|
||
| var baseName = typeof(Foo).FullName!; | ||
| var localizer = Assert.IsType<IStringLocalizer>(mi.Invoke(factory, [typeof(Foo).Assembly, baseName]), exactMatch: false); | ||
| var result = localizer["not-found-key"]; | ||
|
|
||
| Assert.True(result.ResourceNotFound); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test that covers reuse of the factory to ensure the AsyncLocal type name is cleared between invocations.
To also cover the new AsyncLocal behavior and the underlying concurrency bug, please add a test that calls CreateResourceManagerStringLocalizer twice: once after a GetResourcePrefix call (so _typeName is set) and then again without setting _typeName (or after clearing it). The second call should rely on baseName and must not reuse the previous type name, confirming that _typeName.Value is correctly reset and protecting against regressions in the cleanup logic.
Suggested implementation:
Assert.Equal(baseName, result.SearchedLocation);
}
[Fact]
public void CreateResourceManagerStringLocalizer_AsyncLocalTypeNameIsClearedBetweenInvocations()
{
var factory = Context.Services.GetRequiredService<IStringLocalizerFactory>();
var factoryType = factory.GetType();
var getResourcePrefix = factoryType.GetMethod(
"GetResourcePrefix",
BindingFlags.NonPublic | BindingFlags.Instance);
var createLocalizer = factoryType.GetMethod(
"CreateResourceManagerStringLocalizer",
BindingFlags.NonPublic | BindingFlags.Instance)!;
var fooType = typeof(Foo);
var baseName = fooType.FullName!;
// First invocation: simulate setting the AsyncLocal _typeName via GetResourcePrefix
_ = getResourcePrefix!.Invoke(factory, new object[] { fooType });
var localizer1 = Assert.IsType<IStringLocalizer>(
createLocalizer.Invoke(factory, new object[] { fooType.Assembly, baseName }),
exactMatch: false);
var firstResult = localizer1["not-found-key"];
Assert.True(firstResult.ResourceNotFound);
// Second invocation without setting _typeName again. This must rely solely on baseName
// and must not reuse the previous AsyncLocal type name.
var localizer2 = Assert.IsType<IStringLocalizer>(
createLocalizer.Invoke(factory, new object[] { fooType.Assembly, baseName }),
exactMatch: false);
var secondResult = localizer2["not-found-key"];
Assert.True(secondResult.ResourceNotFound);
Assert.Equal(baseName, secondResult.SearchedLocation);
}This test assumes:
- The factory under test exposes a non-public instance method named
GetResourcePrefixthat accepts a singleType(or compatible) parameter. If the actual signature differs (e.g.,TypeInfo), adjust theInvokecall accordingly, for example:new object[] { fooType.GetTypeInfo() }for aTypeInfoparameter.
using System.Reflection;andusing Microsoft.Extensions.Localization;are already present at the top of the file.
If the method name or binding flags differ in your implementation, update the reflection calls to match the actual factory API.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a JsonStringLocalizerFactory concurrency issue where a shared _typeName could bleed across concurrent requests and lead to incorrect type resolution in localization.
Changes:
- Replace the shared
_typeNamefield with anAsyncLocal<string?>and clear it after localizer creation to isolate concurrent flows. - Add a unit test ensuring
CreateResourceManagerStringLocalizerfalls back tobaseNamewhen_typeNameis not set. - Update/extend table dynamic-context tests and relax an assertion around Blazor ChangeDetection caching behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/BootstrapBlazor/Localization/Json/JsonStringLocalizerFactory.cs |
Uses AsyncLocal for per-execution-context _typeName to prevent cross-request contamination. |
test/UnitTest/Localization/JsonStringLocalizerTest.cs |
Adds a regression test for fallback behavior when _typeName is null. |
test/UnitTest/Components/TableTest.cs |
Adds dynamic-context edit-mode tests; weakens ChangeDetection caching assertions (currently commented out). |
src/BootstrapBlazor/BootstrapBlazor.csproj |
Bumps package version to 10.5.2-beta01. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Assert.True(result.ResourceNotFound); | ||
| Assert.Equal(baseName, result.SearchedLocation); | ||
| } | ||
|
|
There was a problem hiding this comment.
This change fixes a concurrency issue in the localizer factory, but there isn’t a regression test that exercises concurrent IStringLocalizerFactory.Create(...) calls (e.g., in parallel tasks) to ensure SearchedLocation/type resolution doesn’t bleed across requests. Adding a small parallel test would help prevent this from reappearing.
Suggested change
| [Fact] | |
| public async Task Create_Ok_InParallel() | |
| { | |
| var factory = Context.Services.GetRequiredService<IStringLocalizerFactory>(); | |
| var expectedNamedSearchedLocation = factory.Create("Lang", "UnitTest")["not-found-key"].SearchedLocation; | |
| var expectedTypedSearchedLocation = factory.Create(typeof(Foo))["not-found-key"].SearchedLocation; | |
| var tasks = Enumerable.Range(0, 50).Select(async index => | |
| { | |
| await Task.Yield(); | |
| if (index % 2 == 0) | |
| { | |
| var localizer = factory.Create("Lang", "UnitTest"); | |
| var result = localizer[$"not-found-key-{index}"]; | |
| Assert.True(result.ResourceNotFound); | |
| Assert.Equal(expectedNamedSearchedLocation, result.SearchedLocation); | |
| } | |
| else | |
| { | |
| var localizer = factory.Create(typeof(Foo)); | |
| var result = localizer[$"not-found-key-{index}"]; | |
| Assert.True(result.ResourceNotFound); | |
| Assert.Equal(expectedTypedSearchedLocation, result.SearchedLocation); | |
| } | |
| }); | |
| await Task.WhenAll(tasks); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7918
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Fix thread-local handling of type names in JSON string localization to prevent concurrency issues and incorrect resource resolution.
Bug Fixes:
Tests: