A race condition occurs when two threads access a shared variable at the same time. The first thread reads the variable, and the second thread reads the same value from the variable.

I used SemaphoreSlim in StartAsync to prevent a race condition, i.e. two threads may change _clientWebSocket concurrently. It may happen if we're calling StartAsync in a Timer.

What about StopAsync? If StopAsync wasn't wrapped in a SemaphoreSlim, what are the chances two threads to pass through the if condition and call CloseOutputAsync twice? The first one would close the output successfully, but the second call would fail with WebSocketException because the web socket is already closed. Is that a race condition too taking into account that we aren't changing any variables? Is it going to happen at all and do I really need the try/catch that wraps the CloseOutputAsync, taking into account that no other part of my code closes the web socket?

I reused that SemaphoreSlim in StopAsync in order to prevent calling StartAsync and StopAsync at the same time, is that a good idea?

private readonly SemaphoreSlim _semaphore = new(1, 1);

public async Task StartAsync(string url)
{
    await _semaphore.WaitAsync().ConfigureAwait(false);

    try
    {
        var ws = new ClientWebSocket();
        _clientWebSocket = ws;

        await _clientWebSocket.ConnectAsync(new Uri(url), CancellationToken.None).ConfigureAwait(false); // TODO: Handle

        _tokenSource = new CancellationTokenSource();

        _ = ReceiveLoopAsync(ws, _tokenSource.Token);
        _ = SendLoopAsync(ws);
    }
    finally
    {
        _semaphore.Release();
    }
}

public async Task StopAsync()
{
    await _semaphore.WaitAsync().ConfigureAwait(false);

    try
    {
        if (_clientWebSocket is { State: not (WebSocketState.Aborted or WebSocketState.Closed or WebSocketState.CloseSent) })
        {
            try
            {
                await _clientWebSocket.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, string.Empty, CancellationToken.None).ConfigureAwait(false);
            }
            catch (Exception) // thrown when we try to close an already closed socket
            {
            }
        }

        _clientWebSocket?.Dispose();
        _clientWebSocket = null;

        _tokenSource?.Cancel();
    }
    finally
    {
        _semaphore.Release();
    }
}

CodePudding user response：

I would say that if you expect StopAsync being called in parallel then it's worth having those measures around.

Though there is another potential flaw that I see in your code - you are sharing _clientWebSocket and _tokenSource between the start and stop so potentially you can end in next situation:

1. Thread 1 calls StartAsync and initializes _clientWebSocket and _tokenSource and exists the semaphore
2. Thread 2 calls StartAsync and overwrites _clientWebSocket and _tokenSource
3. Thread 1 StopAsync calls StopAsync which will stop ClientWebSocket created by Thread 2 but no one will close the one created by Thread 1