c# AsyncSocket Server need locking?

Question

i have written a TCP Socket-Server. In general i want to have the following behaviour:

• A Listening Socket can accept N Connections (There a multiple Listeners on different Ports (1337,733) )
• A Connection can authenticated itself as a "Client", multiple connections can grouped to one "Client"
• Multiple connection can accepted / revice data at the same time (concurrency)

Here my Server-Class:

class Server
{
    internal List ConnectionListeners;

    internal bool IsListening = false;

    internal Server()
    {
        this.ClientManager = new ClientManager();
        this.ConnectionManager = new ConnectionManager();
        this.ConnectionListeners = new List();
    }

    internal void AddConnectionListener(int Port)
    {
        ConnectionListener c = new ConnectionListener(Port);
        c.AcceptedConnection += new ConnectionListener.AcceptedConnectionEventHandler(ConnectionProcessor.AcceptConnection);
        ConnectionListeners.Add(c);
    }

    internal void RemoveConnectionListener(ConnectionListener ConnectionListener)
    {
        ConnectionListeners.Remove(ConnectionListener);
    }

    public delegate void OnStartListeningEventHandler();
    public event OnStartListeningEventHandler OnStartListening;

    internal void StartListening()
    {
        IsListening = true;

        foreach (ConnectionListener cl in this.ConnectionListeners)
        {
            cl.StartListening();
        }

        OnStartListening?.Invoke();
    }

    public delegate void OnStopListeningEventHandler();
    public event OnStopListeningEventHandler OnStopListening;

    internal void StopListening()
    {
        ConnectionManager.DisconnectConnections();
        foreach (ConnectionListener cl in this.ConnectionListeners)
        {
            cl.StopListening();
        }
        IsListening = false;
        OnStopListening?.Invoke();
    }

}

Method of ConnectionProcessor where i hande a new Accept Connection (ConnectionProcessor.AcceptConnection):

        internal void AcceptConnection(Socket Socket)
        {
            Connection Connection = new Connection(Socket);
            Connection.Sent += new Connection.SentEventHandler(onSend);
            Connection.Received += new Connection.ReceivedEventHandler(onRecive);
            Connection.Disconnected += new Connection.DisconnectedEventHandler(OnDisconnect);
            Connection.Recive();

            Logger.Instance.AddLog(new LogMessage(LogMessage.LogLevel.Normal, "Connection ("+Connection.ConnectionId+") Accepted"));
            ConnectionManager.AddConnection(Connection);
        }

ConnectionManager:

    class ConnectionManager
    {
        internal ConnectionManager()
        {
            this.Connections = new List();
        }

        internal void AddConnection(Connection Connection)
        {
            Connections.Add(Connection);
            OnAddConnection(Connection);
        }

        internal void RemoveConnection(Connection Connection)
        {
            Connections.Remove(Connection);
            OnRemoveConnection(Connection);
        }

        internal void DisconnectConnections()
        {
            foreach (Connection c in Connections)
            {
                c.Disconnect();
            }
        }
    }

Everything seems to work, but I am unsure about concurrency.

As you can see in the ConnectionManger, i store each Connection in a List (Connections.Add(Connection)). Its enough do to this? I have to care about that a normal "List" is not Thread safe?

Is my "design" in gerneal the right way to solve my requirements?

Answer 1

Since all you do is adding\removing\enumerating connections in your list - you can use thread-safe collection, without any locks. Unfortunately, there is no ConcurrentList or ConcurrentHashSet, but you can use ConcurrentDictionary with dummy keys, either directly or by wrapping in separate class:

class BasicConcurrentSet<T> : IEnumerable<T> {
    private readonly ConcurrentDictionary<T, byte> _items = new ConcurrentDictionary<T, byte>();

    public void Add(T item) {
        _items.TryAdd(item, 0);
    }

    public void Remove(T item) {
        byte tmp;
        _items.TryRemove(item, out tmp);
    }

    IEnumerator IEnumerable.GetEnumerator() {
        return GetEnumerator();
    }

    public IEnumerator<T> GetEnumerator() {
        foreach (var kv in _items) {
            yield return kv.Key;
        }
    }
}

Adding, removing and enumerating items from such concurrent collection is thread safe.

class ConnectionManager {
    private readonly BasicConcurrentSet<Connection> _connections = new BasicConcurrentSet<Connection>();

    internal ConnectionManager() {

    }

    internal void AddConnection(Connection connection) {
        _connections.Add(connection);
        OnAddConnection(Connection);
    }

    internal void RemoveConnection(Connection connection) {
        _connections.Remove(connection);
        OnRemoveConnection(connection);
    }

    internal void DisconnectConnections() {
        foreach (var connection in _connections) {
            connection.Disconnect();
        }
    }
}

Answer 2

Even if you incapsulate Connections (should be private) inside ConnectionManager class and situation when RemoveConnection or DisconnectConnections can be earlier then AddConnection is impossible - no, this is not thread safe. So my suggestion is make all 3 functions thread safe like this :

  private Object lck;

  internal ConnectionManager()
    {
        lck = new Object();
        this.Connections = new List();
    }

  internal void AddConnection(Connection Connection)
    {
     lock (lck)
      {
        Connections.Add(Connection);
        OnAddConnection(Connection);
      }
    }