Low-erratic quality of examples of pattern

[deep-outcome]

1. First example – Implement the async dispose pattern – has acceptable quality since it clearly counts on null state of _jsonWriter as indicator of disposement and it is obviously made up as simplified.
2. Second example – Implement both dispose and async dispose patterns:
   • States “Doing so ensures that you can properly cascade clean up calls.”. That is not shown in example at all.
   • Also tries to pass around its disposal logic around with null state checks on class variables but it this slightly more complicated – field initializers do not allow to any of them to be null when either of disposal procedures will take its turn. So checking null state of both of them in Dispose(bool) and DisposeAsyncCore is slight overdo,
   • The conditional logic there is not fine. Why to use ?. operator and the set variable to null in each case? Nonetheless both methods should rely on some _alreadyDisposed bool field. Not combinative null checks. Bad conditional logic is also exercised in first example.

Note that if there can happen situation when both Dispose(bool) and DisposeAsyncCore are called (as examples suggest) state of variables should be synchronized (thread safe) as it likely can happen on different threads.

This report works fine if changes in PR #26269 that address some direct failures are confirmed and merged.

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: 61a961dc-3ebf-2a96-a6eb-2e0b62ab909f
• Version Independent ID: d72276e1-9059-815f-dc14-a03a7544ab87
• Content: Implement a DisposeAsync method
• Content Source: docs/standard/garbage-collection/implementing-disposeasync.md
• Product: dotnet-fundamentals
• GitHub Login: @IEvangelist
• Microsoft Alias: dapine

[IEvangelist]

Hi 👋 @Uzivatel919

Thank you so much for posting this issue. We really appreciate it.

Discuss this issue

List item number 1, you mentioned is acceptable quality and clear. Let's move on to list item number 2 - which contains three bullets.

Bullet number 1:

States “Doing so ensures that you can properly cascade clean up calls.”. That is not shown in example at all.

When a class owns a field that either implements IDisposable or IAsyncDisposable, and it itself then implements the corresponding interface simply to cascade calls to either Dispose or DisposeAsync - that is what is shown. That is the example.

Bullet number 2:

Also tries to pass around its disposal logic around with null state checks on class variables but it this slightly more complicated – field initializers do not allow to any of them to be null when either of disposal procedures will take its turn. So checking null state of both of them in Dispose(bool) and DisposeAsyncCore is slight overdo,

Yes, the field initializers do not allow for them to be null, but they're not readonly. Something could set them to null, as such it is best to be defensive. Also, thinking about idempotency. If you're to call Dispose or DisposeAsync more than once and it was set to null the first time, this would then throw NullReferenceException.

Bullet number 3:

The conditional logic there is not fine. Why to use ?. operator and the set variable to null in each case? Nonetheless both methods should rely on some _alreadyDisposed bool field. Not combinative null checks. Bad conditional logic is also exercised in first example.

There is no _alreadyDisposed field in this example. What is bad about this conditional logic?

Future collaborations

I think we might find more success in our collaborations if you're to first create an issue and then give us a chance to reply to the issue before creating a pull request. We could have discussed the concerns here first to determine if they're valid concerns or not. And then used that as the basis whether or not a pull request was even necessary - and the extent of the changes being proposed. Is that fair?

[deep-outcome]

When a class owns a field that either implements IDisposable or IAsyncDisposable, and it itself then implements the corresponding interface simply to cascade calls to either Dispose or DisposeAsync - that is what is shown. That is the example.

Good then. I understood it as cascading calls in hierarchy of base-derived classes. This is what I actually miss on this page.

Yes, the field initializers do not allow for them to be null, but they're not readonly. Something could set them to null, as such it is best to be defensive. Also, thinking about idempotency. If you're to call Dispose or DisposeAsync more than once and it was set to null the first time, this would then throw NullReferenceException.

This I know. Nobody needs to rely on readonly modifier. It is the code flow what designates state of variables. But I take it since I did not consider this before. Nonetheless I meant check on one of them for null state is more proper code. Let squash rest into last quote.

There is no _alreadyDisposed field in this example. What is bad about this conditional logic?

Since you stated they can be null due some hidden logic, I take my objections back leaving it without detailed explanation. Let see only succinct one.

FMPV both methods should rely on definitely obvious non-null state of fields and use SSOT to indicate disposal state.

protected virtual void Dispose(bool disposing)
{
	// lock(_syncObject)
	//	{
	if(_alreadyDisposed)
		return;
	//      }	

	…	
}

protected virtual async ValueTask DisposeAsyncCore()
{
	// lock(_syncObject)
	//	{
	if(_alreadyDisposed)
		return;
	//      }	
	…    
}

In existing example designs null checks in disposal methods can be understand as representation of composed disposal state. In fact I understood it like this before you told me about hidden meaning encrypted in missing readonly modifiers.

Then code can be straightforward as much as possible w/o any redundant null checks or settings of field that is (can be) null to null again.

---

I think we might find more success in our collaborations if you're to first create an issue and then give us a chance to reply to the issue before creating a pull request. We could have discussed the concerns here first to determine if they're valid concerns or not. And then used that as the basis whether or not a pull request was even necessary - and the extent of the changes being proposed. Is that fair?

In fact I was doing it like this and then I was familiarized on that community is greatly welcomed to help in improvements and fixing the docs so I do it now like this. Specifically if I am sure, I know what I am doing and it is not much work, since I expect major reworks are property of regular employees.

See for instance changes in #26192 that was approved w/o preceding issue.

I am about to continue in my style of corrections so if you have any objections please tell them now or later but let be please accurate about what exactly is wrong, what should be better, avoided, improved, changed, made more friendly etc.

[IEvangelist]

Hi @Uzivatel919 -

I am about to continue in my style of corrections so if you have any objections please tell them now or later but let be please accurate about what exactly is wrong, what should be better, avoided, improved, changed, made more friendly etc.

Please post an issue first and allow time for someone on the team to discuss it before pushing a pull request. I've been heads down and missed #26192, but it had some issues that needed to be corrected in #26302. I'm happy to collaborate with you and help you continue to contribute, but we should strive to minimize unnecessary churn. Does that make sense? Thanks again

[deep-outcome]

Hmmmm… You are greatly are welcomed. I went through history and found exactly

We'd appreciate any PRs that make updates to these terms.

See for instance #26064 (comment).

For some reason in my memory it was fused/changed to something like “We'd appreciate any PRs that make updates.”. Now I understand the controversy.

Please I have already some other PRs. These can be converted to issues or just checked.

• dotnet/dotnet-api-docs#7209
• dotnet/dotnet-api-docs#7234
• dotnet/dotnet-api-docs#7236
• dotnet/dotnet-api-docs#7231

[IEvangelist]

Closed as part of #26269