-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[Blazor] Dispatch inbound activity handlers in Blazor Web apps #53185
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
Conversation
operation.Descriptor.Parameters); | ||
break; | ||
case RootComponentOperationType.Update: | ||
pendingTasks[i] = webRootComponentManager.UpdateRootComponentAsync( |
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.
We previously didn't wait for "update" operations to complete before clearing persistent state in the first root component update batch. But it's likely preferable to wait for them to complete before returning back up the inbound activity handler pipeline, hence this change. If others think this change is problematic, we could take another approach (maybe collecting the "add" operations separately and awaiting those).
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.
@javiercn has been the one with the most specific opinions about the semantics for persistent state clearing, so hopefully we can get his view and explanation.
@@ -762,6 +764,7 @@ private async Task TryNotifyClientErrorAsync(IClientProxy client, string error, | |||
|
|||
// Retrieve the circuit handlers at this point. | |||
_circuitHandlers = [.. _scope.ServiceProvider.GetServices<CircuitHandler>().OrderBy(h => h.Order)]; | |||
_dispatchInboundActivity = BuildInboundActivityDispatcher(_circuitHandlers, Circuit); |
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.
This is pretty odd - it's hard to see why we have to do this here in particular. Is it just a matter of lazy initialization? If so would it be clearer still if the field was a Lazy<T>
that was always initialized during the constructor, and then we wouldn't need to initialize it explicitly in other places.
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 agree it's a bit confusing. I believe the reason we collect the circuit handlers here is that calling _scope.GetServices<CircuitHandler>()
may construct new instances of services registered as circuit handlers, and those services may access persistent component state in their constructors. So we need to be sure that persistent component state is available by the time these services get constructed. We might be able to use a Lazy<T>
, but my concern is that making circuit handlers lazily and implicitly initialized might cause us to accidentally change when circuit handlers are constructed without us knowing it.
Edit: Found the existing comment explaining it:
aspnetcore/src/Components/Server/src/Circuits/CircuitFactory.cs
Lines 85 to 91 in e175853
// In Blazor Server we have already restored the app state, so we can get the handlers from DI. | |
// In Blazor Web the state is provided in the first call to UpdateRootComponents, so we need to | |
// delay creating the handlers until then. Otherwise, a handler would be able to access the state | |
// in the constructor for Blazor Server, but not in Blazor Web. | |
var circuitHandlers = components.Count == 0 ? [] : scope.ServiceProvider.GetServices<CircuitHandler>() | |
.OrderBy(h => h.Order) | |
.ToArray(); |
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.
Yeah, this code is correct. The Blazor Server version also does the same thing.
} | ||
} | ||
|
||
return Task.CompletedTask; |
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.
This is a bit weird. It takes a second to realize that we are calling this method and that its mutating the state from the list of pending tasks.
Is there a way we can flow the pendingTasks
to HandleInboundActivityAsync
, otherwise, maybe make it explcit with a class instead of a closure.
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 restructured things a little - let me know if you think this is still a concern!
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.
Looks good to me
… (#53313) # Dispatch inbound activity handlers in Blazor Web apps Fixes an issue where inbound activity handlers were not getting invoked in Blazor Web apps. ## Description Inbound activity handlers were a new feature introduced in .NET 8 to allow Blazor Server apps to monitor circuit activity and make services available to Blazor components that wouldn't otherwise be accessible. However, we also introduced a new "Blazor Web" app model that didn't incorporate this new feature correctly. This PR fixes the issue by: * Building the inbound activity pipeline in Blazor Web apps just after circuit handlers are retrieved * Invoking inbound activity handlers in web root component updates Fixes #51934 ## Customer Impact Multiple customers have indicated being affected by this issue. Since this is a new feature in .NET 8, customers expect it to work in Blazor Web apps, which we recommend for new projects. Without this fix, the functionality only works in traditional Blazor Server apps. ## Regression? - [ ] Yes - [X] No This is a new feature in .NET 8. ## Risk - [ ] High - [ ] Medium - [X] Low The fix is simple and applies to a well-tested area of the framework, so our E2E tests are likely to catch issues with this change. ## Verification - [X] Manual (required) - [X] Automated ## Packaging changes reviewed? - [ ] Yes - [ ] No - [X] N/A
Dispatch inbound activity handlers in Blazor Web apps
Fixes an issue where inbound activity handlers do not get invoked in Blazor Web apps
Description
This PR fixes the issue by:
Fixes #51934