Skip to content

Fix illogical caution: Dispose should say DisposeAsync #45588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

jzabroski
Copy link
Contributor

@jzabroski jzabroski commented Apr 1, 2025

Summary

The warning says:

If you implement the IAsyncDisposable interface but not the IDisposable interface, your app can potentially leak resources. If a class implements IAsyncDisposable, but not IDisposable, and a consumer only calls Dispose, your implementation would never call DisposeAsync. This would result in a resource leak.

The warning is illogical. If you do not implement IDisposable, there is no Dispose method to call. IAsyncDisposable only supports DisposeAsync.


Internal previews

📄 File 🔗 Preview link
docs/standard/garbage-collection/implementing-disposeasync.md docs/standard/garbage-collection/implementing-disposeasync

@jzabroski jzabroski requested review from gewarren and a team as code owners April 1, 2025 13:00
@dotnetrepoman dotnetrepoman bot added this to the April 2025 milestone Apr 1, 2025
@dotnet-policy-service dotnet-policy-service bot added dotnet-fundamentals/svc community-contribution Indicates PR is created by someone from the .NET community. labels Apr 1, 2025
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll comment and leave the final wording to @gewarren

I agree that we should change this cautionary warning. However, this fix is misleading. The purpose of the warning is that a type that implements IAsyncDisposable but not IDisposable can't be used and disposed synchronously. If a consumer chooses to use the synchronous APIs, there won't be a Dispose method. Therefore, consumers using the synchronous APIs are likely to leak resources.

@jzabroski
Copy link
Contributor Author

I see. I think your point is that

  1. in open systems that treat services as plug-ins, the consumer is actually the framework that consumes the plug-in. If the framework (consumer) does not know about IAsyncDisposable, then it may never be called.
  2. if you implement both IDisposable and IAsyncDisposable but the consumer explicitly calls Dispose(), DisposeAsync() may never be called, which may lead to leaking resources.
  3. if the package implements IAsyncDisposable through Microsoft.Bcl.AsyncInterfaces but the consuming package is unaware of async dispose pattern (very old C# compiler), then it may never be called. (This is hopefully never the case, but possible as its ultimately a polyfill that requires the C# compiler to reify into a using disposal pattern.)

There may be other patterns. Not enough coffee yet in the morning.

@BillWagner
Copy link
Member

Hi @jzabroski

The use case for both IAsyncDisposable and IDisposable is quite a bit simpler: Consumers of your type can probably use it either synchronously or asynchronously. For example, consider the File I/O APIs in the .NET runtime: A consumer could call ReadLine() or ReadLineAsync(). Presumably, if a consumer chose synchronous operation by calling ReadLine(), they'd also chose a synchronous Dispose() instead of an asynchronous DisposeAsync().

If you only provide IAsyncDisposable, you don't provide proper synchronous use and dispose.

@gewarren
Copy link
Contributor

I'm going to close this since the original wording is correct.

@gewarren gewarren closed this Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates PR is created by someone from the .NET community. dotnet-fundamentals/svc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants