[Blazor] Improvements to the interaction between SSR and interactive rendering

[MackinnonBuck]

Improvements to the interaction between SSR and interactive rendering

Makes the following improvements to how SSR and interactive rendering interact:

• Detect and initialize interactive root components added via an SSR update
• Cleanly synchronize SSR updates with existing interactive components
  • Updated parameter values get supplied
  • Components get disposed when removed from the DOM

Fixes #48763
Fixes #48764

[MackinnonBuck]

Also TODO is writing thorough E2E tests that capture all the edge cases. I've updated the test app to demonstrate a mainstream scenario, but there are a lot of intricacies that still need validation.

[MackinnonBuck]

The requirement to deserialize descriptors in the order that they were rendered (and without skipping any) doesn't map perfectly to this new rendering model. Descriptors could be skipped due to being replaced or removed on the page before getting activated as interactive components. Furthermore, some situations warrant handling descriptors in the wrong order. For example, components added during a streaming update don't become interactive components until the streaming request completes. However, already-interactive components should receive parameter updates immediately.

Fundamentally, we are trying to make sure that descriptors are legitimate and can't get used more than once. Data protection should guarantee the legitimacy of the descriptor, so we would just need to find an alternative way to guarantee that they don't get reused.

[SteveSandersonMS]

we would just need to find an alternative way to guarantee that they don't get reused.

Would it be enough just to have a HashSet<int> on RemoteRenderer tracking the ones we've consumed? If we know DataProtection ensures the client can't make up new ID numbers on its own, then the total size would be bounded.

[MackinnonBuck]

Something like that should work. The other thing to consider is that the sequence ID will start over at 0 for each new HTTP response, so we'll have to pair it with the invocation ID (which is unique to the HTTP response) as well.

If we're worried about the hash set becoming too large, we could make this slightly better by adding a rule where after we deserialize a descriptor with an unseen invocation ID, all previously seen invocation IDs become invalid. So, you have two hash sets, one that stores consumed invocation IDs, and one that stores consumed sequence IDs for the current invocation ID.

[SteveSandersonMS]

Would it be practical to switch over to use RootComponentManager in all cases? Also on CircuitManager.ts.

[MackinnonBuck]

Possibly - the only downside I can think of is that when using blazor.server.js or blazor.webassembly.js, there would be an added interop call after app startup to add root components, rather than having them be added instantly on startup. I'm not sure if this difference in behavior is meaningful, but I thought I'd play it safe to minimize how much gets changed in this PR.

[SteveSandersonMS]

These sorts of changes could be eliminated if we had an abstraction over the DOM APIs here that presented a view of the world that incorporates "startmarker ... contents ... endmarker" as a single logical node. I think that's something you mentioned you'd be considering doing when we chatted.

[SteveSandersonMS]

The existing LogicalElements APIs might be a sufficient representation, as they already have the concept that marker comments can be used to represent a single logical node with children. We just need to take the static content, find all the marker nodes in them, and upgrade them to be LogicalElements in the same way that toLogicalRootCommentElement already does. Then the logic in DomSync would need to use the LogicalElements APIs when walking the DOM so it sees it with respect to that view of the world.

Part of the reason why synchronizeDomContent accepts a CommentBoundedRange as a way to define the parent node is for precisely this case - we may want to treat a logical element as being the parent.

[MackinnonBuck]

I think this PR is in a good place now. The big thing that I still need to do is write E2E tests, which is what I'll be focusing on over the next few days. That said, I've been validating these changes with some manual testing scenarios (one of which is included in this PR), so I'm fairly confident in the overall stability of this feature.

[MackinnonBuck]

The changes in toLogicalRootCommentElement and toLogicalElement make it so that if a node is a LogicalElement, so are all its descendants. It also fixes an issue where if multiple component descriptors were direct children of the same DOM node, converting them both to logical elements resulted in duplicate children in the root logical element's child array.

[MackinnonBuck]

The updated DOM sync algorithm heavily relies on a sound logical element hierarchy, even for prerendered content, hence these changes. This wasn't really a requirement in the past because the only operation we did on prerendered content was remove it, and we had a special "logical end sibling" property to help with that case.

[SteveSandersonMS]

The sheer amount of sophistication and complexity in this is a bit of an alarm. Perhaps I'm missing key scenarios but all I thought we needed to do was:

• During DOM sync, when we're walking through a list of sibling elements, if we ever see an interactive component marker:
  • Treat that as a distinct new element type with its own separate logic for insert/update/delete (i.e., insertions/updates/deletions are operations on the corresponding interactive root component)
  • Then continue the walk by skipping to the node after the "end" marker, if there is one

I suggested that LogicalElement might be a helpful abstraction for seeing the DOM in a way that already has helpers for getting "next sibling" etc with respect to this virtual hierarchy. However we should only use that if it really is helpful and convenient.

The impression I get here is that interactive rendering's use of LogicalElement has pretty different requirements to the usage during DomSync. As such, trying to unify this into an all-encompassing system might be forcing a lot of complexity and perhaps would compromise on perf too (e.g., needing this new rule that "if a node is a LogicalElement, so are all its descendants").

I appreciate there isn't a lot of free time to finish this off! However we do have to keep maintaining DomSync.ts and LogicalElement.ts in the long run. TBH LogicalElement already was scary to me before this change because it's really intricate and has so many special cases that I can't remember, and I just trust that it's so battle-hardened by now that all these special cases do actually work together in any combination that can actually occur. There would be a good case for not expanding it to accommodate DomSync if DomSync's requirements can actually be met with a simpler alternative.

What do you think?

[MackinnonBuck]

There's finally a valid case for both these conditions now (moving an logical element and its children from one tree to another).

[MackinnonBuck]

There is a bit more complexity in the DOM sync algorithm now that we could be dealing either with logical elements or plain old DOM nodes. One potential simplification would be to always normalize the incoming content to be a logical element hierarchy, but that means Blazor effectively owns the entire document, meaning developers can't directly manipulate any part of the DOM without risking breaking DOM sync functionality.

We could alternatively introduce additional abstractions that more cleanly separate the "logical element" logic from the "plain old DOM node" logic. I've done this in places where there's an obvious separation between how the two node types are dealt with (e.g., walking the edit list), but there's also quite a few places where the logic is exactly the same except for a tiny detail, in which case adding an abstraction just creates noise. I think I've struck the right balance here, but I'm open to improving it if others feel there is a need.

[SteveSandersonMS]

Entirely mixing together the DOM sync algorithm with special-case knowledge of logical elements representation and the interactive component markers is not ideal and will make this pretty hard to understand and maintain in the future. DOM syncing is already fairly sophisticated so it would be extremely valuable to layer this in a way that the interactive component marker handling and logical element walking is handled externally to this code.

Is there a way of adding a bit more abstraction to DomSync.ts so that it doesn't need to know about interactive component markers and the difference between logical and physical elements? What you have with EditWalker is a good start - maybe the synchronizeDomContent export itself could be defined to receive some abstraction like that as the input and output data.

There is good test coverage for the existing DomSync.ts behavior in src\Components\Web.JS\test\DomSync.test.ts, so anything we add/change in DomSync should be reflected in unit tests. The ultimate ideal would be if it didn't really gain new behavior at all besides accepting a more general abstraction to represent the DOM structures it's working over!

What do you think? Or is it impractical to add an abstraction that's still able to deal with some necessary special cases?

[javiercn]

Have we measured the chances of collision here? GetHashCode is not collision resistant.

[MackinnonBuck]

I left a comment about that here. I'm definitely open to improving this, because a collision is definitely possible.

We could reduce the possibility of a collision by generating a string that separately includes the following:

• Sequence number
• Key hash code
• Type name hash code

We could even go further and obtain a SHA1 hash for the type name.

[javiercn]

I think we should still send the type name + sequence number without hashing. We can hash the key if there is any, but if we are using this for comparison, it feels like we are shooting ourselves in the foot computing a "digest" for the key when we have all the data.

If we are concerned about the size of the TypeName, we can create a Map for each type that we render as a server component on the fly that guarantees their uniqueness, but I would start with just sending the data without losing any information.

[MackinnonBuck]

If we send the full type name, is it fine that it might be the type of a server-only component? I wasn't sure if that would be considered a security risk. e.g., Company.App.SomeComponent_AndByTheWayThePasswordIsFoo.

[javiercn]

It's inside a data protected payload, isn't it?

[SteveSandersonMS]

Yeah, this key is being read on the client, not just reflected back to the server. So sending a hash of all the data sounds right to me.

It does potentially still disclose some information to the client. This hash isn't a cryptographic function, and if the client knows the set of inputs is restricted in some way (e.g., if they somehow know the @key is an int or is a string with less than 5 chars, say) they might be able to brute force the inverse of the hash.

However it would be very unlikely for an app developer to put a sensitive secret as the input to @key, and even if they do, almost all secrets have a lot more than 32 bits of entropy and hence a 32-bit hash still leaves most of the entropy undisclosed. So my reading is that this likely fine. Any opinions that this is wrong? I guess if there was a concern we could mix in some other per-component strongly random data.

[MackinnonBuck]

@SteveSandersonMS I agree that from a security perspective, it's probably fine to hash the key.

What do you think of the concern about hash collisions? Do you think it would be fine to alternatively send the full name of the component type to the client, and possibly suggest/require that the key implements IFormattable?

[SteveSandersonMS]

I was thinking about hash collisions a bit yesterday.

It's not as risky as it would be if we needed every hash to be unique - if that were the case then a list of only ~4500 items would have a > 50% chance of a clash (via birthday paradox maths). We only need the keys to be unique enough that the edit operations that happen in practice don't run into clashes. If the edits are deletions/insertions, then we only need the next/preceding item not to have the same hash as the inserted/deleted item. So the probability of this being violated is simply 1/2^32. If the edit operation was randomly shuffling a list, then the risk is harder to compute (I lost interest after a few minutes...).

alternatively send the full name of the component type to the client

I don't think that helps much since the sequence number almost always uniquely identifies the type anyway. In fact it always would if the code was compiled from .razor, since there's no way in Razor syntax to render a child whose type varies without the sequence number varying. The one case where this is not true is if you deploy a new version of your app in between the two requests - that's the one edge case I think it's worth retaining a hash of the type name for.

possibly suggest/require that the key implements IFormattable?

Yeah, that is a good possible solution. We could say that we will only include the @key value in the marker if it implements IFormattable (and if it doesn't, that's not an error since it may be used for interactive rendering, but we simply ignore the key in that case).

The other good thing about this is it avoids false negatives. That is, if someone was using @key=someModelObjectFromTheDatabase which is completely valid and useful in interactive rendering:

• If we use (type, sequence, key?.GetHashCode()), it will produce a different output on every request, perhaps causing us never to retain the interactive components
• If we only use (type, sequence, (key as IFormattable)?.ToString()) then we will err on the side of retaining interactive components, and @key becomes a way to optionally prevent unwanted retention in a predictable way. It also guarantees no false positive matches.

So, your IFormattable suggestion sounds like a better solution to me.

[SteveSandersonMS]

The one case where this is not true is if you deploy a new version of your app in between the two requests - that's the one edge case I think it's worth retaining a hash of the type name for.

Actually forget that - it's an irrelevant scenario. If there is a new version of the app between the two requests, it would only affect WebAssembly because that's the only kind of interactive component that could survive a server restart.

It is still relevant to avoid accidental matches based on sequence alone because the circuit would reject any incoming parameters for a componentId that doesn't match the component type encoded in the Data Protected payload[1]. So there'd be an exception if there's an accidental match for a different component type. And that could happen because when we merge the DOM trees, some parent component might have contained an @if/@else branch that outputs two identical DOM structures that place two different component types at the equivalent output location and happen to have the same sequence number at that point in rendering.

So, to make things much more robust in the default case where there's no @key, I still think we should be including some information based on the component type. We could use the full type name if we are OK with disclosing that. Or if not, we'd want some stable value derived from it. But then we have to decide if we rely on SSR client-server affinity or not. If we want to allow for a cluster of SSR servers without client affinity, we cannot use either:

• A Type->number lookup table maintained in server memory
• type.FullName.GetHashCode(), since GetHashCode isn't required to be stable across processes

Instead we'd need a stable, server-independent hash of the type name. For example the server could maintain a cache of Type->SHA(type.FullName).

[1] We do verify and enforce that, right?

[MackinnonBuck]

Great analysis, @SteveSandersonMS!

It is still relevant to avoid accidental matches based on sequence alone because the circuit would reject any incoming parameters for a componentId that doesn't match the component type encoded in the Data Protected payload... We do verify and enforce that, right?

Yes, if there is a type mismatch between the type in the data protected payload and the type of the component with the supplied ID, we reject the parameter update (see line 130 below).

aspnetcore/src/Components/Server/src/Circuits/RemoteRenderer.cs

Lines 123 to 136 in d339790

	var componentState = GetComponentState(componentId);
	if (!_serverComponentDeserializer.TryDeserializeSingleComponentDescriptor(operation.Marker, out var descriptor))
	{
	throw new InvalidOperationException("Failed to deserialize a component descriptor when updating an existing root component.");
	}
	if (descriptor.ComponentType != componentState.Component.GetType())
	{
	Log.InvalidRootComponentOperation(_logger, operation.Type, message: "Component type mismatch.");
	return;
	}
	_ = RenderRootComponentAsync(componentId, descriptor.Parameters);

However, it's totally possible for the client to lie about the component ID if it happens to correspond to another root component of the same type. It would be tricky to come up with a mechanism to prevent that from happening, because it's the client who decides which SSR/interactive components match up anyway.

type.FullName.GetHashCode(), since GetHashCode isn't required to be stable across processes

Interesting... I didn't know that was the case for strings. Good to know!

For example the server could maintain a cache of Type->SHA(type.FullName)

This seems like the right approach to me.

Based on this discussion, it's looking like we'll generate a key with:

• The sequence number included directly
• A cached SHA for the component type name (maybe a truncated hash if we want to reduce payload size, as long as it doesn't make the risk of collision unacceptable)
• The formatted @key, if it's provided and implements IFormattable

[javiercn]

Is the idea here that @key is "best effort"? Meaning:

1. It's not required.
2. It might not produce a unique value.
3. It might not produce a stable version between renders.

Or do we need it to produce a stable identifier for it to work. If not, what is the outcome?

[MackinnonBuck]

It's not required.

Correct. If it's not supplied, the sequence number and component type will be used to distinguish between component "instances".

It might not produce a unique value... It might not produce a stable version between renders.

Right, if a @key is provided, it needs to produce a stable identifier. Maybe we could require that they @key implement IFormattable, and we use the formatted output in the key we generate for the marker. And if the @key happens to not implement IFormattable, we fall back on the hash code and log a warning. Worst case, the unstable key causes the component to get reinstantiated, but it shouldn't break app functionality in general. Or, alternatively, we could throw an exception to minimize the risk of shipping an app with this mistake.

[MackinnonBuck]

I've added quite a few tests now and updated the marker key generation per the discussion in this PR.

[SteveSandersonMS]

Writes to a Dictionary<T,U> aren't threadsafe according to the docs so should this be a ConcurrentDictionary? Or do you know that this particular TryGetValue+Add pattern would be threadsafe?

[SteveSandersonMS]

Also a style nitpick: we don't usually see this s_ prefix on statics in most of our code (none of the recent code anyway)

[MackinnonBuck]

Yeah, this should be a ConcurrentDictionary

[SteveSandersonMS]

Would it also work to add a test here showing the server won't let you add too many root components without disposing old ones? Not sure if that enforcement happens within the relevant layer for this unit test though.

[SteveSandersonMS]

This doesn't seem ideal for performance (it's rescanning the entire document after each <blazor-ssr> element is added), and I'm not sure it's right for correctness either.

Don't we need to wait until streaming SSR is entirely finished before we start activating interactive components? The prerendered content may include ComponentState generated during an async loading process. The developer won't want interactive components to light up until they have that state, as the whole point of transferring this state is so that during interactive rendering the component doesn't show up in a loading state or duplicate the cost of loading the same data again.

Apologies if I'm misinterpreting what's going on.

[MackinnonBuck]

Yeah, I might not have done a good job picking a descriptive name for that callback. It actually doesn't re-scan the document for <blazor-ssr> elements; its purpose is to notify the RootComponentManager (or whoever else is interested, in theory) that the document has changed. In the case of RootComponentManager, this callback causes it to check each of the component descriptors it already tracks to see if they have been updated or removed (and, if a stream render is not underway, create interactive components for each descriptor that hasn't yet been handled).

Don't we need to wait until streaming SSR is entirely finished before we start activating interactive components?

Yes, and the current implementation does that. But I do think it would be useful to update parameters for (or remove) already-interactive components. The callback is just a way to signal to whoever's interested that the document has changed and some root-components related operations might need to occur. The actual scanning for component comments happens in the DOM sync algorithm for incoming content, so by the time the content gets sync'd with the document, the RootComponentManager is already aware of any new SSR'd components.

[SteveSandersonMS]

I have to admit I'm not really a fan of having many different calls to documentUpdated from lots of different points in the flow. It looks pretty fragile, like we might either add it somewhere it shouldn't go, or forget to add it in some new place, and then the behavior will be unpredictable.

Ideally we would only need to activate interactive components in a fairly constrained set of places:

• When the initial document load or a subsequent enhanced navigation is entirely completed, but only if there's no ongoing streaming rendering and no other enhanced navigation has started
• When some streaming rendering completes (as in, entirely completes, not just a single chunk of it)
• (Potentially) when the WebAssembly runtime completes its async loading process, if that was triggered by having seen a webassembly component marker in one of the two cases above

I don't think we have to deal with cases that are in the middle of a process, since we don't want to activate interactive components until streaming SSR has entirely stopped (because if we did, we wouldn't be able to wait for ComponentState).

I don't think we have to deal with these completely fatal error cases (replaceDocumentWithPlainText) either, since there's no going back from these - the page is permanently dead and the user will just have to leave it, which will dispose everything.

What do you think? I appreciate I might be missing some subtleties about why it's been done this way.

[SteveSandersonMS]

If the scenarios listed above are accurate, then instead of passing around a collection of callbacks, we could define some concept like a "SSRLoadingState" whereby:

• Code can call SSRLoadingState.addWorkItem(somePromise) to represent a thing that's happening, ideally at the top level of an async process like "perform enhanced nav including streaming SSR"
• Boot.Web.ts or whenever can register a callback like SSRLoadingState.onLoadingStateChanged(callback) that fires when we move from 0 pending work items to >= 1, or when we move from >= 1 to 0.

Feel free to ignore this if it's irrelevant. I was just trying to make a constructive suggestion but maybe I'm missing the reasons why it needs notifications at each step in an ongoing process.

[SteveSandersonMS]

Update: @MackinnonBuck explained I was missing what's really going on here. documentUpdated does not scan the document nor activate any new interactive components (that only happens during DOM sync). Instead, documentUpdated notifies the root component manager that it may need to update the parameters on existing root components, since we do want those to be updatable at arbitrary points during streaming SSR. However it might be cleaner still to centralize this so that the calls to RootComponentManager happen during DOM sync (since at that point we already know precisely which marker is being updated), and then RootComponentManager can debounce these calls so it only results in a single message to the server. Then we don't need documentUpdated at all (or at least, not in these cases).

[SteveSandersonMS]

We discussed verbally and decided that:

• There may be some opportunities for refinement that @MackinnonBuck can do today (e.g., maybe centralizing the documentUpdated callback mechanism)
• But we don't want any polish phase to take too long here, since we're on a deadline for preview 7. So I'll approve this now and you can merge whenever you feel it's ready, based on how much time you have available.
• Later, we may explore other possible ways to decouple the DomSync logic from the internals of LogicalElement and component markers. I may look at this a bit tomorrow if I have time.

[MackinnonBuck]

Thanks for all the helpful feedback, @SteveSandersonMS and @javiercn.

Some follow-up items for RC1:

• Review the strategy we use to ensure clients are well-behaved when adding interactive root components
• Determine if we can simplify the DomSync algorithm to not care about the existence of logical elements. Or, if it does have to, use different layering to separate out logic specific to component comments and logical elements.