CA2213 Should Not Give False Positives for classes implementing IAsyncDisposable

[TheCakeMonster]

Analyzer

Diagnostic ID: CA2213

Describe the improvement

Classes that implement IAsyncDisposable along with the async dispose pattern can suffer from false positives of CA2213 when all analyzers are enabled, because the disposal of items in a virtual DisposeAsync method is not recognized.

Example Code

Here is a class that demonstrates the issue on my machine. Create a .NET 6 Console application and add this class. The false positive is from the CancellationTokenSource, which the analyzer thinks is not being disposed.

VS 2022 17.6.4
SDK 7.0.304

namespace AsyncDisposableAnalyzerError;

internal class AsyncDisposableClass : IAsyncDisposable
{
    private bool _disposed;
    private readonly Task _backgroundTask;
    private readonly CancellationTokenSource _backgroundTaskCTS = new();

    public AsyncDisposableClass()
    {
        _backgroundTask = DoBackgroundWorkAsync(_backgroundTaskCTS.Token);
    }

    private static async Task DoBackgroundWorkAsync(CancellationToken cancellationToken)
    {
        try
        {
            await Task.Delay(250000, cancellationToken).ConfigureAwait(false);
        }
        catch (TaskCanceledException)
        {
            // This is normal behaviour; we don't mind this
        }
    }

    /// <summary>
    /// Async disposal implementation
    /// </summary>
    /// <param name="disposing">Whether disposal is being requested from a consumer</param>
    protected virtual async Task DisposeAsync(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                // End the background expiry task and then dispose of the token source 
                _backgroundTaskCTS.Cancel();
                await _backgroundTask.ConfigureAwait(false);
                _backgroundTaskCTS.Dispose();
            }

            _disposed = true;
        }
    }

    /// <summary>
    /// Public entrypoint into the async disposal mechanism
    /// </summary>
    public async ValueTask DisposeAsync()
    {
        // Do not change this code. Put cleanup code in 'DisposeAsync(bool disposing)' method
        await DisposeAsync(disposing: true).ConfigureAwait(false);
        GC.SuppressFinalize(this);
    }
}

The following are the settings from the csproj that enable the analyzer and result in these false positives:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
    <EnableNETAnalyzers>true</EnableNETAnalyzers>
    <AnalysisMode>All</AnalysisMode>
  </PropertyGroup>
</Project>

Additional context

As you can see from the csproj, we use an analysis mode that increases the number of analyzers that run. This issue does not show if you do not specify that a higher level of analysis should be performed. When validating and working on this issue, be sure to specify these csproj settings.

[mavasani]

@TheCakeMonster This issue repros only if your protected virtual async Task DisposeAsync(bool disposing) is declared as a virtual method. Note that the core public async ValueTask DisposeAsync() is calling into this helper virtual method, and we perform interprocedural analysis for non-virtual method calls to detect the dispose invocations. If you are certain that all overrides of the helper method in subtypes of AsyncDisposableClass will always invoke base.DisposeAsync(disposing) in their implementations, you can suppress this CA2213 violation.

[TheCakeMonster]

@mavasani Thanks for checking, and for the additional information.

I think the format/structure of this code section was generated for me by Visual Studio; I don't think I would have made the method virtual by myself. In other words, I think there is an analyzer fix associated with the IAsyncDisposable interface (implement interface?) that generates the code that causes the false positive. It feels like there is a disconnect between the fix and the analyzer checking its results if VS generates code that the analyzer is unable to successfully check.

It is possible that I copied the code from the fix for the sync method and then modified it I suppose, but I don't think so. The point is that I thought I had used the recommended "implement using the Dispose pattern" approach in this code, so it seems a little odd that it results in a build warning.

[JorisCleVR]

When implementing according to the guidelines of dotnet as described in https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync I am also running into this false positive.

The following class structure is proposed on the page for a non sealed class:

class ExampleConjunctiveDisposableusing : IDisposable, IAsyncDisposable
{
    IDisposable? _disposableResource = new MemoryStream();
    IAsyncDisposable? _asyncDisposableResource = new MemoryStream();

    public void Dispose()
    {
        Dispose(disposing: true);
        GC.SuppressFinalize(this);
    }

    public async ValueTask DisposeAsync()
    {
        await DisposeAsyncCore().ConfigureAwait(false);

        Dispose(disposing: false);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            _disposableResource?.Dispose();
            _disposableResource = null;

            if (_asyncDisposableResource is IDisposable disposable)
            {
                disposable.Dispose();
                _asyncDisposableResource = null;
            }
        }
    }

    protected virtual async ValueTask DisposeAsyncCore()
    {
        if (_asyncDisposableResource is not null)
        {
            await _asyncDisposableResource.DisposeAsync().ConfigureAwait(false);
        }

        if (_disposableResource is IAsyncDisposable disposable)
        {
            await disposable.DisposeAsync().ConfigureAwait(false);
        }
        else
        {
            _disposableResource?.Dispose();
        }

        _asyncDisposableResource = null;
        _disposableResource = null;
    }

Because it is a non sealed class the DisposeAsyncCore method must be virtual. Furthermore the virtual DisposeAsyncCore method is always called by the public DisposeAsync in this pattern. Therefore all Child classes will call the virtual method when DisposeAsync is called.
Since this is the dotnet proposed implementation I rather not have my team manually suppress the CA2213 violation in this case.

[JorisCleVR]

After some further testing it seems to only go wrong in case of implementing IAsyncDisposable without implement IDisposable.
Therefore the code in my earlier comment will not result in a CA2213.
The following code example does, because it only implements IAsyncDisposable:

    public class ExampleConjunctiveDisposableusing : IAsyncDisposable
    {
        private IAsyncDisposable asyncDisposableResource = new MemoryStream();

        public async ValueTask DisposeAsync()
        {
            await this.DisposeAsyncCore().ConfigureAwait(false);

            GC.SuppressFinalize(this);
        }

        protected virtual async ValueTask DisposeAsyncCore()
        {
            if (this.asyncDisposableResource != null)
            {
                await this.asyncDisposableResource.DisposeAsync().ConfigureAwait(false);
            }

            this.asyncDisposableResource = null;
        }
    }

Since https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync describes that a class in specific case may implement only IAsyncDisposable (without implementing IDisposable). This is still a false positive of the analyzer.

[MartyIX]

@JorisCleVR Is your sample fixed by #6976 or not? Or is it somehow different?

[JorisCleVR]

@MartyIX As far as I can see I think it should indeed also solve this issue. That would be great! Thank you for fixing it.

[sharwell]

Task DisposeAsync(bool disposing)

Note that the DisposeAsync method should not have a disposing parameter. This method will never be called from a finalizer.