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.

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

Fix date format changing during UI updates by restoring request execution context #2643

Merged
merged 8 commits into from
Mar 6, 2024

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Mar 5, 2024

Fixes #2554

Switching between date formats bug:

  • Started when request localization was introduced. Blazor request would use the browser culture to render the UI.
  • However, UI updates triggered by telemetry subscriptions were run using the gRPC service's execution context. Requests to gRPC don't have a header indicating culture, so they defaulted to the systems.
  • The default system culture then flowed in Blazor UI when telemetry subscription fired and called StateHasChanged().

The fix is to capture the execution context when the Blazor page subscribes to telemetry changes, and then restore it when running the subcription. The restored Blazor execution context sets the right culture for the callback to run in.

@davidfowl You've worked with execution contexts the most here. Does this look right to you?

Microsoft Reviewers: Open in CodeFlow

@DamianEdwards
Copy link
Member

Shouldn't Blazor's dispatcher be handling this automatically?

@davidfowl
Copy link
Member

Shouldn't Blazor's dispatcher be handling this automatically?

No blazor doesn't handle this. This is when you have a callback API, you have to build something like this if you want to capture the ExecutionContext and have things like async locals work (that's also how you create a memory leak 😄 )

@JamesNK
Copy link
Member Author

JamesNK commented Mar 5, 2024

Shouldn't Blazor's dispatcher be handling this automatically?

I expected it should. I expected InvokeAsync to restore the original HTTP request execution context (or at least the culture) but it doesn't. There are some GH issues from customers in aspnetcore repo where Blazor folks say this is by design and you're reponsible for restoring the context.

@davidfowl
Copy link
Member

davidfowl commented Mar 5, 2024

It captures the sync context, not the execution context.

@JamesNK
Copy link
Member Author

JamesNK commented Mar 5, 2024

The end result is weirdness around culture though 🤷‍♂️

Anyway, no automatic culture restore is the world we live in, and this change isn't big or complicated so I'm happy with this change to fix the problem.

David, could you approve if the EC related code is good 🙏

@davidfowl
Copy link
Member

Anyway, no automatic culture restore is the world we live in, and this change isn't big or complicated so I'm happy with this change to fix the problem.

Dont build callback APIs 😄

@JamesNK
Copy link
Member Author

JamesNK commented Mar 5, 2024

Would changing the subscription API to have an subscription.WaitForNextUpdateAsync() method be better?

I could do that, but OTEL subscriptions are used in quite a few places. It would be a day of work to change the dashboard everywhere. Do you want that rather than adding execution context to the callback?

@JamesNK
Copy link
Member Author

JamesNK commented Mar 6, 2024

@davidfowl Changes from offline conversation applied.

@JamesNK JamesNK merged commit 498c216 into main Mar 6, 2024
8 checks passed
@JamesNK JamesNK deleted the jamesnk/executioncontext branch March 6, 2024 02:33
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard date format changes
4 participants