IDisposable questions about specific case

Question

Question #1:

StartAsync handles the disposal of _clientWebSocket and _tokenSource. So do I really need to dispose these in Dispose() as well? I think I should keep _semaphore.Dispose() only in the Dispose(), because my code already handles the rest.

Question #2:

What if the user forgets to call Dispose() or wrap it in using? It's usually solved by calling Dispose() in the deconstructor/finalizer but in this case my class is sealed.

public sealed class Client : IDisposable
{
    private readonly SemaphoreSlim _semaphore = new(1, 1);

    private ClientWebSocket? _clientWebSocket;
    private CancellationTokenSource? _tokenSource;

    public void Dispose()
    {
        _semaphore.Dispose();

        // TODO: Do I need to dispose these since my code below does that? 
        _clientWebSocket?.Dispose();
        _clientWebSocket = null;

        _tokenSource?.Cancel();
        _tokenSource = null;
    }

    public Task StartAsync()
    {
        _clientWebSocket = new ClientWebSocket();
        _tokenSource = new CancellationTokenSource();

        try
        {
            ...
        }
        catch (Exception ex)
        {
        }
        finally
        {
            _clientWebSocket?.Dispose();
            _clientWebSocket = null;

            _tokenSource?.Cancel();
            _tokenSource = null;
        }
    }

    public ValueTask SendAsync()
    {
        if (_clientWebSocket is { State: WebSocketState.Open })
        {
            return;
        }

        ...
    }
}

You don't need a finalizer, because the semaphore, websocket, and cancelationtokens already have their own finalizers and will cover you that way, but you should still dispose them in your own Dispose() method. — Joel Coehoorn, Apr 28 '22 at 20:22
But not deterministically. That's especially important for limited OS handles like the web socket. — Blindy, Apr 28 '22 at 20:24
@JoelCoehoorn, does that mean I could remove the IDisposable inheritance, since it will dispose them anyway? — nop, Apr 28 '22 at 20:26
No. You should move websocket and cancelation token to be local to the method and dispose them there, as suggested in the answer, but the semephore could still be a class member, and that means you want to implement IDisposable. You just don't need to worry about the finalizer part of the pattern. — Joel Coehoorn, Apr 29 '22 at 14:11

score 4 · Accepted Answer · answered Apr 28 '22 at 20:28

4

StartAsync handles the disposal of _clientWebSocket and _tokenSource. So do I really need to dispose these in Dispose() as well? I think I should keep _semaphore.Dispose() only in the Dispose(), because my code already handles the disposal of the rest.

Then why are they fields? It sounds like they should be local variables, in which case they wouldn't be part of your class' dispose method either way.

Also forget the pattern you're using and use using instead.

What if the user forgets to call Dispose() or wrap it in using?

It's their choice, but there is a Roslyn analyzer warning for that: CA2000

It's usually solved by calling Dispose() in the deconstructor/finalizer

Less usually, more always, but you are correct. You want your finalizer to clean up after your object if the caller doesn't, with the obvious caveat of threading (what thread will the finalizer run on? The answer to that question prevents you from disposing of OpenGL handles in finalizers for example).

in this case my class is sealed

That has nothing to do with your question, you can implement finalizers in sealed classes.

answered Apr 28 '22 at 20:28

Blindy

65,249
10
91
131

Thanks for your answer! They are fields and not local variables because there is a SendAsync method relying on `_clientWebSocket` and `StopAsync` relying on `_tokenSource.`. I didn't add these methods to the question because I wanted to keep it simple, but it seems like I should have. – nop Apr 28 '22 at 20:32
But `StartAsync` disposes of those objects, how are the other functions using them? – Blindy Apr 28 '22 at 20:35
By having `if (_clientWebSocket is { State: WebSocketState.Open })` checks. – nop Apr 28 '22 at 20:36
That's not using them, it's a false condition. I'm asking, if your `StartAsync` call disposes of your socket when it ends, how are the other functions using that socket? You are presumably awaiting those calls, so it's not like you can call `SendAsync` before `StartAsync` returns. – Blindy Apr 28 '22 at 20:38
Fair point, i'm using `Task.Run(...)` to prevent the blocking in StartAsync but that's outside the class – nop Apr 28 '22 at 20:42
1

Only thing I'll add regarding finalizers - the general rule of thumb is don't use them for purely managed code. They really exist for cleaning up unmanaged objects. Finalizers impact performance and lifetime of an object and are non-deterministic, so only implement them when necessary otherwise they do more harm than good. – Zer0 Apr 28 '22 at 20:50
@Zer0, that's a great comment – nop Apr 28 '22 at 20:56
It is a good point, the problem is that it's difficult to determine what's a purely managed class. Are socket classes? No, they use OS socket handles. Are cancellation token sources? No, they use OS event handles. Are windows? No in WinForms, yes in WPF. A much better rule of thumb, and what CA2000 tracks, is that if you have any disposable field in your class, your class should implement `IDisposable` itself and dispose of those objects. – Blindy Apr 29 '22 at 14:39
In fact, let me blow your mind for a second: tasks also wrap around OS wait event handles, so by all rights they should be disposed. What about `ValueTask` you ask? Well, it wraps a `Task`, so that also needs to be disposed. In .Net6 and 7 there have been a lot of effort spent to ensure those handles aren't constructed most of the time, but that only makes it more difficult to determine when to dispose or not. – Blindy Apr 29 '22 at 14:44

IDisposable questions about specific case

Question #1:

Question #2:

1 Answers1