-3
\$\begingroup\$

I am trying to build a high performance socket server and this is what I have come up with so far. Can anyone please review my code and see if I can make any improvements on it?

// State object for reading client data asynchronously
public class StateObject
{
    // Client  socket.
    public Socket workSocket = null;

    // Size of receive buffer.
    public const int BufferSize = 1024;

    // Receive buffer.
    public byte[] buffer = new byte[BufferSize];

    // Received data string.
    public StringBuilder sb = new StringBuilder();

    // GUID of client for identification
    public string guid = string.Empty;
}
public class CAsnycSocketServer : IDisposable
{
    public AsyncCallback workerCallBack;
    private Socket mainSocket = null;
    private static ArrayList ClientSockets;
    private Socket workerSocket = null;
    private bool shutdownServer = false;

    public static ManualResetEvent allDone = new ManualResetEvent(false);

    public int MaxConnections { get; set; } //Not implemented

    /// <summary>
    /// Default constructor
    /// </summary>
    public CAsnycSocketServer()
    {
    }

    /// <summary>
    /// Constructor to start listening
    /// </summary>
    /// <param name="port"></param>
    public CAsnycSocketServer(int port)
    {
        StartListening(port);
    }

    public void ShutdownServer()
    {
        shutdownServer = true;
        CloseSockets();
    }

    public void Dispose()
    {
        shutdownServer = true;
        CloseSockets();
        mainSocket.Dispose();
        ClientSockets.Clear();
    }

    /// <summary>
    /// Gets the current connected client count
    /// </summary>
    /// <returns></returns>
    public int getConnectedClientCount()
    {
        lock (ClientSockets.SyncRoot)
        {
            return ClientSockets.Count;
        }
    }

    /// <summary>
    /// Closes the main listening socket and all the clients
    /// </summary>
    public void CloseSockets()
    {
        if (mainSocket != null)
        {
            mainSocket.Close();
        }

        lock (ClientSockets.SyncRoot)
        {
            foreach (StateObject so in ClientSockets)
            {
                if (so.workSocket != null)
                {
                    so.workSocket.Close();
                    so.workSocket.Dispose();
                }
            }
        }

        ClientSockets.Clear();
    }

    /// <summary>
    /// Starts listening for client connections
    /// </summary>
    /// <param name="port"></param>
    public void StartListening(int port)
    {
        try
        {
            mainSocket = new Socket(AddressFamily.InterNetwork,
                                      SocketType.Stream,
                                      ProtocolType.Tcp);

            IPEndPoint ipLocal = new IPEndPoint(IPAddress.Any, port);

            // Bind to local IP Address...
            mainSocket.Bind(ipLocal);
            // Start listening...
            mainSocket.Listen(100);

            ClientSockets = new ArrayList();

            while (!shutdownServer)
            {
                // Set the event to nonsignaled state.
                allDone.Reset();

                // Start an asynchronous socket to listen for connections.
                Console.WriteLine("Server listening for connections");
                mainSocket.BeginAccept(new AsyncCallback(ClientConnectCallback), mainSocket);

                // Wait until a connection is made before continuing.
                allDone.WaitOne();
            }
        }
        catch (Exception ex)
        {
        }
    }

    /// <summary>
    ///  Callback which occurs when a client connects to the server
    /// </summary>
    /// <param name="ar"></param>
    public void ClientConnectCallback(IAsyncResult ar)
    {
        // Signal the main thread to continue.
        allDone.Set();

        try
        {
            // Get the socket that handles the client request.
            Socket listener = (Socket)ar.AsyncState;
            Socket handler = listener.EndAccept(ar);

            // Create the state object.
            StateObject state = new StateObject();
            state.guid = Guid.NewGuid().ToString();

            state.workSocket = handler;

            ClientSockets.Add(state);
            Console.WriteLine("Client connected : " + state.guid + " Count : " + ClientSockets.Count);

            handler.BeginReceive(state.buffer, 0, StateObject.BufferSize, 0,
                new AsyncCallback(ClientDataCallback), state);
        }
        catch (ObjectDisposedException)
        {
        }
        catch (SocketException se)
        {
        }
    }

    /// <summary>
    /// Callback for when a client sends data to the server
    /// </summary>
    /// <param name="ar"></param>
    public void ClientDataCallback(IAsyncResult ar)
    {
        try
        {
            String content = String.Empty;

            // Retrieve the state object and the handler socket
            // from the asynchronous state object.
            StateObject state = (StateObject)ar.AsyncState;
            Socket handler = state.workSocket;

            // Read data from the client socket.
            int bytesRead = handler.EndReceive(ar);

            if (bytesRead == 0)
            {
                //Client has disconnected
                Console.WriteLine("Client disconnected : {0}", state.guid);
                CleanAndRemoveClient(state);
            }

            if (bytesRead > 0)
            {
                // There  might be more data, so store the data received so far.
                state.sb.Append(Encoding.ASCII.GetString(
                    state.buffer, 0, bytesRead));

                // Check for newline, If it is not there, read
                // more data.
                content = state.sb.ToString();

                if (content.EndsWith(Environment.NewLine) == true)
                {
                    // All the data has been read from the
                    // client. Display it on the console.
                    Console.WriteLine("Read {0} bytes from socket. \n Data : {1} GUID : {2}",
                        content.Length, content, state.guid);

                    // Echo the data back to the client.
                    //Send(handler, content);

                    state.sb = new StringBuilder();
                }
                else
                {
                    // Not all data received. Get more.
                    handler.BeginReceive(state.buffer, 0, StateObject.BufferSize, 0,
                    new AsyncCallback(ClientDataCallback), state);
                }
            }
        }
        catch (Exception ex)
        {
            CleanAndRemove();
        }
    }

    /// <summary>
    /// Not implemented
    /// </summary>
    /// <param name="handler"></param>
    /// <param name="data"></param>
    //private static void Send(Socket handler, String data)
    //{
    //    // Convert the string data to byte data using ASCII encoding.
    //    byte[] byteData = Encoding.ASCII.GetBytes(data);

    //    // Begin sending the data to the remote device.
    //    handler.BeginSend(byteData, 0, byteData.Length, 0,
    //        new AsyncCallback(SendCallback), handler);
    //}

    /// <summary>
    /// Sends data to a client based on the passed GUID to identify the client
    /// </summary>
    /// <param name="guid"></param>
    /// <param name="data"></param>
    private void SendToClient(string guid, string data)
    {
        // Convert the string data to byte data using ASCII encoding.
        byte[] byteData = Encoding.ASCII.GetBytes(data);

        foreach (StateObject so in ClientSockets)
        {
            if (so.guid == guid)
            {
                if (so.workSocket.Connected)
                {
                    // Begin sending the data to the remote device.
                    so.workSocket.BeginSend(byteData, 0, byteData.Length, 0,
                        new AsyncCallback(SendCallback), so.workSocket);
                }
            }
        }
    }

    /// <summary>
    /// Overload - send byte data to a client
    /// </summary>
    /// <param name="guid"></param>
    /// <param name="data"></param>
    private void SendToClient(string guid, byte[] data)
    {
        foreach (StateObject so in ClientSockets)
        {
            if (so.guid == guid)
            {
                if (so.workSocket.Connected)
                {
                    // Begin sending the data to the remote device.
                    so.workSocket.BeginSend(data, 0, data.Length, 0,
                        new AsyncCallback(SendCallback), so.workSocket);
                }
            }
        }
    }

    /// <summary>
    /// Callback when a send completes to a client
    /// </summary>
    /// <param name="ar"></param>
    private void SendCallback(IAsyncResult ar)
    {
        try
        {
            // Retrieve the socket from the state object.
            Socket handler = (Socket)ar.AsyncState;

            // Complete sending the data to the remote device.
            int bytesSent = handler.EndSend(ar);
            Console.WriteLine("Sent {0} bytes to client.", bytesSent);

            //handler.Shutdown(SocketShutdown.Both);
            //handler.Close();
        }
        catch (Exception e)
        {
            Console.WriteLine(e.ToString());
        }
    }

    /// <summary>
    /// Closes, disposes and removes a disconnected client from the ClientSockets arrayList
    /// </summary>
    /// <param name="state"></param>
    private void CleanAndRemoveClient(StateObject state)
    {
        lock (ClientSockets.SyncRoot)
        {
            for (int i = ClientSockets.Count - 1; i >= 0; i--)
            {
                StateObject so = (StateObject)ClientSockets[i];

                if (so == state)
                {
                    so.workSocket.Close();
                    so.workSocket.Dispose();
                    ClientSockets.RemoveAt(i);
                }
            }
        }
    }

    /// <summary>
    /// Closes, disposes and removes a any disconnected clients from the ClientSockets arrayList
    /// </summary>
    /// <param name="state"></param>
    private void CleanAndRemove()
    {
        lock (ClientSockets.SyncRoot)
        {
            for (int i = ClientSockets.Count - 1; i >= 0; i--)
            {
                StateObject so = (StateObject)ClientSockets[i];

                if (so.workSocket.Connected == false)
                {
                    so.workSocket.Close();
                    so.workSocket.Dispose();
                    ClientSockets.RemoveAt(i);
                }
            }
        }
    }
}
\$\endgroup\$

closed as off-topic by RubberDuck, nhgrif, Dan Pantry, Heslacher, Comintern Apr 2 '15 at 23:05

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 4
    \$\begingroup\$ I'm voting to close this question because the code is not yet complete, hence not ready to be reviewed. \$\endgroup\$ – RubberDuck Apr 2 '15 at 14:43
8
\$\begingroup\$

Name your class properly without spelling errors CAsnycSocketServer should be CAsyncSocketServer. I don't know whatfor you have prefixed the class with C , if you did it because it is a Class, then you should consider to remove it.


In the CloseSockets() method you already call Clear on the ClientSockets so the same call in the Dispose() method is superfluous.


You name all your private variables using camelCase casing, only ClientSockets differ. You should keep your style and name it also using camelCase casing.


By calling the parameterless default constructor and immediately disposing the object, your code will crash, because ClientSockets is null. Initialize ClientSockets in the parameterless constructor and call it form the other constructor to be on the safe side.


If you swallow all (or a singl type) exceptions, you should put a comment there to explain why you are swallowing them.

catch (Exception ex)
{
}  

Talking about comments...

// Convert the string data to byte data using ASCII encoding.
byte[] byteData = Encoding.ASCII.GetBytes(data);

A comment should describe why something is done. Let the code explain by itself what is done by using meaningful names for classes, methods and variables.


Dead code (commented code) should be deleted to improve readability.


foreach (StateObject so in ClientSockets)
{
    if (so.guid == guid)
    {
        if (so.workSocket.Connected)
        {
            // Begin sending the data to the remote device.
            so.workSocket.BeginSend(data, 0, data.Length, 0,
                new AsyncCallback(SendCallback), so.workSocket);
        }
    }
}  

By putting the second condition into the first if by using && you will reduce horizontal spacing and therfor improve readability like:

    foreach (StateObject so in ClientSockets)
    {
        if (so.guid == guid && so.workSocket.Connected)
        {  
            // Begin sending the data to the remote device.
            so.workSocket.BeginSend(data, 0, data.Length, 0,
                new AsyncCallback(SendCallback), so.workSocket);
        }
    }  
\$\endgroup\$
6
\$\begingroup\$

This is not exception handling.

catch (Exception ex)
{
}

And neither is this.

catch (ObjectDisposedException)
{
}
catch (SocketException se)
{
}

If you're going to catch an exception, you need to be prepared to do something with it. Handle it, re throw it, or leave a comment explaining why exactly it's okay to just swallow it.


You're shooting for high performance, right? Then why are you writing to the console?

Console.WriteLine("Server listening for connections");
  1. This involves unnecessary over head.
  2. You see these kinds of things in code that is being manually tested instead of properly unit tested.

Your comments indicate that you're using a pseudo-code driven approach. This is good, but the last (and often overlooked) step to that approach is to remove redundant comments.

// Create the state object.
StateObject state = new StateObject();
state.guid = Guid.NewGuid().ToString();

The comments you leave behind should clarify the code, not simply restate it.


if (bytesRead == 0)
{
    //...
}
if (bytesRead > 0)
{
    //...
}
  1. You're not returning, so an else if would be more future proof here.
  2. It's not obvious whether or not you considered the other possible path of bytesRead being less than zero.

Yes. Yes. I know that it shouldn't ever be less than zero, but document that in the code by throwing an exception and leaving a comment saying that it shouldn't ever occur. This way, the maintainer will know that this possible (highly unlikely, but still possible) condition was considered.

else if (bytesRead < 0)
{
    // this shouldn't ever happen
    // if it has, something has gone terribly wrong
    // integer overflow perhaps

    Throw new InvalidOperationException("Read a negative number of bytes.");
}
\$\endgroup\$
  • \$\begingroup\$ Many thanks for the constructive comments. I forgot to add that it was not completed. I had been testing the class using a console app hence the console statements and would in future add events. I also had not finished the exception handling. \$\endgroup\$ – malovern Apr 2 '15 at 14:21
  • 2
    \$\begingroup\$ If your code is not completed, it is not ready to be reviewed and you have effectively wasted 3 people's time. Think of Code Review as what happens when you submit a Pull Request to an open source project. You feel like you're done with it, so now it's time for a second set of eyes on it to tell you whether or not you're actually done with it. \$\endgroup\$ – RubberDuck Apr 2 '15 at 14:41
  • \$\begingroup\$ I disagree, peer reviewing at an early stage can catch problems far before they evolve. Such as suggestions, for example, to use different methods for what I am trying to achieve. If you feel this is wasting your time, please feel free not to comment further. \$\endgroup\$ – malovern Apr 2 '15 at 14:52
  • 3
    \$\begingroup\$ Code that is not completed and/or is not working is off-topic for code review. This is not really a point of debate. Your question is, as it stands, off topic, for the exact reason you just illustrated- RubberDuck wrote a review for you and then you turned around and said "oh actually I've already done that bit it's just not added in the original code". This is why code needs to be as complete as possible before we review. \$\endgroup\$ – Dan Pantry Apr 2 '15 at 14:55
  • 2
    \$\begingroup\$ "...peer reviewing at an early stage can catch problems far before they evolve." I agree, that's true, but that is not called "code review". that's called team programming, or extreme programming, or whiteboarding. The Code Review site is not the place to discuss which protocols, designs, procedures should be used, but to review whether the intended protocols, designs and procedures have been implemented well. It seems your code is still in the design/programming stage, not the review stage. \$\endgroup\$ – rolfl Apr 2 '15 at 17:12

Not the answer you're looking for? Browse other questions tagged or ask your own question.