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.

Sign up for GitHub

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

Jump to bottom

Blazor Request Localization Updates Culture for All Threads #36259

Closed
wants to merge 1 commit into from
Closed

Blazor Request Localization Updates Culture for All Threads #36259

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Middleware/Localization/src/RequestLocalizationMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public async Task Invoke(HttpContext context)

context.Features.Set<IRequestCultureFeature>(new RequestCultureFeature(requestCulture, winningProvider));

SetCurrentThreadCulture(requestCulture);
SetDefaultThreadCulture(requestCulture);

if (_options.ApplyCurrentCultureToResponseHeaders)
{
Expand All @@ -133,10 +133,10 @@ public async Task Invoke(HttpContext context)
await _next(context);
}

private static void SetCurrentThreadCulture(RequestCulture requestCulture)
private static void SetDefaultThreadCulture(RequestCulture requestCulture)
{
CultureInfo.CurrentCulture = requestCulture.Culture;
CultureInfo.CurrentUICulture = requestCulture.UICulture;
CultureInfo.DefaultThreadCurrentCulture = requestCulture.Culture;
CultureInfo.DefaultThreadCurrentUICulture = requestCulture.UICulture;
Comment on lines +138 to +139
Copy link
Contributor Author

@TanayParikh TanayParikh Sep 8, 2021 •
edited
Loading

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).

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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!

Copy link
Contributor Author

@TanayParikh TanayParikh Sep 10, 2021 •
edited
Loading

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.

https://github.com/dotnet/wpf/blob/9ea906285a6daadcafc57d1a558be7f02c660548/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/CulturePreservingExecutionContext.cs#L12-L19

https://github.com/dotnet/wpf/blob/64bde4776eb04c039a304f6071ec25809242a287/src/Microsoft.DotNet.Wpf/src/Shared/MS/Internal/CulturePreservingExecutionContext.cs#L38-L47

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link

@javiercampos javiercampos Sep 16, 2021 •
edited
Loading

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.

}

private static CultureInfo? GetCultureInfo(
Expand Down