Clarify possible interleaving of component initialization and disposal

kjkrum commented 7 months ago

Description

Disposal can occur at any time, including during component initialization.

This gives rise to a couple questions that affect how defensive a developer needs to be when implementing initialization methods:

Can Dispose only be called between initialization methods, or concurrently with them?
If Dispose is called during initialization, will the remaining initialization methods be called?

Page URL

https://learn.microsoft.com/en-us/aspnet/core/blazor/components/lifecycle?view=aspnetcore-8.0

Content source URL

https://github.com/dotnet/AspNetCore.Docs/blob/main/aspnetcore/blazor/components/lifecycle.md

Document ID

4871f4f7-9cf6-ac7a-0f21-c65d34bd1119

Article author

@guardrex

github-actions[bot] commented 7 months ago

😎 March! The gateway to spring! 🌞

A green dinosaur 🦖 will be along shortly to assist. Stand-by ........

guardrex commented 7 months ago

I believe the answer to the second question is "no," and I don't understand the first question. It's best if you ask these questions on the product unit's repo. Open an issue to ask there ...

https://github.com/dotnet/aspnetcore/issues

Please add ...

cc: @guardrex https://github.com/dotnet/AspNetCore.Docs/issues/32209

... to the bottom of your opening comment so that I can follow along. I might re-open this for doc work depending on what they say.

kjkrum commented 6 months ago

@guardrex As you may have seen, my attempts to get this clarified were thwarted by ridiculously aggressive issue closure. I can answer the question myself based on the current behavior in ComponentBase.cs. Of course, reading the code is never a substitute for documentation. It remains unclear whether this behavior is intended or formally specified anywhere.

The first time SetParametersAsync is called, it calls RunInitAndSetParametersAsync. This calls OnInitialized, OnInitializedAsync, and CallOnParametersSetAsync, which calls OnParametersSet and OnParametersSetAsync. Unless something throws, all of these methods will be called. Nothing can interleave. So based on the current behavior, I'd say this statement in the docs is misleading:

Disposal can occur at any time, including during component initialization.

Since Dispose is called on the synchronization context, it is currently impossible for Dispose to be called "during" initialization. Therefore, implementations of OnInitialized, OnInitializedAsync, OnParametersSet and OnParametersSetAsync do not need to be defensive about whether the component has been disposed.

guardrex commented 6 months ago

Let's discuss this a bit more. There seems to be two aspects of the issue ...

currently impossible for Dispose to be called "during" initialization.

I didn't think that that's what the remark is addressing, taking into account that remark as a supporting sentence of that particular paragraph.

Generally, this paragraph isn't not about when/how Dispose is triggered. It's focused on what/when those methods would be executed, and I was told earlier (perhaps incorrectly) that they can be called during initialization.

WRT the implementation details of disposal, we don't usually want to get deep into such an advanced subject or framework behaviors that are subject to change without notice. It really is best if the dev working on advanced code study the reference source to see what the framework is doing ... as you did.

I think the problem is that the remark isn't specifically worded to support the subject and focus of the paragraph. The following change came to mind for that sentence ...

IDisposable/IAsyncDisposable can be called at any time, including during component initialization.

... but based on your research and chat with Javier, even that seems wrong (or potentially wrong at least).

Let's ask @MackinnonBuck to put an 👁️ on this to see what he thinks ...

Mackinnon, this is what the paragraph states ...

If a component implements IDisposable, IAsyncDisposable, or both, the framework calls for resource disposal when the component is removed from the UI. Disposal can occur at any time, including during component initialization.

Cross-ref: https://learn.microsoft.com/en-us/aspnet/core/blazor/components/lifecycle?view=aspnetcore-8.0#component-disposal-with-idisposable-and-iasyncdisposable

Is the following replacement for the second sentence better? ...

Don't rely on the exact timing of when IDisposable/IAsyncDisposable are executed when you implement either of these methods.

Should it also extend to the point of even mentioning the timing? ...

Don't rely on the exact timing of when IDisposable/IAsyncDisposable are executed when you implement either of these methods. Disposal code in these methods shouldn't assume that objects created during initialization exist.

kjkrum commented 6 months ago

@guardrex

Generally, this paragraph isn't not about when/how Dispose is triggered. It's focused on what/when those methods would be executed, and I was told earlier (perhaps incorrectly) that they can be called during initialization.

The more I think about it, the more I think it has to be the opposite of that. If disposal is triggered during initialization, it would be wrong for the framework to call Dispose and subsequently call one of the initialization methods. The only correct thing for the component to do at that point would be to throw ObjectDisposedException.

The current implementation of RunInitAndSetParametersAsync is correct and simple. If disposal is triggered during initialization, the remaining initialization methods will still be called. Assuming Dispose is truly called on the sync context, it's impossible for Dispose to be called until after initialization. (And if it isn't, that would open up a whole other can of concurrency worms.) It would also be correct for RunInitAndSetParametersAsync to not call the remaining initialization methods if disposal has been triggered, but that would increase complexity to optimize for an edge case. The current implementation is perfectly pragmatic.

If the current implementation were documented as the intended behavior, initialization methods would never have to worry about whether Dispose has been called. I doubt the developers are intentionally leaving this question open, because again, what could a component realistically do in that scenario other than throw?

A developer should never have to dig into the source to answer questions like this, because the source doesn't differentiate between intentional behavior and implementation details. A good developer always writes to the docs, not the code. But in this case, the docs actually created the question. The statement that disposal can occur "during" initialization was surprising. (And, I'm pretty sure now, incorrect.)

guardrex commented 6 months ago

I understand that, but that doesn't match my recollection of why this is here or what "component initialization" means, which I think it part of the problem with the line.

I think this line just means that the renderer can dispose very early ... so early that disposal methods can be triggered before lifecycle initialization methods would've run.

I'll dig around a bit and get back to you.

guardrex commented 6 months ago

I think I was on the right track about why this was here, but I think I didn't capture the scenario with the language that I used. Check out what led to this line ...

What Javier said is what this should express, including explaining it separately (outside of this paragraph) and focusing more on the behavior the dev was asking about, which pertains to when disposal methods execute relative to when Tasks started during initialization complete.

I'm sorry that this caused so much trouble. I wish I would've looked for the sources first.

The subjects that you've raised, which are a different set of concepts for coverage, really should be assessed by Mackinnon for possible inclusion into the docs.

Mackinnon, I think I can fix that one bad line, but I'll leave this open until you have a chance to look at @kjkrum's remarks to see what else might be covered.

kjkrum commented 6 months ago

My expectation/assumption was similar to the two issues you linked. I find this behavior very surprising. I'm pretty new to Blazor and coming at it with the idea that the component API mimics the familiar behavior of a single-threaded UI framework. I think I need to dive deeper into what a component really is and adjust my mental models. I might also need to review async/await. I was thinking that Dispose being called in the same sync context as the init methods was equivalent to Dispose being called in the same asynchronous control flow as the init methods, which would make the behavior described in dotnet/aspnetcore#22932 impossible.

kjkrum commented 6 months ago

Well, hang on. Not all sync contexts are logically single-threaded, but Blazor's is. So the Task A/Dispose/Task B call sequence described in dotnet/aspnetcore#22932 really should be impossible.

guardrex commented 6 months ago

I don't think the .NET task scheduler works like that, but that definitely isn't in my wheelhouse. We have similar guidance in I think at least one other spot along the lines of don't count on the order of Task completion around here ... it's arbitrary.

In this case, the idea was to tell readers that the dispose methods might execute at any given time and not to count on it happening in relation to any lifecycle method. A few words might also mention that disposal code has to check on an object before disposing of it, which is common anyway, because there's no guarantee that it was created in the first place in init or other lifecycle methods.

I have a good idea of how I'd like to address this. It will just take a couple of days (or three weeks! 🏃‍♂️😅) to reach this after resolving a couple of other higher priority items.

kjkrum commented 6 months ago

I found similar developer confusion in dotnet/aspnetcore#22695. This comment is the heart of the matter. For anyone familiar with other single-threaded UI frameworks, this is a huge violation of the Principle of Least Astonishment.

kjkrum commented 6 months ago

I think I've pieced together how it all works. I would not have had this question if I'd read something like this all in one place. I'm not sure where that place should be. Maybe here.

A single logical thread of execution does not imply a single asynchronous control flow. A component is reentrant at any point where it awaits an incomplete task. Other lifecycle methods or Dispose may be called before the asynchronous control flow resumes after the await. Therefore, a component must ensure that it is in a valid state before awaiting any potentially incomplete task. In particular, a component must ensure that it is in a valid state for rendering when OnInitializedAsync and OnParametersSetAsync return. If these methods return incomplete tasks, they must ensure that the part of the method that completes synchronously leaves the component in a valid state for rendering. Another implication of components being reentrant is that a method cannot defer a task until after the method returns by passing it to Invoke. Calling Invoke may only defer the task until the next await.

It is strongly recommended that components implementing IDisposable or IAsyncDisposable call async methods using a CancellationToken from a CancellationTokenSource that is canceled when the component is disposed. A disposable component should check whether it has been disposed after awaiting any task that does not receive the component's CancellationToken. Incomplete tasks may also prevent a disposed component from being garbage collected. ComponentBase eats exceptions caused by task cancellation (more precisely, it eats all exceptions if the task it's awaiting is canceled) so component methods don't need to handle TaskCanceledException and OperationCanceledException.

Note that ComponentBase cannot follow these guidelines because it has no concept of what constitutes a valid state for a derived component and it does not itself implement IDisposable or IAsyncDisposable. If OnInitializedAsync returns an incomplete task that does not use a CancellationToken and the component is disposed before the task completes, ComponentBase will still call OnParametersSet and await OnParametersSetAsync. Therefore, if a disposable component does not use a CancellationToken, OnParametersSet and OnParametersSetAsync should check if the component is disposed.

guardrex commented 6 months ago

I'll analyze that tomorrow and see what could be floated to the product unit for coverage. It would be nice to NOT surprise folks on disposal behavior ... I agree.

If I don't get to this tomorrow (Friday), I'll certainly reach it next week.

guardrex commented 3 months ago

UPDATE (7/30): This is still on hold for further possible article updates due to high priority work by the product unit for 9.0. They won't be free to look at this for a while longer. The release is in November, and they're under-the-gun to wrap up a lot of work. In the meantime, the updates that I made earlier on #32457 cover Javier's remarks.

I'm going to adjust the priority on this down until it's reached, and I will definitely try to get it resolved by EOY.

dotnet / AspNetCore.Docs