0

Look at this particular line of code: using var ws = new ClientWebSocket() in the class below. When is that object disposed taking into account that it uses using and it is being used in a few methods more like SendAsync, ReceiveAsync, etc.? If it was in a regular method like Main(), it usually gets disposed at the closing branch, but in this case it's not disposed there. This class also doesn't inherit IDisposable. I removed the unnecessary details from the class.

public class Client
{
    private ClientWebSocket? _ws;

    private async Task<bool> ConnectAsync(CancellationToken cancellationToken)
    {
        using var ws = new ClientWebSocket();
        _ws = ws;

        ...
    }

    public async Task SendAsync(string data)
    {
        ...

        await _ws.SendAsync(dataBytes, WebSocketMessageType.Text, true, CancellationToken.None);
    }
}
nop
  • 4,711
  • 6
  • 32
  • 93
  • 2
    `using var` is just syntactic shorthand for an explicit block that lasts until the end of the method -- `Main` is a method like any other, it just happens to also (usually) be the end of the execution. `ws` will be disposed at the end of `ConnectAsync`, and hence this code is not correct -- such an object should not be stored in a field. If it is, it should be disposed as part of making `Client` disposable as well. – Jeroen Mostert Mar 12 '22 at 13:40
  • Is `await _clientWebSocket.SendAsync(...` a typo of `await _ws.SendAsync(...`? – Caius Jard Mar 12 '22 at 13:43
  • 1
    Looks like an ObjectDisposedException waiting to happen.. – Caius Jard Mar 12 '22 at 13:44
  • @CaiusJard, oh, it was typo. – nop Mar 12 '22 at 13:44
  • @JeroenMostert, many people were using it that way, so I wonder how that works. It actually works just fine btw. – nop Mar 12 '22 at 13:45
  • 1
    The only way it works "just fine" is if all the actions that happen on the created object happen within the context of `ConnectAsync`, that is, if it's OK that it gets disposed at the end, but if that's the case it may as well be passed as a parameter to enforce that it won't be used outside that context. The other way it would work fine is if the object secretly doesn't care that it's disposed because it just implements the interface as part of its contract (e.g. `MemoryStream`). – Jeroen Mostert Mar 12 '22 at 14:03
  • @JeroenMostert, you are right about that, because `ConnectAsync` ends up with `ws.CloseAsync`. There is a send message loop above the ConnectAsync, which involves the ClientWebSocket object. – nop Mar 12 '22 at 14:13

1 Answers1

1

Your using statement is C#'s new declaration style of using introduced in C# version 8, meaning you no longer have unnecessary nesting for your statements. However the principals are still the same. Consider the following way of writing your code prior to C# 8:

public class Client
{
    private ClientWebSocket? _ws;

    private async Task<bool> ConnectAsync(CancellationToken cancellationToken, string data)
    {
        using (var ws = new ClientWebSocket())
        {
            // When your code leaves this block, your websocket is disposed
            ws.SendAsync(dataBytes, WebSocketMessageType.Text, true, CancellationToken.None);
            
            // Assigning ws to _ws isn't necessary as it isn't usable outside your using statement
            _ws = ws;
            
            // The end of your using block - the websocket will now be disposed
        }
        
        // This line would fail as your websocket has been disposed
        _ws.SendAsync(dataBytes, WebSocketMessageType.Text, true, CancellationToken.None);
    }
}

The dispose method gets called when your code leaves the using block. A declaration means that the scope after which it will be disposed of is now the block of code that contains the using declaration, in this instance, your ConnectAsync method. So even though you still have a reference to it when it's assigned to _ws, it can't be used as it's already been disposed when your code left this method block.

Misanthropist
  • 169
  • 1
  • 7
  • Thanks, you are right. It actually never reaches the end of ConnectAsync, because of the loops. – nop Mar 12 '22 at 14:37