[Blazor] Async disposable support for Blazor

[javiercn]

Currently we only support disposing components synchronously. For disposable components there are several design decissions that we've made along the way:

• Within a render batch we process the disposed child components for a given component as a bundle, and produce a list of errors each component we process.
• We don't however, stop the render batch from running. Instead we simply notify the error and let the renderer deal with it within its handle error implementation.
  • RemoteRenderer will terminate the circuit.
  • WebAssemblyRenderer will log the error to the console.
• A component being disposed can trigger a render somewhere else due to some notification during disposal, for example.

All these behaviors happen synchronously, so that means that a render batch only finishes rendering after all the components have been disposed and there are no more renders to process.

Supporting async disposal brings in several questions:

• Do we wait for disposed child components within a given component render to finish disposing before continuing the work?
• Do we wait to send UI updates until all components have successfully been disposed asynchronously?
• Do we trigger new render batches after each component has completed disposing or do we wait until all of the components disposed in a previous batch have finished disposing?

Overview of the design

• We want disposal to behave in a similar way as event handlers and lifecycle methods.
• If as part of disposing an async disposable component a render is triggered within the syncrhonous piece, that render belongs in the same render batch being currently processed.
• Otherwise, it belongs in a separate render batch.
• Synchronous exceptions within async disposables are reported right away.
• Asynchronous exceptions are reported once the task completes.
• We report all the asynchronous disposal errors for a single render batch at the same time.
  • This simplifies processing, but we can also group them by component instead.
  • That said, synchronous errors will be reported before asynchronous errors are so as to not wait for them to complete.

[pranavkm]

Thanks @javiercn for the description. The design seems consistent with the rest of rendering. Steve would probably know best if this bit is problematic:

If as part of disposing an async disposable component a render is triggered within the syncrhonous piece, that render belongs in the same render batch being currently processed.

But the rest of it looks great.

[JamesNK]

Unit tests?

[SteveSandersonMS]

I'm still going through this PR, but here are initial reactions to the questions. My overall hope is that async disposal behaves like a set of invisible background jobs that nothing is waiting for (except prerendering). If there's an exception we report it, but otherwise async disposal doesn't influence anything else.

Do we wait for disposed child components within a given component render to finish disposing before continuing the work?

No, the inner loop of rendering is all about keeping things synchronous. I don't think anything benefits from knowing when async disposal is finished, other than prerendering which already has a system for tracking incomplete async tasks.

Do we wait to send UI updates until all components have successfully been disposed asynchronously?

Yes. There's nothing to be gained by waiting for disposal (AFAIK).

Do we trigger new render batches after each component has completed disposing or do we wait until all of the components disposed in a previous batch have finished disposing?

Neither. I don't think that the completion of disposal should trigger any rendering. We know that the component that was disposed is no longer in the UI, so what would we render?

Of course I might be missing something important. Please let me know if you think I am!

[SteveSandersonMS]

Thanks for starting the discussion about the design goals here! It's helpful to see a sketch implementation without getting too bogged down in tests.

I notice that quite a bit of the complexity here (and I'm not saying it's crazy complex) comes from tracking the disposals in groups so we can report their aggregate results. Let's consider whether we could drop some of that and make things simpler for ourselves, which might also make things easier to understand for customers.

Specifically, what if we treat each async disposal as a completely independent background task? If any of them fail individually then we report it (immediately, not waiting for the rest of its friends), but otherwise nothing else happens. Then we don't have to deal with any of the aggregate reporting.

I think this might be easier for customers to understand because if you have a lot of async disposal, some members of the group might take ages to complete, or might not even ever complete. Then if you're waiting for the whole group, you'd never see the failure statuses of the ones that did fail sooner.

It does make sense for prerendering (only) to be waiting for all its associated async disposals to complete before we issue the response, because we do want to fail the response if any of the async disposals fail. However we could do that using the existing _render.AddToPendingTasks facility that @javiercn implemented long ago.

[javiercn]

🆙📅

[javiercn]

This is good to go, I think

[SteveSandersonMS]

ComponentState is already internal

Suggested change

	internal Task DisposeInBatchAsync(RenderBatchBuilder batchBuilder)
	public Task DisposeInBatchAsync(RenderBatchBuilder batchBuilder)

[SteveSandersonMS]

Is this any different from result.AsTask()? There's probably some subtlety I'm missing. If so, would you consider adding a comment to clarify what we're doing the await for?

[JamesNK]

Can the entire try/catch be replaced with return ((IAsyncDisposable)Component).DisposeAsync().AsTask();? I don't know what it is adding.

[javiercn]

AsTask probably works, I'm just too used to task and I didn't think of just calling AsTask, I would check though to ensure it does the right thing.

As for the try..catch, we want to avoid the caller having to deal with capturing exceptions, this is a result of us not using async to avoid the state machine. We follow this pattern in several places.

[javiercn]

From what I can see, https://github.com/dotnet/runtime/blob/bf2e135c12cbd34aeba2fa4a31d0e84184041a17/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs#L191-L228 AsTask will do just this, so I think I'll replace the entire thing with that.

@JamesNK thanks for the suggestion!

[javiercn]

Nevermind, I still need the try..catch for when the thing throws synchronously

[javiercn]

Actually, AsTask doesn't work here because when the exception is thrown synchronously the result is never set, which leads to an incorrect result.

[javiercn]

It can be used in the async code path, so I've removed the Awaited bit

[SteveSandersonMS]

LGTM!

[SteveSandersonMS]

Superb - great to see all these test cases!

I did have one minor suggestion (internal -> public in one place) and one question that might warrant adding a comment if possible.

[campersau]

Is the order of calling CleanupComponentStateResources(batchBuilder); before DisposeAsync important? Because in TryDisposeInBatch it is called after Dispose.

[javiercn]

It is not important, but it makes a bit more sense in the async case to do it before instead of after since dispose async can take longer and you don't want to be holding on to resources you aren't going to need. I was conservative and didn't change the behavior on TryDisposeInBatch just in case. Mostly because I don't think it matters in that case.

[zmadkour]

Can you tell me, in which .net core version should I expect this PR? :D

[campersau]

@ZiadMadkour in RC1 see https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-5-preview-8/comment-page-2/#comment-2575