Fix event during onafterrender #32017
Conversation
SteveSandersonMS
added
area-blazor
| @@ -138,90 +160,6 @@ protected override void HandleException(Exception exception) | |||
| } | |||
| } | |||
|
|
|||
| /// <inheritdoc /> | |||
| public override Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? eventFieldInfo, EventArgs eventArgs) | |||
There was a problem hiding this comment.
Can you help me understand what the consequences of removing this API are?
There was a problem hiding this comment.
The API remains, it's just this override that is no longer needed, since the base class implementation is now sufficient.
| /// at least an extra allocation and layer of try/catch per call, plus more work to schedule continuations | ||
| /// at the call site. | ||
| /// </remarks> | ||
| public static void Schedule<T>(T state, Action<T> callback) |
There was a problem hiding this comment.
nit: should the method be aligned with the class? WebAssemblyCallScheduler / Schedule, or WebAssemblyCallQueue.Enqueue ?
Although since this is static, there's a bit of redundancy, maybe WebAssemblyDispatcher.Schedule?
There was a problem hiding this comment.
TBH with this being internal I'd be inclined to leave it. Trying to communicate something like "run it maybe right away or maybe later" is hard to put into a single word. We already use the term "dispatcher" for something different so I was going for "call queue" as the most literal statement of what it is.
src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyCallQueue.cs
Outdated
Show resolved
Hide resolved
| // For now, this minimal work queue is an internal detail of how the framework dispatches | ||
| // incoming JS->.NET calls and makes sure they get deferred if a renderbatch is in process. | ||
|
|
||
| internal static class WebAssemblyCallQueue |
There was a problem hiding this comment.
Super nit suggestion: You could consider making this a singleton to make unit testing this type easier.
src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyRenderer.cs
Outdated
Show resolved
Hide resolved
| // we have to delay the renderbatch acknowledgement until it gets to the front of that queue. | ||
| // This is for consistency with Blazor Server which queues all JS-to-.NET calls relative to each | ||
| // other, and because various bits of cleanup logic rely on this ordering. | ||
| var tcs = new TaskCompletionSource(); |
There was a problem hiding this comment.
Should this specify RunContinuationsAsynchronously?
There was a problem hiding this comment.
I'm not aware of any need for that here.
| { | ||
| if (_isCallInProgress) | ||
| { | ||
| _pendingWork.Enqueue(() => callback(state)); |
There was a problem hiding this comment.
If you wanted to avoid the allocation, you could make this an Action<object>. This way the caller can be aware of any allocations incurred due to boxing.
There was a problem hiding this comment.
I didn't spot a good way to do that without forcing an extra allocation if T would otherwise be a value type (e.g., in the synchronous execution case where the implementation here can avoid boxing). I guess technically there could be two overloads of this method, one if T:class and one for T:struct. Also it would force the caller to put a typecast inside their callback which is a bit awkward.
Since 99% of the time the call will be executed synchronously, I didn't think it worth making the code that much more to maintain and risk future inconsistencies.
Please let me know if you disagree or have ideas for how to avoid that drawback.
There was a problem hiding this comment.
None in particular. I guess if we profile this and see additional allocations, we could devise something interesting.
…CallQueue.cs Co-authored-by: Pranav K <[email protected]>
…lyRenderer.cs Co-authored-by: Pranav K <[email protected]>
src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyCallQueue.cs
Show resolved
Hide resolved
| // No need to await because we want the root components to be added in parallel (e.g., | ||
| // not waiting for each other's OnInitializedAsync tasks), and any async exceptions will | ||
| // be handled by normal component lifecycle mechanisms. | ||
| _ = renderer.AddComponentAsync(rootComponent.ComponentType, rootComponent.Selector, rootComponent.Parameters); |
There was a problem hiding this comment.
This changes the semantics for line 175 below. At least we need to capture all of them and do a Task.WhenAll before we call store.ExistingState.Clear()
We should also look at what we do on Blazor server and Desktop and see if we do it in sequence or in parallel. I think we should be consistent across platforms
There was a problem hiding this comment.
Great point, well spotted. I've changed it to retain the existing semantics.
| // For now, this minimal work queue is an internal detail of how the framework dispatches | ||
| // incoming JS->.NET calls and makes sure they get deferred if a renderbatch is in process. | ||
|
|
||
| internal static class WebAssemblyCallQueue |
There was a problem hiding this comment.
This looks very similar to the RendererSynchronizationContext is there a way we could "reuse" that here instead of having a separate work queue?
These things are tricky to test implement and can lead to subtle bugs that are hard to track down
There was a problem hiding this comment.
The main issue with using a sync context is the back-compat about enforcement of being "on" the sync context (discussed above), but even besides that, the existence of synchronous interop makes WebAssembly a different kind of runtime environment for which we can't assume that RendererSynchronizationContext has the right behaviors. For example, incoming calls may already be on the sync context but we still need to defer their work. It's not the right fit for this runtime.
I know your concern is more future-thinking and looks towards a scenario where we have real multithreading. My expectation is that when/if we get to that, we'll be in a position to recreate the same threading semantics as Server pretty much exactly (assuming we want to), but that's not the case today and shouldn't be considered in scope for fixing this bug. My goal in this PR is to wipe out the largest class of possible inconsistency issues I can while not regressing compat or perf.
ghost
added this to the
Fixes #30070, which is another case of execution order inconsistency between Blazor Server and WebAssembly.
Rather than fixing it by putting in another special case, I again wanted to design this class of bug out of existence by bringing WebAssembly hosting more closely into alignment with Server hosting. I say "again" because that was also the approach in #31612, but evidently there are still cases where I didn't go far enough. So now in this PR I'm going significantly further still by taking this out of the renderer and making a general-purpose work queue for all JS-to-.NET calls.
The solution here now simultaneously solves all three of the WebAssembly-specific event ordering problems we've had, and makes it possible to remove a lot of tricky special-case stuff we were using before. Hopefully it also has no perf or compat drawbacks for developers. It looks like not much code but it took a lot of thinking to reduce it to this, especially around things like exception handling. It's still not easy to reason about so please do say so if you have any concerns about whether it's really correct.