Send message to all connected clients issue

[mattkbach]

Hello!

I'm not sure how to do this properly so I could be doing this wrong but basically I'm trying to send a messages to all connected clients. This is the code I'm using:

internal static void WSSendUpdate(string data)
        {
            var clients = new List<string>(_WSServer.ListClients());
            if (clients.Count() > 0)
            {
                foreach (var client in clients)
                {
                    _WSServer.SendAsync(data);
                }
            }
        }

This works well except for when a client disconnects. Client connects, data streams great no problem, then on disconnect this is what happens:

[WatsonWsServer **IP**:49753] disconnected
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed
[WatsonWsServer **IP**:49753] disposed

And this repeats infinitely every time the WSSendUpdate() method is called and bascially locks up the application.

Any help or suggestions on this would be greatly appreciated!

Thanks

[mattkbach]

Resolved this by keeping track of the connections in a separate list and not using _WSServer.ListClients()

[mattkbach]

Ok so after digging into this a bit more it seems the issue is these log entries are actually coming from the SendAsync catch exception and probably a queue of messages that still need to be sent.

I'm not sure how exactly CancellationTokens work as I have not used them before but would using CancellationTokens help clear up these messages/queue faster? In the SendAsync() function, it seems CancellationTokens are handled in the same way as an exception.

As for the CancellationToken implementation and managing the tokens, this is what I have come up with far. However, I'm still getting a disposed exception and not a token cancellation exception. These is the code I have:

static Dictionary<string, CancellationTokenSource> _WSClients = new Dictionary<string, CancellationTokenSource>();

static void WSClientConnected(object sender, ClientConnectedEventArgs args)
{
    LogEntry("[Websocket] Client connected: " + args.IpPort);
    _WSLastIpPort = args.IpPort;
    if (!_WSClients.Keys.Contains(args.IpPort))
    {
        CancellationTokenSource _tokenSource = new CancellationTokenSource();
        _WSClients.Add(args.IpPort, _tokenSource);
    }
}

static void WSClientDisconnected(object sender, ClientDisconnectedEventArgs args)
{
    LogEntry("[Websocket] Client disconnected: " + args.IpPort);
    if (_WSClients.Keys.Contains(args.IpPort))
    {
        _WSClients[args.IpPort].Cancel();
        _WSClients.Remove(args.IpPort);
    }
}

internal static void WSSendUpdate(string data)
{
    if (_WSClients.Count > 0)
    {
        foreach (var client in _WSClients)
        {
            try
            {
                _WSServer.SendAsync(client.Key, data, client.Value.Token);
            }
            catch (Exception e)
            {
                LogEntry($"[Websocket] Catch Error {e}");
            }

        }
    }
}

Sorry for the noob questions ... still new to C# and just learning.

[jchristn]

I'm sorry I just saw that you re-opened this.

Dictionary is not thread-safe. The collection can change if clients connect/disconnect while you're in the middle of iterating during WSSendUpdate. I'd recommend, in WSSendUpdate, to make a copy of the dictionary, and iterate over the copy, where each .SendAsync is encapsulated in a try/catch.

[jchristn]

Either that, or use a lock around dictionary access.

Here is a code sample from the earlier recommendation:

internal static void WSSendUpdate(string data)
{
  Dictionary<string, CancellationTokenSource> temp = new Dictionary<string, CancellationTokenSource>(_WSClients);

  if (temp.Count > 0)
  {
    foreach (var client in _WSClients)
    {
      try
      {
        if (temp.ContainsKey(client.Key))
        {
          _WSServer.SendAsync(client.Key, data, client.Value.Token);
        }
      }
      catch (Exception e)
      {
        // no longer connected
      }
    }
  }
}

And with a lock

static readonly object _Lock = new object();
static Dictionary<string, CancellationTokenSource> _WSClients = new Dictionary<string, CancellationTokenSource>();

static void WSClientConnected(object sender, ClientConnectedEventArgs args)
{
  LogEntry("[Websocket] Client connected: " + args.IpPort);
  _WSLastIpPort = args.IpPort;
  lock (_Lock)
  {
    if (!_WSClients.Keys.Contains(args.IpPort))
    {
      CancellationTokenSource _tokenSource = new CancellationTokenSource();
      _WSClients.Add(args.IpPort, _tokenSource);
    }
  }
}

static void WSClientDisconnected(object sender, ClientDisconnectedEventArgs args)
{
  LogEntry("[Websocket] Client disconnected: " + args.IpPort);
  lock (_Lock)
  {
    if (_WSClients.Keys.Contains(args.IpPort))
    {
      _WSClients[args.IpPort].Cancel();
      _WSClients.Remove(args.IpPort);
    }
  }
}

internal static void WSSendUpdate(string data)
{
  lock (_Lock)
  {
    foreach (var client in _WSClients)
    {
      try
      {
        if (temp.ContainsKey(client.Key))
        {
          _WSServer.SendAsync(client.Key, data, client.Value.Token);
        }
      }
      catch (Exception e)
      {
        // no longer connected
      }
    }
  }
}

The upside with the lock approach is that you're always working on the master copy. The downside is that sending updates locks the dictionary, so it can't be modified while sending updates. Meaning if someone connects or disconnects, the event will hang until the lock is released.