[release/8.0] [Blazor] Close the circuit when all Blazor Server components are disposed

[MackinnonBuck]

[Blazor] Close the circuit when all Blazor Server components are disposed

Allows a Blazor Server circuit to close when all root Blazor Server components get dynamically removed from the page.

Description

The overall approach I've taken is:

1. Define what it means for the circuit to be in use (WebRootComponentManager.hasAnyExistingOrPendingServerComponents()):
   • There are interactive Blazor Server components on the page, or...
   • The initialization of an interactive Blazor Server component has started, but hasn't completed yet, or...
   • There are SSR'd components on the page that haven't been initialized for interactivity yet (consider stream rendering, where we don't activate new components until the response completes), but they have either a "Server" or "Auto" render mode
2. Determine the cases where a circuit's "in use" status may have changed:
   • After a render batch is applied (see attachCircuitAfterRenderCallback in WebRootComponentManager.ts)
     • An applied render batch may result in the creation/disposal of a root component
   • After an SSR update occurs, but before the first render batch is applied
     • Consider the case where an SSR'd component with a Server render mode gets removed from the page, but before the circuit has a chance to initialize
3. Decide what to do if the "in use" status may have changed (WebRootComponentManager.circuitMayHaveNoRootComponents()):
   • If the circuit is not in use:
     • Start a timeout with some configurable duration (SsrStartOptions.circuitInactivityTimeoutMs), if it hasn't started already
     • When the timeout expires, dispose the circuit
   • If the circuit is not in use:
     • Clear any existing timeout

This PR also:

• Addresses a bug preventing Virtualize from working correctly when a WebAssembly and Server instance is present on the page at the same time
• Adds E2E tests

Fixes #48765

[MackinnonBuck]

I appreciate that logic like this can be a bit hard to follow. Unfortunately, we have a recurring problem in Blazor's JS implementation where an assumption was previously made that certain state should be "static" / "singleton" / "module-level", when in fact it's actually better represented as being instance state with a limited lifetime.

In this example, it would be better if there was a more concrete concept of a "circuit" instance on the JavaScript side, where that circuit can be disposed and a new one can be created. This allows a separate copy of state to exist on each circuit instance, so you don't end up with tricky synchronization/ordering challenges during disposal/initialization.

Unfortunately, these kinds of challenges already existed in this file with the disconnection/reconnection logic. It would be possible for us to through and rework this file to use instance-level state (rather than module-level state) at the risk of us potentially changing something subtle in the existing initialization/reconnection logic behavior. Maybe it's worth doing this now, or maybe this is something we should look into more closely for .NET 9.

[SteveSandersonMS]

Yeah, this is tricky. I do think the Web.JS code during .NET 8 has gone from "of mixed quality" to "needs a very thorough rewrite if we're going to maintain and expand this long term". It's always been a lower priority than the C# that runs server-side, and its smallness used to let us get away with idiosyncrasies, but it's no longer small and gets ever more spaghettified.

So I would say:

• For .NET 8, let's follow your judgement about how much tidying is needed in the short term. We need correctness and to avoid too much weirdness that will make debugging difficult (especially given we may still be patching 8.0 three years from now).
• For .NET 9, we should be prepared to eat the cost of a bigger renovation of the JS/TS code. I would hope that in something like 3-4 weeks we could thoroughly go through each part of it and replace each functional area with a more modern, tidy, instance-oriented implementation with some Jest tests. We would come up with sensible new abstractions that link together the major pieces without them all reaching into each other's internals. (In theory we could even attempt a from-scratch reimplementation but that would be braver.)
  • I mean all of it, including rendering, event handling, etc. - not just circuit management.
  • A major asset for us is the E2E tests which are really thorough in most aspects, so we should have good confidence about correctness. That's even more so now a lot of the WebAssembly startup flow is outside this repo, so we no longer have to worry about recreating all the semantics around loading and any errors that can occur.

@MackinnonBuck What do you think? If you think we should be tackling any of this sooner, or if you have views on the best approach to renovate this in .NET 9, that's definitely worth discussion.

@mkArtakMSFT What do you think? Would you be prepared to put that much investment in this area in .NET 9? You could think of it as being similar to the big project to align the WebAssembly SDKs - we found time for that in .NET 8 so hopefully we have capacity for a similarly large investment in long term code quality in .NET 9.

[MackinnonBuck]

@SteveSandersonMS I definitely agree that a thorough rework of the JS/TS code is required, and I think this is something we should prioritize early in .NET 9, especially if we intend to do some of the other tracked blazor.*.js improvements to support things like microfrontend scenarios, etc.

Regarding circuit management, I've decided to update the implementation now to significantly clean up Boot.Server.Common.ts. A lot has been moved to CircuitManager.ts, which now logically represents the state and lifetime of a single circuit (whose connection can be closed and reinitialized an arbitrary number of times). While it's a fairly large change, it should be easier to reason about and fix bugs going forward.

[SteveSandersonMS]

An applied render batch may result in the creation/disposal of a root component

I thought that since render batches (on the client) imply interactive rendering, and interactive components can't contain other interactive root components, this shouldn't be the case. Could you clarify what, if anything, I'm missing? Thanks!

[MackinnonBuck]

I thought that since render batches (on the client) imply interactive rendering, and interactive components can't contain other interactive root components, this shouldn't be the case. Could you clarify what, if anything, I'm missing? Thanks!

A render batch is still sent to the browser for root component disposal, which causes the component to be unregistered as a root component in BrowserRenderer.ts. We could close the circuit immediately after the last Server root component gets removed via an SSR update, but this doesn't account for the possibility of dynamically added root components via JavaScript.

[SteveSandersonMS]

That's an excellent thing to anticipate. Nice!

[javiercn]

Don't worry folks, the TS39 has you covered https://github.com/tc39/proposal-explicit-resource-management#motivations

[SteveSandersonMS]

A render batch is still sent to the browser for root component disposal, which causes the component to be unregistered as a root component in BrowserRenderer.ts. We could close the circuit immediately after the last Server root component gets removed via an SSR update, but this doesn't account for the possibility of dynamically added root components via JavaScript.

I'm surprised that WebRootComponentManager doesn't already know about root components being removed (through all means). Is that not the case?

The only reason I'm asking is that rendering happens thousands of times more often than the set of root components changing, possibly even at 60fps or more (with any number of batches per frame), so taking work out of the core render cycle has benefits. Of course, if the amount of work in checking for an empty set of root components is near-zero then it wouldn't matter.

[SteveSandersonMS]

Looks good!

The sheer amount of changes to how things are wired together is definitely beyond what I can build a mental model for while reading the code, so I definitely can't predict whether there are edge cases where things get stale or similar. Long term I think we need to be prepared for a broader restructure, but I definitely appreciate right now you're making the best choices about working within the existing system.

If there's anything you feel suspicious or uncertain about whether we know it's correct please say so and I'll be happy to help reason about the edge cases.

[SteveSandersonMS]

Also this will need to be retargeted to release/8.0 to go into RC2. It will then be flowed to main automatically.

[MackinnonBuck]

Thanks for the review, @SteveSandersonMS!

I'm surprised that WebRootComponentManager doesn't already know about root components being removed (through all means). Is that not the case?

Unfortunately, it doesn't. It's meant to act as the middleman that keeps in sync SSR'd components and their corresponding interactive components, so it doesn't know anything about root components that get created by other means.

Of course, if the amount of work in checking for an empty set of root components is near-zero then it wouldn't matter.

In the common case where root components do exist, the impact isn't measurable (at least from my testing with performance.now()).