Fix this implementation of 'IDisposable' to conform to the dispose pattern

Question

I have this working code here which is to download a file with progress. When I run SonarScanner, it complaints that I should consider to

Fix this implementation of 'IDisposable' to conform to the dispose pattern.

public class HttpClientDownloadWithProgress : IDisposable
{
    private readonly string _downloadUrl;
    private readonly string _destinationFilePath;
    private readonly string _username;
    private readonly string _password;
    private HttpClient _httpClient;
    
    public delegate Task ProgressChangedHandler(long? totalFileSize, long totalBytesDownloaded, double? progressPercentage, string fileName);

    public event ProgressChangedHandler ProgressChanged;

    public HttpClientDownloadWithProgress(string downloadUrl, string destinationFilePath, string username, string password)
    {
        _downloadUrl = downloadUrl;
        _destinationFilePath = destinationFilePath;
        _username = username;
        _password = password;
    }

    public async Task StartDownload()
    {            
        _httpClient = new HttpClient { Timeout = TimeSpan.FromDays(1) };

        var byteArray = Encoding.ASCII.GetBytes($"{_username}:{_password}");
        _httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", Convert.ToBase64String(byteArray));

        using (var response = await _httpClient.GetAsync(_downloadUrl, HttpCompletionOption.ResponseHeadersRead)) {
            await DownloadFileFromHttpResponseMessage(response);
        }
            
    }

    private async Task DownloadFileFromHttpResponseMessage(HttpResponseMessage response)
    {
        response.EnsureSuccessStatusCode();

        var totalBytes = response.Content.Headers.ContentLength;

        using (var contentStream = await response.Content.ReadAsStreamAsync()) {
            await ProcessContentStream(totalBytes, contentStream);
        }
        
    }

    private async Task ProcessContentStream(long? totalDownloadSize, Stream contentStream)
    {
        var totalBytesRead = 0L;
        var readCount = 0L;
        var buffer = new byte[8192];
        var isMoreToRead = true;

        using (var fileStream = new FileStream(_destinationFilePath, FileMode.Create, FileAccess.Write, FileShare.None, 8192, true))
        {
            do
            {
                var fileName = Path.GetFileName(_destinationFilePath);

                var bytesRead = await contentStream.ReadAsync(buffer, 0, buffer.Length);
                if (bytesRead == 0)
                {
                    isMoreToRead = false;
                    await TriggerProgressChanged(totalDownloadSize, totalBytesRead, fileName);
                    continue;
                }

                await fileStream.WriteAsync(buffer, 0, bytesRead);

                totalBytesRead += bytesRead;
                readCount += 1;

                if (readCount % 100 == 0) {
                    // change status to "DOWNLOADED"
                    await TriggerProgressChanged(totalDownloadSize, totalBytesRead, fileName);
                }
                // change status to "DOWNLOADING"
            }
            while (isMoreToRead);
        }
    }

    private async Task TriggerProgressChanged(long? totalDownloadSize, long totalBytesRead, string fileName)
    {
        if (ProgressChanged == null)
            return;

        double? progressPercentage = null;
        if (totalDownloadSize.HasValue)
            progressPercentage = Math.Round((double)totalBytesRead / totalDownloadSize.Value * 100, 2);

        await ProgressChanged(totalDownloadSize, totalBytesRead, progressPercentage, fileName);
    }

    public void Dispose()
    {
        _httpClient?.Dispose();
    }
}

What's wrong with my Dispose() method? How can I implement the dispose pattern?

Answer 1

Some older linters like the pattern to be:

1. Implement IDisposable and forward the call to Dispose(true)
2. Implement a finalizer that forwards to Dispose(false)
3. Add a virtual protected void Dispose(bool disposing) method doing the actual disposal of resources based on whether the call originates from the finalizer or the destructor.

This ensures that the GC finalizer thread gets a chance to clear of unmanaged resources if any.

Something along these lines

protected void Dispose(bool disposing)
{
    if(disposing){
       _httpClient?.Dispose();
       GC.SuppressFinalize(this); // no need to run the finalizer we have dispose everything we need
    }
   
}

public void Dispose()=>Disposing(true);

public ~HttpClientDownloadWithProgress()=>Disposing(false);
}

But in your case it's doubtful that inheritance would be the way forward. I'd rather compose an HttpClient and a Progress and keep the client alive longer so other composites may use it.