what's the problem in idisposable pattern implementation withing abstract class

Question

I have following implementation for one of an abstract class but getting warning from sonarqube code analysis

public abstract class BackgroundService : IHostedService, IDisposable
{
    private bool _disposedValue;//https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose
    private Task _executingTask;
    private readonly CancellationTokenSource _stoppingCts = new CancellationTokenSource();

    protected abstract Task ExecuteAsync(CancellationToken stoppingToken);

    public virtual Task StartAsync(CancellationToken cancellationToken)
    {
        _executingTask = ExecuteAsync(_stoppingCts.Token);

        if (_executingTask.IsCompleted)
        {
            return _executingTask;
        }

        return Task.CompletedTask;
    }

    public virtual async Task StopAsync(CancellationToken cancellationToken)
    {
        if (_executingTask == null)
        {
            return;
        }

        try
        {
            _stoppingCts.Cancel();
        }
        finally
        {
            await Task.WhenAny(_executingTask, Task.Delay(Timeout.Infinite, cancellationToken));
        }

    }

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

    protected void Dispose(bool disposing)//todo kamran
    {
        if (_disposedValue)
        {
            return;
        }

        if (disposing)
        {
            _stoppingCts.Cancel();
        }            

        _disposedValue = true;
    }

    ~BackgroundService()
    {
        Dispose(false);
    }
}

sonar cube error is

Fix this implementation of 'IDisposable' to conform to the dispose pattern.
Code Smell
Provide 'protected' overridable implementation of 'Dispose(bool)' on 'BackgroundService' or mark the type as 'sealed'.
'BackgroundService.Dispose()' should not be 'virtual' or 'abstract'.
'BackgroundService.Dispose()' should also call 'Dispose(true)'.

It quite literally says it: `Dispose()` should not be virtual ... — Fildor, Feb 15 '22 at 08:32
As an aside, if you're writing a class intended for wide use and parallelism is involved somehow, it pays off to make the implementation thread-safe to ensure disposing doesn't happen more than once, because it's very hard for callers to ensure this and far easier if parallel calls to `Dispose` are safe. This is as simple as making the check use `Interlocked.CompareExchange` at the start of your method to do an atomic test-and-set, rather than a simple field assignment. — Jeroen Mostert, Feb 15 '22 at 08:41
For more discussion on why `Dispose()` should *not* be virtual: https://stackoverflow.com/questions/3619490/why-should-dispose-be-non-virtual#:~:text=The%20Dispose%20method%20should%20not,(%20Dispose(bool)%20). — Matthew Watson, Feb 15 '22 at 08:54
@JeroenMostert off-topic to this question, but I always use `Interlocked.Exchange(ref x, 1) == 0`, instead of `CompareExchange`: are there any downsides to making the equality outside and not using `CompareExchange`? — Jcl, Feb 15 '22 at 08:55
@Jcl: not that I can think of -- I'm just used to using `CompareExchange` generally because it's more conservative (won't write anything if the expected state isn't encountered) and useful in more scenarios. For this particular one `Exchange` should do just as well since the ultimate value is stable. — Jeroen Mostert, Feb 15 '22 at 09:05
@JeroenMostert thanks, [this](https://gist.github.com/javiercampos/6e611e10e648c531a553c4494ecd6cb3) is my `Guard` class, and I've been using it forever, and just made me doubt I could have used it wrong all this time :-) — Jcl, Feb 15 '22 at 09:10
@Jcl: well, that class in particular does have a potential problem when you start overlapping calls to `Reset` and `Once`, of course -- you would need additional synchronization if you wanted to be sure of the order of operations (or else be sure that, for example, `Reset` is never called at any point where other threads could be doing `Once`). Add a little more of that and you might as well use a full-blown `Monitor`, which is more flexible. I'm not sure I'd call this class worthy of what it's trying to abstract myself, but that's a personal preference. — Jeroen Mostert, Feb 15 '22 at 09:17
@JeroenMostert yep, I understand the `Reset` problem, and I only use it on very uncommon ocassions (it's a recent addition and only added it for Blazor Server, where the framework takes some lifetime decisions away from me), and properly synced. I see your point there. Anyway, this is off-topic here, thanks for your answer! :-) — Jcl, Feb 15 '22 at 09:20

score 3 · Accepted Answer · answered Feb 15 '22 at 08:45

3

Your Dispose() method should not be virtual: your Dispose(bool) one should be (and it's the one to be overriden by inheritors): it's a common and known pattern where you use the bool parameter to know if you are disposing deterministically (by a call to Dispose or a using block) or via a finalizer (which you haven't implemented in your abstract class, by the way), where you only need to dispose unmanaged references.

You are lacking the finalizer disposing to make the pattern complete:

~BackgroundService() {
   Dispose(false);
}

Otherwise, the GC.SuppressFinalize(this) makes no sense

answered Feb 15 '22 at 08:45

Jcl

27,696
5
61
92

thanks jcl. i have updated the implementation but still getting same sonarqube warning – Kamran Shahid Feb 15 '22 at 09:32
@KamranShahid can we see the updated implementation? because if *the same* warning is still there after making `Dispose()` non-virtual, and `Dispose(bool)` virtual, then sonarqube is wrong – Jcl Feb 15 '22 at 09:55
I have updated it in the question. i hope i have correctly implemented as per your suggestion. still getting same warnings – Kamran Shahid Feb 15 '22 at 10:09
Nope, your `Dispose(bool)` needs to be `virtual` (`protected virtual void Dispose(bool disposing)`), as stated in the answer – Jcl Feb 15 '22 at 10:22

what's the problem in idisposable pattern implementation withing abstract class

1 Answers1