-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fix event and JS component args serialization #35038
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 event and JS component args serialization #35038
Conversation
…erop-based mechanism
@@ -44,10 +44,6 @@ public void Add(string name, object? value) | |||
/// <returns>The <see cref="ParameterView" />.</returns> | |||
public ParameterView ToParameterView() | |||
{ | |||
// Since this is internal, we should expect the usage to always be correct, | |||
// i.e. the given count should match the number of 'Add' calls. | |||
Debug.Assert(_frames[0].ElementSubtreeLengthField == _frames.Length); |
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 check was unhelpful because the capacity needs to be understood as a max capacity, not a guarantee that it will be filled entirely. I realised that if the JS-side code tries to supply undefined
, because of JSON serialization conventions, that parameter would be omitted, so it's possible for the number of supplied parameters to be less than the total count.
// From this point onwards, JavaScript code can add root components if configured | ||
await Renderer.InitializeJSComponentSupportAsync( | ||
_options.RootComponents.JSComponents, | ||
JSRuntime.ReadJsonSerializerOptions()); |
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 kind of initialization is now done from the WebRenderer
constructor instead, so it's common to all.
/// </summary> | ||
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public class CircuitJSComponentInterop : JSComponentInterop | ||
internal class CircuitJSComponentInterop : JSComponentInterop |
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 no longer needs to be public because it's no longer called directly through JS interop. It's now called via the new WebRendererInteropMethods
thing.
@@ -93,8 +75,7 @@ public virtual int AddRootComponent(string identifier, string domElementSelector | |||
/// <summary> | |||
/// For framework use only. | |||
/// </summary> | |||
[JSInvokable] | |||
public void SetRootComponentParameters(int componentId, int parameterCount, byte[] parametersJsonUtf8) | |||
protected internal void SetRootComponentParameters(int componentId, int parameterCount, JsonElement parametersJson, JsonSerializerOptions jsonOptions) |
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 changes in this method are all because the parameters now arrive as a JsonElement
instead of a byte[]
, because JS interop can supply a JsonElement
natively without us having to manually do anything at either end.
/// <summary> | ||
/// For framework use only. | ||
/// </summary> | ||
public int BrowserRendererId { get; set; } |
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 gone because the BrowserRendererId
used to be how WebAssembly would distinguish events for different renderers. That's no longer needed because there's now a separate DotNetObjectReference representing the interop methods for each renderer.
// pending byte arrays synchronously after the call. This also helps because the recipient isn't | ||
// required to consume all the pending byte arrays, since it's legal for the JS data model to contain | ||
// more data than the .NET data model (like overposting) | ||
jsRuntime.ByteArraysToBeRevived.Clear(); |
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.
Previously we cleared this set before invoking the user code via JS interop. But now we do it synchronously after. This is to allow the callee to retrieve byte arrays during custom deserialization. I don't think there are any drawbacks to this change.
} | ||
|
||
[JSInvokable] | ||
public Task DispatchEventAsync(JsonElement eventDescriptor, JsonElement eventArgs) |
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 fact that we can receive JsonElement
as JS interop params is a cool thing I hadn't even realised would work before. It works because STJ will natively supply a JsonElement
if you ask it to deserialize to that type.
This means that anywhere we need to do custom deserialization, we don't actually need to have manual-serialize-and-write-to-UTF8-array code on the JS side, as we can simply send normal params from JS and receive them as JsonElement
on the .NET side. This simplifies things a lot.
} | ||
|
||
base.Dispose(disposing); | ||
} |
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.
For a change, we actually have an unmanaged
resource (the DotNetObjectReference). If you really wanted you could use the Dispose pattern.
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 aspect we really care about cleaning up is the .NET storage of the DotNetObjectReference
, so it's really more of a managed resource. TBH it doesn't actually matter if we don't clean this up at all because if the renderer is going away, so is the JS runtime instance, which is where these things are stored. I'm only doing the disposal here like this because it's conventional to do so and probably easier to understand than not doing.
@@ -242,21 +242,6 @@ public async ValueTask<bool> ReceiveJSDataChunk(long streamId, long chunkId, byt | |||
return await circuitHost.ReceiveJSDataChunk(streamId, chunkId, chunk, error); | |||
} | |||
|
|||
public async ValueTask DispatchBrowserEvent(JsonElement eventInfo) |
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.
Removing this method gives me a pause - I can't find the history for this change (it ends at this PR - #12250), but I vaguely recall we used to plumb events thru JSInterop but special cased it at as a perf optimization. Are we losing out on those benefits as part of this change?
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 only difference we had before by not using JS interop was more scenario-specific logs, which I don't think are very consequential. As for perf, it's not going to be meaningfully different because even before this change, we were JSON-serializing the data before sending it over BlazorPack, which is still equivalent to what JS interop does.
But the more fundamental issue is that is has to go via JS interop if we want it to also have JS interop features like transporting byte[]
or DotNetObjectReference.
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.
Update: I should be clearer about this because there is some extra cost in going through JS interop, but (1) it's the same as we had in 5.0, so not a regression - just not an improvement either, and (2) this sets us up to improve it further in the future if we want.
Overview for Blazor Server:
Version | How event dispatch worked | Summary of transformations |
---|---|---|
5.0 | JS sent JSON strings; .NET received .NET strings then called STJ to build .NET objects | JS JSON string -> Encode as UTF8 for wire -> Decode as .NET string for hub method -> Encode as UTF8 for JSON parsing -> .NET EventData object |
6.0-pre7 | JS sent UTF8-encoded bytes; .NET received bytes then called STJ to build .NET objects | JS JSON string -> Encode as UTF8 for wire -> Receive as UTF8 on hub method -> JSON parse to .NET EventData object |
This PR | JS sends JSON strings; .NET receives .NET strings then our JS interop code serializes as UTF8 and our custom deserialization logic processes the JsonElement over it | JS JSON string -> Encode as UTF8 for wire -> Decode as .NET string for hub method -> JSInterop does own UTF8 encoding and passes that to STJ -> JsonElement -> .NET EventData object |
So in effect, this PR removes an optimization we had previously added in a 6.0 preview and takes us back to the same perf characteristics we had in 5.0 and before, in that it goes back to passing the data via a .NET string representation instead of staying as UTF8 all the way through.
For WebAssembly, this PR doesn't change the perf characteristics either, because it was already doing event dispatch via JS interop.
Of course it would be nice if we didn't have to go through that extra representation, but it's fundamental to how JS interop is defined in terms of exchanging strings (JS strings and .NET strings, not UTF8 bytes). But the good news is that following the layering properly like this PR does means we can improve that layering in the future and would automatically get benefits here. We could eliminate the .NET string representation entirely, as suggested at #35065.
null, | ||
"DispatchEventAsync", | ||
dotNetObjectId, | ||
JsonSerializer.Serialize(new object[] { descriptor, eventArgs }, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase })); |
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'm doing these Ignitor updates to make the Ignitor tests still roughly make sense and not further away from working than before, but they are all quarantined and completely broken even before this PR, as they all time out waiting for renderbatches. Fixing Ignitor completely (or more likely removing it, as we've discussed) is out of scope for this PR.
The change you see here retains a meaningful way to dispatch events, since the old way is not longer applicable.
Fixes #34338
The issue was that event dispatch did not get sent via JS interop, but instead had different custom transports for each of the hosting platforms. As a result, features like sending byte arrays and DotNetObjectReference did not work, because they are features of JS interop.
The solution here is a pretty significant set of refactorings that replace the three platform-specific event dispatch mechanisms with a new single mechanism based on JS interop. It also means we can consolidate and simplify a lot of the implementation. For example:
InitializeJSComponentSupportAsync
APIs and the three platform-specific implementations are all goneSo, this PR removes more lines than it adds 🎉. But it is still a lot of change!
This change also means we can now send byte arrays and DotNetObjectReference as parameters to JS root components, since I've also re-platted that onto this new JSinterop-based thing.
The only user-visible behavioral change apart from the new functionality is that Blazor Server no longer has extra custom log messages related to events, since events are now just JS interop calls. I don't think this is a loss.
Testing
I have updated the E2E test app but haven't yet updated the Selenium test scripts. Working on that next.