TCP async socket server client communication

Question

I develop my first async TCP socket server and client program in c# and would like to review the first parts of it. I like to get some information’s about smelly code that I missed and what I could improve in the program. I know it's a little bit much code, but it would awesome if you help me to improve my skills!

I have to classes AsyncSocketListener and AsyncClient. Both use events to tell the main program if messages sent or received. Here are the classes:

AsyncSocketListener

static class AsyncSocketListener
{
    private static ushort port = 8080;
    private static ushort limit = 250;

    private static ManualResetEvent mre = new ManualResetEvent(false);
    private static Dictionary<int, StateObject> clients = new Dictionary<int, StateObject>();

    #region Event handler
    public delegate void MessageReceivedHandler(int id, String msg);
    public static event MessageReceivedHandler MessageReceived;

    public delegate void MessageSubmittedHandler(int id, bool close);
    public static event MessageSubmittedHandler MessageSubmitted;
    #endregion

    /* Starts the AsyncSocketListener */
    public static void StartListening()
    {
        IPHostEntry host = Dns.GetHostEntry(String.Empty);
        IPAddress ip = host.AddressList[3];
        IPEndPoint socket = new IPEndPoint(ip, port);

        try
        {
            using (Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
            {
                listener.Bind(socket);
                listener.Listen(limit);
                while (true)
                {
                    mre.Reset();
                    listener.BeginAccept(new AsyncCallback(OnClientConnect), listener);
                    mre.WaitOne();
                }
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    /* Gets a socket from the clients dictionary by his Id. */
    private static StateObject getClient(int id)
    {
        StateObject state = new StateObject();
        return clients.TryGetValue(id, out state) ? state : null;
    }

    /* Checks if the socket is connected. */
    public static bool IsConnected(int id)
    {
        StateObject state = getClient(id);
        return !(state.listener.Poll(1000, SelectMode.SelectRead) && state.listener.Available == 0);
    }

    /* Add a socket to the clients dictionary. Lock clients temporary to handle multiple access.
     * ReceiveCallback raise a event, after the message receive complete. */
    #region Receive data
    public static void OnClientConnect(IAsyncResult result)
    {
        mre.Set();
        StateObject state = new StateObject();
        try
        {
            lock (clients)
            {
                state.Id = !clients.Any() ? 1 : clients.Keys.Max() + 1;
                clients.Add(state.Id, state);
                Console.WriteLine("Client connected. Get Id " + state.Id);
            }
            state.listener = (Socket)result.AsyncState;
            state.listener = state.listener.EndAccept(result);
            state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public static void ReceiveCallback(IAsyncResult result)
    {
        StateObject state = (StateObject)result.AsyncState;
        try
        {
            int receive = state.listener.EndReceive(result);
            if (receive > 0)
                state.sb.Append(Encoding.UTF8.GetString(state.buffer, 0, receive));
            if (receive == StateObject.BufferSize)
                state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
            else
            {
                MessageReceived(state.Id, state.sb.ToString());
                state.sb = new StringBuilder();
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }
    #endregion

    /* Send(int id, String msg, bool close) use bool to close the connection after the message sent. */
    #region Send data
    public static void Send(int id, String msg, bool close)
    {
        StateObject state = getClient(id);
        if (state == null)
            throw new Exception("Client does not exist.");
        if (!IsConnected(state.Id))
            throw new Exception("Destination socket is not connected.");
        try
        {
            byte[] send = Encoding.UTF8.GetBytes(msg);
            state.Close = close;
            state.listener.BeginSend(send, 0, send.Length, SocketFlags.None, new AsyncCallback(SendCallback), state);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ArgumentException)
        {
            // TODO:
        }
    }

    private static void SendCallback(IAsyncResult result)
    {
        StateObject state = (StateObject)result.AsyncState;
        try
        {
            state.listener.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO:
        }
        finally
        {
            MessageSubmitted(state.Id, state.Close);
        }
    }
    #endregion

    public static void Close(int id)
    {
        StateObject state = getClient(id);
        if (state == null)
            throw new Exception("Client does not exist.");
        try
        {
            state.listener.Shutdown(SocketShutdown.Both);
            state.listener.Close();
        }
        catch (SocketException)
        {
            // TODO:
        }
        finally
        {
            lock (clients)
            {
                clients.Remove(state.Id);
                Console.WriteLine("Client disconnected with Id {0}", state.Id);
            }
        }
    }
}

AsyncClient

class AsyncClient : IDisposable
{
    private const ushort port = 8080;

    private Socket listener = null;
    private bool close = false;

    public ManualResetEvent connected = new ManualResetEvent(false);
    public ManualResetEvent sent = new ManualResetEvent(false);
    public ManualResetEvent received = new ManualResetEvent(false);

    #region Event handler
    public delegate void ConnectedHandler(AsyncClient a);
    public static event ConnectedHandler Connected;

    public delegate void MessageReceivedHandler(AsyncClient a, String msg);
    public static event MessageReceivedHandler MessageReceived;

    public delegate void MessageSubmittedHandler(AsyncClient a, bool close);
    public static event MessageSubmittedHandler MessageSubmitted;
    #endregion

    /* Starts the AsyncClient */
    public AsyncClient()
    {
    }

    public void StartClient()
    {
        IPHostEntry host = Dns.GetHostEntry(String.Empty);
        IPAddress ip = host.AddressList[3];
        IPEndPoint socket = new IPEndPoint(ip, port);

        try
        {
            this.listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            this.listener.BeginConnect(socket, new AsyncCallback(OnConnectCallback), listener);
            connected.WaitOne();
            Connected(this);
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public bool IsConnected()
    {
        return !(this.listener.Poll(1000, SelectMode.SelectRead) && this.listener.Available == 0);
    }

    private void OnConnectCallback(IAsyncResult result)
    {
        Socket server = (Socket)result.AsyncState;
        try
        {
            server.EndConnect(result);
            connected.Set();
        }
        catch (SocketException)
        {
        }
    }

    #region Receive data
    public void Receive()
    {
        StateObject state = new StateObject();
        state.listener = this.listener;
        state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
    }

    private void ReceiveCallback(IAsyncResult result)
    {
        StateObject state = (StateObject)result.AsyncState;

        int receive = state.listener.EndReceive(result);
        if (receive > 0)
            state.sb.Append(Encoding.UTF8.GetString(state.buffer, 0, receive));
        if (receive == StateObject.BufferSize)
            state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
        else
        {
            MessageReceived(this, state.sb.ToString());
            state.sb = new StringBuilder();
            received.Set();
        }
    }
    #endregion

    #region Send data
    public void Send(String msg, bool close)
    {
        if (!IsConnected())
            throw new Exception("Destination socket is not connected.");
        byte[] response = Encoding.UTF8.GetBytes(msg);
        this.close = close;
        this.listener.BeginSend(response, 0, response.Length, SocketFlags.None, new AsyncCallback(SendCallback), this.listener);
    }

    private void SendCallback(IAsyncResult result)
    {
        try
        {
            Socket resceiver = (Socket)result.AsyncState;
            resceiver.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO;
        }
        MessageSubmitted(this, this.close);
        sent.Set();
    }
    #endregion

    private void Close()
    {
        try
        {
            if (IsConnected())
            {
                this.listener.Shutdown(SocketShutdown.Both);
                this.listener.Close();
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public void Dispose()
    {
        connected.Close();
        sent.Close();
        received.Close();
        Close();
    }
}

I save the connection information in a third class called StateObject.

StateObject

class StateObject
{
    /* Contains the state information. */
    private int id;
    private bool close = false; // Used to close the socket after the message sent.

    public Socket listener = null;
    public const int BufferSize = 1024;
    public byte[] buffer = new byte[BufferSize];
    public StringBuilder sb = new StringBuilder();

    public StateObject() { }

    public int Id
    {
        get { return this.id; }
        set { this.id = value; }
    }

    public bool Close
    {
        get { return this.close; }
        set { this.close = value; }
    }
}

With the following code I test the server and client:

public Server()
{
    InitializeComponent();
    /* I use this to test my code on one machine. */
    new Thread(new ThreadStart(AsyncSocketListener.StartListening)).Start();
    AsyncSocketListener.MessageReceived += new AsyncSocketListener.MessageReceivedHandler(ClientMessageReceived);
    AsyncSocketListener.MessageSubmitted += new AsyncSocketListener.MessageSubmittedHandler(ServerMessageSubmitted);

    AsyncClient.Connected += new AsyncClient.ConnectedHandler(ConnectedToServer);
    AsyncClient.MessageReceived += new AsyncClient.MessageReceivedHandler(ServerMessageReceived);
    AsyncClient.MessageSubmitted += new AsyncClient.MessageSubmittedHandler(ClientMessageSubmitted);

    for (int i = 0; i < 10; i++)
    {
        AsyncClient client = new AsyncClient();
        Thread thread = new Thread(new ThreadStart(client.StartClient));
        thread.Name = "Client" + i;
        thread.Start();
    }
}

/* Code to handle the events from AsyncSocketListener and AsyncClient. */
#region Server Code
private static void ClientMessageReceived(int id, String msg)
{
    AsyncSocketListener.Send(id, msg.Replace("client", "server"), true);
    Console.WriteLine("Server get Message from client. {0} ", msg);
}

private static void ServerMessageSubmitted(int id, bool close)
{
    if (close)
        AsyncSocketListener.Close(id);
}
#endregion

#region Client code
private static void ConnectedToServer(AsyncClient a)
{
    a.Send("Hello, I'm the client.", false);
    a.sent.WaitOne();
    a.Receive();
    a.received.WaitOne();
}

private static void ServerMessageReceived(AsyncClient a, String msg)
{
    Console.WriteLine("Client get Message from server. {0} ", msg);
}

private static void ClientMessageSubmitted(AsyncClient a, bool close)
{
    if (close)
        a.Dispose();
}
#endregion

If the code is fine, I will use polymorphism for the AsyncSocketListener and AsyncClient because some parts are very similar. What do you think?

Edit:

At the moment the client could connect and send a message to the server. The server could reply on it. But it is not possible to exchange messages after this. I'm trying to fix this, but I can't find a solution. Any idea?

Answer 1

0: Keep class members private unless there is a darn good reason to expose them. And then, if you have to, use properties.

1: Use readonly on class declarations which are considered unmodifiable after construction. Example:

public readonly ManualResetEvent connected = new ManualResetEvent(false);

This declares intent and keeps other code from accidentally modifying an invariant. Plus, the runtime can sometimes perform certain optimizations knowing a field is readonly.

2: Develop to interfaces. This allows for decoupling of implementation plus ease of testing/mocking.

So this being said, here's how I refactored it:

IStateObject:

public interface IStateObject
{
    int BufferSize { get; }

    int Id { get; }

    bool Close { get; set; }

    byte[] Buffer { get; }

    Socket Listener { get; }

    string Text { get; }

    void Append(string text);

    void Reset();
}

StateObject:

public sealed class StateObject : IStateObject
{
    /* Contains the state information. */

    private const int Buffer_Size = 1024;
    private readonly byte[] buffer = new byte[Buffer_Size];
    private readonly Socket listener;
    private readonly int id;
    private StringBuilder sb;

    public StateObject(Socket listener, int id = -1)
    {
        this.listener = listener;
        this.id = id;
        this.Close = false;
        this.Reset();
    }

    public int Id
    {
        get
        {
            return this.id;
        }
    }

    public bool Close { get; set; }

    public int BufferSize
    {
        get
        {
            return Buffer_Size;
        }
    }

    public byte[] Buffer
    {
        get
        {
            return this.buffer;
        }
    }

    public Socket Listener
    {
        get
        {
            return this.listener;
        }
    }

    public string Text
    {
        get
        {
            return this.sb.ToString();
        }
    }

    public void Append(string text)
    {
        this.sb.Append(text);
    }

    public void Reset()
    {
        this.sb = new StringBuilder();
    }
}

IAsyncSocketListener:

public interface IAsyncSocketListener : IDisposable
{
    event MessageReceivedHandler MessageReceived;

    event MessageSubmittedHandler MessageSubmitted;

    void StartListening();

    bool IsConnected(int id);

    void OnClientConnect(IAsyncResult result);

    void ReceiveCallback(IAsyncResult result);

    void Send(int id, string msg, bool close);

    void Close(int id);
}

AsyncSocketListener:

public delegate void MessageReceivedHandler(int id, string msg);
public delegate void MessageSubmittedHandler(int id, bool close);

public sealed class AsyncSocketListener : IAsyncSocketListener
{
    private const ushort Port = 8080;
    private const ushort Limit = 250;

    private static readonly IAsyncSocketListener instance = new AsyncSocketListener();

    private readonly ManualResetEvent mre = new ManualResetEvent(false);
    private readonly IDictionary<int, IStateObject> clients = new Dictionary<int, IStateObject>();

    public event MessageReceivedHandler MessageReceived;

    public event MessageSubmittedHandler MessageSubmitted;

    private AsyncSocketListener()
    {
    }

    public static IAsyncSocketListener Instance
    {
        get
        {
            return instance;
        }
    }

    /* Starts the AsyncSocketListener */
    public void StartListening()
    {
        var host = Dns.GetHostEntry(string.Empty);
        var ip = host.AddressList[3];
        var endpoint = new IPEndPoint(ip, Port);

        try
        {
            using (var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
            {
                listener.Bind(endpoint);
                listener.Listen(Limit);
                while (true)
                {
                    this.mre.Reset();
                    listener.BeginAccept(this.OnClientConnect, listener);
                    this.mre.WaitOne();
                }
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    /* Gets a socket from the clients dictionary by his Id. */
    private IStateObject GetClient(int id)
    {
        IStateObject state;

        return this.clients.TryGetValue(id, out state) ? state : null;
    }

    /* Checks if the socket is connected. */
    public bool IsConnected(int id)
    {
        var state = this.GetClient(id);

        return !(state.Listener.Poll(1000, SelectMode.SelectRead) && state.Listener.Available == 0);
    }

    /* Add a socket to the clients dictionary. Lock clients temporary to handle multiple access.
     * ReceiveCallback raise a event, after the message receive complete. */
    #region Receive data
    public void OnClientConnect(IAsyncResult result)
    {
        this.mre.Set();

        try
        {
            IStateObject state;

            lock (this.clients)
            {
                var id = !this.clients.Any() ? 1 : this.clients.Keys.Max() + 1;

                state = new StateObject(((Socket)result.AsyncState).EndAccept(result), id);
                this.clients.Add(id, state);
                Console.WriteLine("Client connected. Get Id " + id);
            }

            state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public void ReceiveCallback(IAsyncResult result)
    {
        var state = (IStateObject)result.AsyncState;

        try
        {
            var receive = state.Listener.EndReceive(result);

            if (receive > 0)
            {
                state.Append(Encoding.UTF8.GetString(state.Buffer, 0, receive));
            }

            if (receive == state.BufferSize)
            {
                state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
            }
            else
            {
                var messageReceived = this.MessageReceived;

                if (messageReceived != null)
                {
                    messageReceived(state.Id, state.Text);
                }

                state.Reset();
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }
    #endregion

    /* Send(int id, String msg, bool close) use bool to close the connection after the message sent. */
    #region Send data
    public void Send(int id, string msg, bool close)
    {
        var state = this.GetClient(id);

        if (state == null)
        {
            throw new Exception("Client does not exist.");
        }

        if (!this.IsConnected(state.Id))
        {
            throw new Exception("Destination socket is not connected.");
        }

        try
        {
            var send = Encoding.UTF8.GetBytes(msg);

            state.Close = close;
            state.Listener.BeginSend(send, 0, send.Length, SocketFlags.None, this.SendCallback, state);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ArgumentException)
        {
            // TODO:
        }
    }

    private void SendCallback(IAsyncResult result)
    {
        var state = (IStateObject)result.AsyncState;

        try
        {
            state.Listener.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO:
        }
        finally
        {
            var messageSubmitted = this.MessageSubmitted;

            if (messageSubmitted != null)
            {
                messageSubmitted(state.Id, state.Close);
            }
        }
    }
    #endregion

    public void Close(int id)
    {
        var state = this.GetClient(id);

        if (state == null)
        {
            throw new Exception("Client does not exist.");
        }

        try
        {
            state.Listener.Shutdown(SocketShutdown.Both);
            state.Listener.Close();
        }
        catch (SocketException)
        {
            // TODO:
        }
        finally
        {
            lock (this.clients)
            {
                this.clients.Remove(state.Id);
                Console.WriteLine("Client disconnected with Id {0}", state.Id);
            }
        }
    }

    public void Dispose()
    {
        foreach (var id in this.clients.Keys)
        {
            this.Close(id);
        }

        this.mre.Dispose();
    }
}

IAsyncClient:

public interface IAsyncClient : IDisposable
{
    event ConnectedHandler Connected;

    event ClientMessageReceivedHandler MessageReceived;

    event ClientMessageSubmittedHandler MessageSubmitted;

    void StartClient();

    bool IsConnected();

    void Receive();

    void Send(string msg, bool close);
}

AsyncClient:

public delegate void ConnectedHandler(IAsyncClient a);
public delegate void ClientMessageReceivedHandler(IAsyncClient a, string msg);
public delegate void ClientMessageSubmittedHandler(IAsyncClient a, bool close);

public sealed class AsyncClient : IAsyncClient
{
    private const ushort Port = 8080;

    private Socket listener;
    private bool close;

    private readonly ManualResetEvent connected = new ManualResetEvent(false);
    private readonly ManualResetEvent sent = new ManualResetEvent(false);
    private readonly ManualResetEvent received = new ManualResetEvent(false);

    public event ConnectedHandler Connected;

    public event ClientMessageReceivedHandler MessageReceived;

    public event ClientMessageSubmittedHandler MessageSubmitted;

    public void StartClient()
    {
        var host = Dns.GetHostEntry(string.Empty);
        var ip = host.AddressList[3];
        var endpoint = new IPEndPoint(ip, Port);

        try
        {
            this.listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            this.listener.BeginConnect(endpoint, this.OnConnectCallback, this.listener);
            this.connected.WaitOne();

            var connectedHandler = this.Connected;

            if (connectedHandler != null)
            {
                connectedHandler(this);
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public bool IsConnected()
    {
        return !(this.listener.Poll(1000, SelectMode.SelectRead) && this.listener.Available == 0);
    }

    private void OnConnectCallback(IAsyncResult result)
    {
        var server = (Socket)result.AsyncState;

        try
        {
            server.EndConnect(result);
            this.connected.Set();
        }
        catch (SocketException)
        {
        }
    }

    #region Receive data
    public void Receive()
    {
        var state = new StateObject(this.listener);

        state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
    }

    private void ReceiveCallback(IAsyncResult result)
    {
        var state = (IStateObject)result.AsyncState;
        var receive = state.Listener.EndReceive(result);

        if (receive > 0)
        {
            state.Append(Encoding.UTF8.GetString(state.Buffer, 0, receive));
        }

        if (receive == state.BufferSize)
        {
            state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
        }
        else
        {
            var messageReceived = this.MessageReceived;

            if (messageReceived != null)
            {
                messageReceived(this, state.Text);
            }

            state.Reset();
            this.received.Set();
        }
    }
    #endregion

    #region Send data
    public void Send(string msg, bool close)
    {
        if (!this.IsConnected())
        {
            throw new Exception("Destination socket is not connected.");
        }

        var response = Encoding.UTF8.GetBytes(msg);

        this.close = close;
        this.listener.BeginSend(response, 0, response.Length, SocketFlags.None, this.SendCallback, this.listener);
    }

    private void SendCallback(IAsyncResult result)
    {
        try
        {
            var resceiver = (Socket)result.AsyncState;

            resceiver.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO;
        }

        var messageSubmitted = this.MessageSubmitted;

        if (messageSubmitted != null)
        {
            messageSubmitted(this, this.close);
        }

        this.sent.Set();
    }
    #endregion

    private void Close()
    {
        try
        {
            if (!this.IsConnected())
            {
                return;
            }

            this.listener.Shutdown(SocketShutdown.Both);
            this.listener.Close();
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public void Dispose()
    {
        this.connected.Dispose();
        this.sent.Dispose();
        this.received.Dispose();
        this.Close();
    }
}

Answer 2

White space and Casing is good.

Just a few things to think about:

1. #region is not very well accepted. If you have to use #region, then you should look at moving the code out into a method or its own class.
2. Use var instead of explicit declarations for obvious variables. This makes the code much easier to scan over.
3. If you insist on comments to explain what the method is doing, us the C# /// syntax. This does a couple of things: it allows intellisense to pick up the description when using the library, and there are tools that can take the /// comments and create API help documentation.
4. General practice in C# is to use either m_ or _ at the beginning of class variables. This eliminates the need to use this., which clutters up the code a little.
5. Be consistent with your use of { } after if statements. It makes it eaasier to focus on the logic when it doesn't have to process changes in the formatting.
6. In the line thread.Name = "Client" + i; I would make "Client" a constant.
7. Separate concerns. For instance: Console.WriteLine does not belong in your server class.
8. If the constructor doesn't do anything, get rid of it, it is only adding noise to your class.
9. In my opinion, class variables should be initialized in the constructor, not on declaration.
10. Use meaningful names for variables: a does not portray what the variable does. tcpClient on the other hand does. This makes your methods much easier to read.
11. getClient should be GetClient. C# naming conventions

Most of these are minor, and don't affect the application, they just add that little bit to your code, and will make it easier to read and maintain in the future.