-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Blazor Request Localization Updates Culture for All Threads #36259
Conversation
CultureInfo.DefaultThreadCurrentCulture = requestCulture.Culture; | ||
CultureInfo.DefaultThreadCurrentUICulture = requestCulture.UICulture; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description has details of this change.
It fixes this issue, but wondering if this may have consequences (outside blazor?). Was there a reason we specified CurrentCulture
? (This code hasn't changed for 6+ years)
Would doing this possibly change cultures for all users on a server (if all circuits are in the same process) or is that not a concern (due to circuits being isolated in their own process).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this would affect other users and requests and is not the right solution. You need something blazor specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fix for this would be in the https://github.com/dotnet/aspnetcore/blob/main/src/Components/Components/src/Rendering/RendererSynchronizationContext.cs. We would stash away the CurrentCulture before we queue up the work and apply it when we schedule it. Let's talk about this if you need more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://docs.microsoft.com/en-us/dotnet/api/system.threading.executioncontext?view=net-5.0#remarks:
the impersonation context and culture would typically flow with the execution context
Since we're already capturing the ExecutionContext
as part of the work item (here), it seems like maybe we already have the necessary info to restore the culture when the work item executes.
Perhaps it already should just work, and the real bug is something different. We already invoke the work item (in at least one case) via ExecutionContext.Run
here, so I wonder whether that's supposed to reassign the culture.
Basically, it would be great to figure out how this is supposed to work first!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExecutionContext.Run goes into here -> to here. Which doesn't have any impact on the culture. It doesn't appear the execution context deals with the culture directly (unless it's inheriting the culture changes from the execution threads). The following seems to indicate that this should currently work as is.
However, the current behavior matches the expected behavior for 4.5.2 and earlier:
https://github.com/dotnet/wpf/blob/64bde4776eb04c039a304f6071ec25809242a287/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/CulturePreservingExecutionContext.cs#L49-L64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we need to come up with a pattern or documentation for this, but the issue isn't that all of InvokeAsync is broken. Let's punt on this PR for 6.0 until we have better clarity.
Makes sense, thanks for the clarification. Going to close out this PR for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue isn't that the sync context isn't preserving the culture, it's that when an InvokeAsync is triggered from a background task e.g. in the issue report, the InvokeAsync is triggered when their cache is updated from a background task.
Right, that makes more sense! Thanks for checking and clarifying.
It's an interesting problem to have because I don't know of any other .NET application models that involve this possibility. Blazor Server is the only case I know where there are simultaneously multiple users with different cultures, and it's possible for one user's actions to directly make calls into another user's context and update their UI.
It's as if we need an API for setting the "circuit culture", and then we make Invoke
automatically apply the circuit culture as part of the transition. That would naturally guide developers to get the right results even if they don't quite understand the situation (as long as they do understand they should set circuit culture and not just Thread.CurrentCulture/Thread.CurrentUICulture).
Alternatively we could do something like:
- During every render when you're already on the sync context, have the renderer capture
Thread.CurrentUICulture
- Make
Invoke
automatically reapply any previously-captured culture
Then developers wouldn't have to call any new APIs and things would behave just as you'd expect. It may be a technically breaking change if someone (for some reason) actually doesn't want the culture to be retained automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be a technically breaking change if someone (for some reason) actually doesn't want the culture to be retained automatically.
Ah that's a great point. Should we target this for 7.0 in this case, not sure how feasible breaking changes may be at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems a bit risky to take a change here this late. If we front load this for 7.0, we'll have a lot more time to figure out if something's gone awry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this can't go into 6.0, but could it be possible to have (as I suggested here: #35965 ) a nullable and settable CultureInfo
property on ComponentBase
/IComponent
and have the renderer change/restore the CurrentUICulture
when rendering a component instance (have it be CultureInfo.CurrentUICulture
or just disregard it when it's null), or maybe make an ICultureAwareComponent
interface that you can apply to your components with that property (so that it wouldn't be a breaking change at all and no existing code would act differently if that's not implemented). That way, you could even have different components on the same circuit with different cultures, should anyone ever need that and it'd not be a breaking change at all.
This gif from the original issue shows the culture changing to the 'correct' new culture, but then reverting back to the original culture.
The underlying issue here was that Hosted Services weren't being updated with the culture from request localization. Hence the culture initially reflects the culture provided via request localization, however when the Hosted Service provided a data refresh, it reverted back to the original culture (as the hosted service didn't know of the culture change).
The fix here is simply updating the culture of all threads, instead of just the current thread, when executing the request localization middleware.
I've tested and verified this locally. If the approach is good to go I'll cleanup and push the tests.
Fixes: #28521