Asynchronous Sockets

Question

I need to implement a TCP client application. The client and the server send messages to each other. I want to make this program scalable enough to handle connections to multiple servers at the same time. It seems like asynchronous sockets is the way to go for this. I'm new to C# so I'm pretty sure I don't know what I'm doing here. I wrote some classes and a simple console program to get started with. Eventually, I want to create a Windows Forms application but I want to start small and simple first. The Client class runs in its own thread. Is this all thread-safe and correctly done? It's a lot of code and I tried to cut out some fat.

Program.cs

    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Threading;

    namespace FastEyeClient
    {
        class Program
        {
            static void Main(string[] args)
            {
                Client client = new Client();
                client.ConnectEvent += new ConnectEventHandler(OnConnect);
                client.SetLiveStatusEvent += new SetLiveStatusEventHandler(OnSetLiveStatus);

                client.Connect("hostname", 1987);

                Thread.Sleep(1000);

                client.SetLiveStatus("hostname", true);
            }

            private static void OnConnect(object sender, ConnectEventArgs e)
            {
                Console.WriteLine(e.Message);
            }

            private static void OnSetLiveStatus(object sender, SetLiveStatusEventArgs e)
            {
                Console.WriteLine(e.Message);
            }
        }
    }

Client.cs

    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Net;
    using System.Net.Sockets;
    using System.Threading;

    namespace FastEyeClient
    {
        public delegate void ConnectEventHandler(object sender, ConnectEventArgs e);
        public delegate void SetLiveStatusEventHandler(object sender, SetLiveStatusEventArgs e);

        public class Client : IDisposable
        {
            public event ConnectEventHandler ConnectEvent;
            public event SetLiveStatusEventHandler SetLiveStatusEvent;

            ServerManager m_Manager;

            EventWaitHandle m_WaitHandle;
            readonly object m_Locker;
            Queue<Event> m_Tasks;
            Thread m_Thread;

            public Client()
            {
                m_Manager = new ServerManager(this);

                m_WaitHandle = new AutoResetEvent(false);
                m_Locker = new object();
                m_Tasks = new Queue<Event>();

                m_Thread = new Thread(Run);
                m_Thread.Start();
            }

            public void EnqueueTask(Event task)
            {
                lock (m_Locker)
                {
                    m_Tasks.Enqueue(task);
                }

                m_WaitHandle.Set();
            }

            public void Dispose()
            {
                EnqueueTask(null);
                m_Thread.Join();
                m_WaitHandle.Close();
            }

            private void Run()
            {
                while (true)
                {
                    Event task = null;

                    lock (m_Locker)
                    {
                        if (m_Tasks.Count > 0)
                        {
                            task = m_Tasks.Dequeue();

                            if (task == null)
                            {
                                return;
                            }
                        }
                    }

                    if (task != null)
                    {
                        task.DoTask(m_Manager);
                    }
                    else
                    {
                        m_WaitHandle.WaitOne();
                    }
                }
            }

            public void Connect(string hostname, int port)
            {
                EnqueueTask(new ConnectEvent(hostname, port));
            }

            public void SetLiveStatus(string hostname, bool status)
            {
                EnqueueTask(new SetLiveEvent(hostname, status));
            }

            public void OnConnect(bool isConnected, string message)
            {
                if (ConnectEvent != null)
                {
                    ConnectEvent(this, new ConnectEventArgs(isConnected, message));
                }
            }

            public void OnSetLiveStatus(string hostname, string message)
            {
                if (SetLiveStatusEvent != null)
                {
                    SetLiveStatusEvent(this, new SetLiveStatusEventArgs(hostname, message));
                }
            }
        }
    }

Server.cs

    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Net;
    using System.Net.Sockets;

    namespace FastEyeClient
    {
        public class Server
        {
            private ServerManager m_Manager;
            private string m_Hostname;
            private bool m_IsLive;

            private class StateObject
            {
                public Socket AsyncSocket = null;
                public const int BufferSize = 1024;
                public byte[] Buffer = new byte[BufferSize];
                public StringBuilder Builder = new StringBuilder();
            }

            public Server(ServerManager manager, Socket socket)
            {
                try
                {
                    m_Manager = manager;

                    IPEndPoint endPoint = (IPEndPoint)socket.RemoteEndPoint;
                    IPAddress ipAddress = endPoint.Address;
                    IPHostEntry hostEntry = Dns.GetHostEntry(ipAddress);
                    Hostname = hostEntry.HostName;

                    IsLive = false;

                    StateObject state = new StateObject();
                    state.AsyncSocket = socket;

                    socket.BeginReceive(state.Buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(ReceiveCallback), state);
                }
                catch (Exception)
                {
                }
            }

            public string Hostname
            {
                get
                {
                    return m_Hostname;
                }
                set
                {
                    m_Hostname = value;
                }
            }

            public bool IsLive
            {
                get
                {
                    return m_IsLive;
                }
                set
                {
                    m_IsLive = value;
                }
            }

            private void ReceiveCallback(IAsyncResult result)
            {
                try
                {
                    StateObject state = (StateObject)result.AsyncState;
                    Socket socket = state.AsyncSocket;

                    int read = socket.EndReceive(result);

                    if (read > 0)
                    {
                        state.Builder.Append(Encoding.ASCII.GetString(state.Buffer, 0, read));

                        if (state.Builder.Length > 1)
                        {
                            string messages = state.Builder.ToString();

                            ParseMessages(messages);
                        }
                    }

                    StateObject newState = new StateObject();
                    newState.AsyncSocket = socket;

                    socket.BeginReceive(newState.Buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(ReceiveCallback), newState);
                }
                catch (Exception)
                {
                }
            }

            private void ParseMessages(string messages)
            {
                string[] messagesArray = messages.Split('\n');

                foreach (string message in messagesArray)
                {
                    string[] tokens = message.Split(',');

                    if (tokens[0].Contains("@"))
                    {
                        ParseServerMessage(tokens);
                    }
                }
            }

            private void ParseServerMessage(string[] tokens)
            {
                tokens[0].Remove(0, 1);

                if (tokens[0] == "4")
                {
                    bool status;

                    if (tokens[1] == "0")
                    {
                        status = false;
                        m_Manager.SetLiveStatus(m_Hostname, status);
                    }
                    else if (tokens[1] == "1")
                    {
                        status = true;
                        m_Manager.SetLiveStatus(m_Hostname, status);
                    }
                }
            }
        }
    }

ServerManager.cs

    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Net;
    using System.Net.Sockets;

    namespace FastEyeClient
    {
        public class ServerManager
        {
            private Client m_Client;

            private Dictionary<string, Server> m_Servers;
            private object m_Locker;

            public ServerManager(Client client)
            {
                m_Client = client;

                m_Servers = new Dictionary<string, Server>();
                m_Locker = new object();
            }

            public void AddServer(string hostname, int port)
            {
                try
                {
                    IPAddress[] IPs = Dns.GetHostAddresses(hostname);

                    Socket socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);

                    socket.BeginConnect(IPs, port, new AsyncCallback(ConnectCallback), socket);
                }
                catch (Exception)
                {
                    bool isConnected = false;
                    string message = "Could not connect to server.";

                    m_Client.OnConnect(isConnected, message);
                }
            }

            private void ConnectCallback(IAsyncResult ar)
            {
                bool isConnected;
                string message;

                try
                {
                    Socket socket = (Socket)ar.AsyncState;

                    socket.EndConnect(ar);

                    IPEndPoint endPoint = (IPEndPoint)socket.RemoteEndPoint;
                    IPAddress ipAddress = endPoint.Address;
                    IPHostEntry hostEntry = Dns.GetHostEntry(ipAddress);
                    string hostname = hostEntry.HostName;

                    lock (m_Servers)
                    {
                        if (m_Servers.ContainsKey(hostname))
                        {
                            isConnected = false;
                            message = "Client is already connected to server";
                        }
                        else
                        {
                            m_Servers.Add(hostname, new Server(this, socket));

                            isConnected = true;
                            message = "Successfully connected.";
                        }
                    }

                    m_Client.OnConnect(isConnected, message);
                }
                catch (Exception)
                {
                    isConnected = false;
                    message = "Could not connect to server.";

                    m_Client.OnConnect(isConnected, message);
                }
            }

            public void SetLiveStatus(string hostname, bool newStatus)
            {
                string message;

                lock (m_Locker)
                {
                    if (m_Servers.ContainsKey(hostname))
                    {
                        if (m_Servers[hostname].IsLive == newStatus)
                        {
                            message = "Server is already set to this status.";
                        }
                        else
                        {
                            m_Servers[hostname].IsLive = newStatus;

                            message = "Successfully set new status.";
                        }
                    }
                    else
                    {
                        message = "Server not found.";
                    }
                }

                m_Client.OnSetLiveStatus(hostname, message);
            }
        }
    }

Answer 1

Thread safety stuff.

Raising events

This is not thread safe:

if (SetLiveStatusEvent != null)
{
    SetLiveStatusEvent(this, new SetLiveStatusEventArgs(hostname, message));
}

The value of SetLiveStatusEvent could change after the check for null, but before you invoke it. This not an issue in the example code you posted, but would be in a more complex system. Try something like this instead:

var handler = SetLiveStatusEvent;
if (handler != null)
{
    handler(this, new SetLiveStatusEventArgs(hostname, message));
}

When you are writing your event listeners, keep in mind that there is it possibility that they will be invoked after they have been unsubscribed from the event.

Use of Queue class with manual synchronization

I don't see any problems with how you interact with the Queue, but in .Net 4.0 there is the System.Collection.Concurrent.BlockingCollection class that will take care of all of the synchronization logic for you.

Other stuff

Explicit delegate creation

You don't need to create a new instance of a delegate.

socket.BeginConnect(IPs, port, new AsyncCallback(ConnectCallback), socket);
client.ConnectEvent += new ConnectEventHandler(OnConnect);

You can just pass the name of the method.

socket.BeginConnect(IPs, port, ConnectCallback, socket);
client.ConnectEvent += OnConnect;

Empty catch clause

In the ReceiveCallback method and some other places, there are empty catch (Exception) blocks, in production code I would want to at least see the exception logged.

Also, when possible avoid catching Exception in favor of more specific exceptions.

Auto Properties

Assuming you are using C# 4.0, you could use:

public string Hostname
{
   get;
   set;
}

Instead of:

public string Hostname
{
   get
   {
       return m_Hostname;
   }
   set
   {
       m_Hostname = value;
   }
}

Answer 2

Running Code Analysis in Visual Studio on this code throws up a few issues, with which I shall combine some of my own remarks.

Program.cs

Thread.Sleep(1000);

Why is this here? If it's because you're waiting for the client to connect, this is a bad idea because you don't know if the client might actually take longer than 1 second to connect. Instead, you should execute the next line inside the OnConnect event handler:

static void Main(string[] args)
{
    Client client = new Client();
    client.ConnectEvent += new ConnectEventHandler(OnConnect);
    client.SetLiveStatusEvent += new SetLiveStatusEventHandler(OnSetLiveStatus);

    client.Connect("hostname", 1987);
}

private static void OnConnect(object sender, ConnectEventArgs e)
{
    Console.WriteLine(e.Message);
    client.SetLiveStatus("hostname", true);
}

As mjcopple said in his answer, this:

client.ConnectEvent += new ConnectEventHandler(OnConnect)

and its equivalent on the next line, is redundant: you should only use new ConnectEventHandler when you're passing in a delegate, and even then not always. Rewrite those two lines:

client.ConnectEvent += OnConnect;
client.SetLiveStatusEvent += OnSetLiveStatus;

"hostname"

What sorcery is this? You've conjured a string from thin air! Where did that come from? You should avoid magic strings where possible, instead assigning them to a List<string> or Dictionary<string, string> of commonly used phrases within your application. The same can be said for 1987 in the Connect call: what is 1987 and where did it come from? Assign it to a resource list.

Client.cs

Code Analysis Says: Implement IDisposable correctly.
Your Client class nominally implements IDisposable, but you've only written half the implementation. You also need a Dispose(bool) method, called by Dispose(). If called as Dispose(true), it should also clean up managed resources; if it's Dispose(false) you should only clean up your class' resources. You also need to tell the garbage collector not to run the finalizer on your class because you've already implemented Dispose. In this case there's no difference that I can see, so a few simple modifications:

public void Dispose()
{
    this.Dispose(true);
    GC.SuppressFinalize(this);
}
public void Dispose(bool includeManaged)
{
    EnqueueTask(null);
    m_Thread.Join();
    m_WaitHandle.Close();
}

Naming
You've got some strange names in this class.

EnqueueTask()

What is this verb "Enqueue"? Method names of this format should be verb-object, as in "do this to this thing". If EnqueueTask simply adds the passed object to a queue, the correct name is QueueTask.

m_

Don't prefix your names like that. If you're adding a prefix, it should still follow all the rules of variable naming for C#. For private fields, variables are named in _lowerCamelCase, with the underscore before it. Public properties are named in PascalCase. So, you can rename a number of variables, for example m_client should be _mClient.

Server.cs

I do believe I've found an error here.

IPEndPoint endPoint = (IPEndPoint)socket.RemoteEndPoint;
IPAddress ipAddress = endPoint.Address;
IPHostEntry hostEntry = Dns.GetHostEntry(ipAddress);
Hostname = hostEntry.HostName;

Where is Hostname declared? I can't find a field or property called Hostname in this class. If Hostname is a type, it needs a variable identifier after it; if it's a declaration and identifier, it needs a type before it. Were you trying to assign to m_Hostname? I stand corrected: the property concerned does exist.

Naming - the same points apply here as to Client.

catch(Exception) {
}

Did you just... catch a generic exception? And ignore it? You need to work out what exceptions to catch and what to do with them. In this case, you're most likely to get a SocketException or InvalidOperationException, assuming your arguments are constructed properly so you don't get an ArgumentException or ArgumentNullException. So instead of catching a generic exception, catch both of those and tell someone about them. Something's gone wrong and this program doesn't work, I want to know why.

catch(SocketException e)
{
    Console.WriteLine("Could not receive data from the client - there was an error in the connection.");
    Console.WriteLine("Error message: {0}", e.Message);
}
catch(InvalidOperationException e)
{
    Console.WriteLine("Could not receive data from the client - some of the server's data is invalid.");
    Console.WriteLine("Error message: {0}", e.Message);
}

ServerManager.cs

This class is generally good, apart from naming. Rename your variables here and you're OK on this class.