Ensure ComponentState is always disposed

[SteveSandersonMS]

Addresses an issue where RenderTreeFrame[] pooling became ineffective because ComponentState instances were only disposed if the component itself was IDisposable (and not IAsyncDisposable).

The approach is:

• In Renderer, stop varying the code path based on different disposability patterns for the component. Just call componentState.DisposeAsync in all cases.
• componentState.DisposeAsync unconditionally releases the buffers before looking at the disposability pattern for the component and calling Dispose or DisposeAsync as appropriate.

[SteveSandersonMS]

Not a breaking change because ComponentState is only becoming public in 8.0.

[davidfowl]

I see.

[SteveSandersonMS]

I looked into ways this could be validated in tests or even at runtime normally, but didn't find a great option:

• If we track the rent/return counts inside ArrayBuilder instances, we can only assert it's gone to zero when something is disposing the ArrayBuilder, which is pointless because that's the case that's always going to work. What we want to detect is cases where it does not get disposed.
• We can't track/check it statically, because then unrelated Renderer instances would interfere

We would need to do something like treat the renderer as the boundary for a context in which ArrayBuilder can be instantiated, so they all register themselves with the owning renderer. Then when the renderer is disposed it could look at all the associated ArrayBuilder instances to check they have been disposed. However that involves a lot more tracking, and we don't generally put in these heavy guardrails everywhere to verify things get disposed, so it doesn't seem like the right thing to do.

We do have unit tests already that show component instances do get disposed during renderer disposal. Since this PR changes the disposal logic so that components are only disposed through disposal of their ComponentState, we do effectively have at least that level of coverage.

[javiercn]

We do have unit tests already that show component instances do get disposed during renderer disposal. Since this PR changes the disposal logic so that components are only disposed through disposal of their ComponentState, we do effectively have at least that level of coverage.

Could we add some tracking logic on our TestRenderer to know when a ComponentState instance gets created and when it gets disposed?

That should give us a way to track this in tests, isn't it?

[javiercn]

Looks good, just one comment about the tests

[SteveSandersonMS]

Could we add some tracking logic on our TestRenderer to know when a ComponentState instance gets created and when it gets disposed?

It's a good idea, so I've done it.

However this has led to some further changes, so please re-review:

• I've had to make ComponentState's DisposeAsync virtual, as otherwise the tests can't observe the disposal. At first I was going to decline to add the tests on this basis, but on more consideration, it makes sense that if a renderer can subclass ComponentState, it may have reasons to want to do things when those instances are disposed. I'm trusting that the overhead of virtual dispatch isn't noticeably expensive here, or that if it is, it will show up in the benchmarks (particularly for WebAssembly).
• This means we now must call ComponentState's DisposeAsync in all cases. Previously we didn't call it when disposing into a RenderBatch, as that was a separate code path and independently dealt with clean up everything (correctly as far as I can tell). However that would now violate the expectations of anyone subclassing ComponentState and overriding DisposeAsync. So I've had to change the dispose-during-renderbatch code paths.
  • Previously there was both TryDisposeInBatch and DisposeInBatchAsync implemented independently. I've merged these into a single DisposeInBatchAsync, and removed most of its guts so it just calls DisposeAsync instead of doing its own cleanup.

I think the end result is much clearer - this collapses about 6 different disposal code paths into just two (during a renderbatch, and during renderer teardown). The only drawback is that DisposeAsync is now virtual but there's a good argument for that being more correct anyway.

[javiercn]

I think the virtual call on ComponentState is fine. Hopefully the JIT is able to see that there is only one implementation and devirtualize the calls.

[SteveSandersonMS]

/backport to release/8.0-preview5

[github-actions]

Started backporting to release/8.0-preview5: https://github.com/dotnet/aspnetcore/actions/runs/5070919992