5
\$\begingroup\$

I'm writing this game server for my small 2D MMO game.

So here are my questions:

  1. What do you think about the Thread-Safety of the code?
  2. Please explain how can it be made thread-safe / show example/ fix the thing if its not thread safe already.
using System;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading;
using System.Collections.Generic;
using System.Data;
using System.Data.SqlClient;
using System.Data.SqlTypes;

public class Constanti
{
    public const int CLASS_DARKELF_MAGICIAN = 1;
    public const int CLASS_HUMAN_MAGICIAN   = 2;
    public const int CLASS_WARRIOR          = 3;
    public const int CLASS_MODERN_GUNMAN    = 4;
    public const int SUIT_1 = 1;
    public const int SUIT_2 = 2;
    public const int SUIT_3 = 3;
    public const int SUIT_4 = 4;
    public const int SUIT_Admin = 5;

    //MAX/MIN
    public const int MAX_LEVEL = 100;
    public const int MAX_SKILL_LEVEL = 1000;

    //SERVER MAX/MIN
    public const int MAX_CONNECTIONS = 300;
    public const int MAX_CONNECTIONS_IP = 54;
}

// State object for reading client data asynchronously
public class Player
{
    // 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();
    //Player-Info
    public int PlayerStats_Health = 0;
    public int PlayerStats_Energy = 0;
    public int PlayerInfo_Class = 0;
    public int PlayerInfo_Suit = 0;
    public int PlayerInfo_Level = 0;
    public int PlayerInfo_SkillLevel = 0;

    public void SetDefaults()
    {
        PlayerStats_Health = 100;
        PlayerStats_Energy  = 200;
        PlayerInfo_Class = Constanti.CLASS_DARKELF_MAGICIAN;
        PlayerInfo_Suit = Constanti.SUIT_1;
        PlayerInfo_Level = 1;
        PlayerInfo_SkillLevel = 1;
    }

    public Player()
    {
    }

    public String pIPAddress;
}

public class GameObjectLists
{
    public static List<Player> PlayersList = new List<Player>();
}

public class AsynchronousSocketListener
{
    // Thread signal.
    public static ManualResetEvent allDone = new ManualResetEvent(false);
    public static int PlayersOnline = 0;
    public AsynchronousSocketListener()
    {}

    public static void InitializeMySQL()
    {
        //TODO MySQLI/MySQL Connection
    }

    public static void MysqlUpdateQuery()
    {
        //Mysql UPDATE, no return stmt
    }

    public static String MySQLSelect()
    {
        //TODO MySQL Select
        String retdata="test";

        return retdata;
    }

    public static void StartListening()
    {
        // Data buffer for incoming data.
        byte[] bytes = new Byte[1024];
        // Establish the local endpoint for the socket.
        // The DNS name of the computer
        /*
        IPHostEntry ipHostInfo = Dns.Resolve(Dns.GetHostName());
        IPAddress ipAddress = ipHostInfo.AddressList[0];*/

        IPEndPoint localEndPoint = new IPEndPoint(IPAddress.Any, 86);

        // Create a TCP/IP socket.
        Socket listener = new Socket(AddressFamily.InterNetwork,SocketType.Stream, ProtocolType.Tcp);
        listener.Blocking = false;
        // Bind the socket to the local endpoint and listen for incoming connections.
        try
        {
            listener.Bind(localEndPoint);
            listener.Listen(50);
            Console.WriteLine("Server Started, waiting for connections...");

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

                // Start an asynchronous socket to listen for connections.
                // is there DOS vulnerability here ?

                listener.BeginAccept(
                    new AsyncCallback(AcceptCallback),
                    listener);

                // Wait until a connection is made before continuing.
                allDone.WaitOne();
            }
        }
        catch (Exception e)
        {
            Console.WriteLine(e.ToString());
        }

        //Console.WriteLine("\nPress ENTER to continue...");
        Console.Read();
    }


    public static void AcceptCallback(IAsyncResult ar)
    {

        // Get the socket that handles the client request.
        Socket listener     = (Socket)ar.AsyncState;
        Socket clientsocket = listener.EndAccept(ar);
        // Signal the main thread to continue.
        allDone.Set();
        clientsocket.Blocking = false;      //set to non-blocking
        // Create the state object.
        Player PlayerInfo = new Player();
        PlayerInfo.workSocket = clientsocket;

        IPEndPoint thisIpEndPoint = PlayerInfo.workSocket.RemoteEndPoint as IPEndPoint; //Get Local Ip Address
        PlayerInfo.pIPAddress = thisIpEndPoint.Address.ToString();

        GameObjectLists.PlayersList.Add(PlayerInfo);
        PlayersOnline++;

        int numconnsofip = 0;
        GameObjectLists.PlayersList.ForEach(delegate(Player PlayerInfoCheck)
        {
                //Console.WriteLine(name);
                if (PlayerInfoCheck.pIPAddress == PlayerInfo.pIPAddress)
                {
                    numconnsofip++;
                }
        });

        if (PlayersOnline > Constanti.MAX_CONNECTIONS || numconnsofip > Constanti.MAX_CONNECTIONS_IP)
        {
            Disconnect(clientsocket, PlayerInfo);
        }
        else
        {
            Console.WriteLine("Player with IP:[{0}] has [{1}] Connections", thisIpEndPoint.Address.ToString(), numconnsofip);
            PlayerInfo.SetDefaults();
            //clientsocket.LingerState = new LingerOption(true, 2);    // give it up to 2 seconds for send
            Console.WriteLine("New Connection Total:[{0}]", PlayersOnline);
            clientsocket.BeginReceive(PlayerInfo.buffer, 0, Player.BufferSize, 0, new AsyncCallback(ReadCallback),
                PlayerInfo);
        }
    }

    public static void ProtocolCore(Player PlayerInfo, String data)
    {
        Console.WriteLine("Procesing Packet:{0}",data);
        //if data == bla bla then send something to everyone:

        //Is this thread-safe?

        GameObjectLists.PlayersList.ForEach(delegate(Player ObjPlayerInfo)
        {
            ObjPlayerInfo.PlayerStats_Health += 1;  //thread safe? how?
            Send(data,ObjPlayerInfo);
        });
    }

    public static void ReadCallback(IAsyncResult ar)
    {
        // TEST #1 - IF WE HANG HERE, THERE WILL BE STILL OTHER CONNECTIONS COMING HERE, BUT NO MULTI THREADING?? 
        // Retrieve the state object and the clientsocket socket
        // from the asynchronous state object.
        Player PlayerInfo = (Player)ar.AsyncState;
        Socket clientsocket = PlayerInfo.workSocket;
        try
        {
            String content = String.Empty;  //content buffer

            // Read data from the client socket. 
            // IF THIS FAILS, WE CATCH / ASSUMING THAT:
            // THE CLIENT FORCE-CLOSED THE CONNECTION OR OTHER REASON.
            int bytesRead = clientsocket.EndReceive(ar);    

            if (bytesRead > 0)
            {
                // There  might be more data, so store the data received so far.

                PlayerInfo.sb.Append(Encoding.ASCII.GetString(
                    PlayerInfo.buffer, 0, bytesRead));

                // Check for end-of-file tag. If it is not there, read 
                // more data.
                content = PlayerInfo.sb.ToString();
                int eofindex = content.IndexOf("<EOF>");
                if (eofindex > -1)
                {
                    // All the data has been read from the 
                    // client. Display it on the console.
                    content = content.Substring(0,eofindex);  //remove THE <EOF>

                    Console.WriteLine("Read {0} bytes from socket. Data : {1}",content.Length, content);

                    //PROCESS THE PACKET/DATA (PROTOCOL CORE)
                    ProtocolCore(PlayerInfo, content);

                    //Echo the data back to the client.
                    Send(content, PlayerInfo);
                    // CLEAR THE BUFFERS
                    PlayerInfo.sb.Remove(0, PlayerInfo.sb.Length);
                    Array.Clear(PlayerInfo.buffer, 0, PlayerInfo.buffer.Length);

                    // GO TO LISTEN FOR NEW DATA
                    clientsocket.BeginReceive(PlayerInfo.buffer, 0, Player.BufferSize, 0,
                    new AsyncCallback(ReadCallback), PlayerInfo);
                }
                else
                {
                    // Not all data received. Get more.
                    clientsocket.BeginReceive(PlayerInfo.buffer, 0, Player.BufferSize, 0,
                    new AsyncCallback(ReadCallback), PlayerInfo);
                }
            }
            else
            {
                //ASSUMING WE RECEIVED 0 SIZED PACKET or CLIENT DISCONNECT / THEREFORE CLOSE THE CONNECTION
                Disconnect(clientsocket, PlayerInfo);
            }
        }
        catch (Exception e)
        {
            Console.WriteLine(e.ToString());
            Disconnect(clientsocket, PlayerInfo);
        }
    }

    private static void Send(String data,Player PlayerInfo)
    {
        // Convert the string data to byte data using ASCII encoding.
        byte[] byteData = Encoding.ASCII.GetBytes(data);
        try
        {
            // Begin sending the data to the remote device.
            PlayerInfo.workSocket.BeginSend(byteData, 0, byteData.Length, 0,
                new AsyncCallback(SendCallback), PlayerInfo);
        }
        catch (Exception e)
        {
            Console.WriteLine(e.ToString());
            Disconnect(PlayerInfo.workSocket, PlayerInfo);
        }
    }

    private static void Disconnect(Socket clientsocket, Player PlayerInfo)
    {

        try
        {
            PlayersOnline--; //Is this Thread-Safe also?
            Console.WriteLine("Socket Disconnected, PlayerObjects:[{0}]", GameObjectLists.PlayersList.Count);
            GameObjectLists.PlayersList.Remove(PlayerInfo);
            clientsocket.Shutdown(SocketShutdown.Both);
            clientsocket.Close();
        }
        catch (Exception e)
        {
           Console.WriteLine(e.ToString());
        }
    }

    private static void SendCallback(IAsyncResult ar)
    {
        // Retrieve the socket from the state object.
        Player PlayerInfo = (Player)ar.AsyncState;
        Socket clientsocket = PlayerInfo.workSocket;
        try
        {
            // Complete sending the data to the remote device.
            int bytesSent = clientsocket.EndSend(ar);
            Console.WriteLine("Sent {0} bytes to client.", bytesSent);
        }
        catch (Exception e)
        {
            Console.WriteLine(e.ToString());
            Disconnect(clientsocket, PlayerInfo);
        }
    }


    public static int Main(String[] args)
    {
        InitializeMySQL();
        StartListening();
        return 0;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Have you tested your code? Does it seem to work? \$\endgroup\$
    – svick
    Commented Jun 30, 2013 at 11:45
  • \$\begingroup\$ Hmmm... You should restrict code to those specific sections you have questions about. For instance, Contanti probably isn't related to your threading question (you've put too many unrelated things inside it, anyways - learn to use Enums). I seriously doubt that your sockets should know about your DB, period. Stuff like ObjPlayerInfo.PlayerStats_Health += 1; is threadsafe unless there are explicit serialization guards - which you seem to lack. You shouldn't be outputting to Console directly - inject the relevant output stream. \$\endgroup\$ Commented Jul 1, 2013 at 20:29
  • \$\begingroup\$ "small" "MMO" pick one :p \$\endgroup\$
    – Dan
    Commented Sep 10, 2014 at 20:04

2 Answers 2

5
\$\begingroup\$

Couple of things

  1. Magic Number #1

    // Data buffer for incoming data.
    byte[] bytes = new Byte[1024];
    

    This could be something like

    const int BUFFER_SIZE = 1024;
    byte[] bytes = new byte[BUFFER_SIZE];
    

    You already have something like this set up in the Player Class

    upon further inspection we see that this byte array isn't even used in the containing method, we should just get rid of it.

  2. Magic Number #2

    listener.Listen(50);
    

    you should call this number something like

    const int LISTEN_TIME = 50;
    

    and call the listen method like this

    listener.Listen(LISTEN_TIME);
    
  3. In the StartListening method's catch statement you probably want to dispose the socket so that it isn't bound up the next time you call the method.

  4. Naming for variables should be camelCase not PascalCase You have some that are PascalCase

    Player ObjPlayerInfo
    

    Some that are all lowercase

    Socket clientsocket = PlayerInfo.workSocket;
    

    Be Consistent

  5. Return something more meaningful in your exceptions, these probably shouldn't be shown to the user, you could also log these errors as well.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ To add to this, don't explicitly declare empty default constructors. \$\endgroup\$
    – Dan
    Commented Sep 10, 2014 at 20:04
3
\$\begingroup\$

There's a big issue with using BeginSend/EndSend.

Specifically that this combination does not guarantee the entire packet gets sent.

Most operating system, including Windows, implement the Nagle algorithm, where if many small packets are sent in a pretty short period of time, as many as possible will get stuffed in before the TCP packet is actually sent.

If there's not enough room for the entire packet in the TCP buffer currently getting filled, it will cut the packet off to fit, and will unblock and return the number of bytes actually sent.

The callback needs to check for this, and do a second BeginSend to send the rest of it if it happens.

Also, sometimes the request ends up being synchronous anyway, because reasons. In this case, CompletedSynchronously will be sent true in the ASyncResult. This may need to be tested for as well.

Multithreading tends to increase the odds of this happening, so this may give the impression that there is a thread safety issue. But that's not the actual problem.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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