TCP Server NetworkModel for MUD

Started by
16 comments, last by hplus0603 11 years, 1 month ago

So, after taking the advice of the forum some 6 months ago to start my game-making career off with a simple text adventure, I am now neck-deep in a (somewhat) working MUD. What can I say? Go big or go home. Up until a few days ago, the networking portion of my MUD was working beautifully, but as I started sending more and more messages between client and server, I began noticing some odd behavior. Namely, my poorly implemented NetworkModel class had no way to deal with message overlap. I have since attempted to remedy this problem, as I will show you shortly.

First, let me explain my strategy. I'm sending strings separated with ` and ~ to denote the various parts of the message. These strings are then split, routed, and examined by the engines on both sides to generate an effect. Pretty standard. When a client connects to my server, I store the open connection in two dictionaries - one by characterID, the other by acccountID. Both are filled outside of the class by the Game Manager class. Requests are added to a list that the Game Manager pulls from, executing the top-most request then discarding it. Both dictionaries and the list are custom thread-safe class I've designed (cuz I'm using VS 2008).

Alright, code avalanche incoming.


public class ClientInfo
    {
        public Socket WorkSocket = null;        
        public byte[] Buffer;
        public byte[] BackBuffer;
        public string RecIncom = "";

        public ClientInfo(Socket client, int buffersize)
        {
            WorkSocket = client;
            Buffer = new byte[buffersize];
            BackBuffer = new byte[buffersize];
        }
    }
    
    public class NetworkModel
    {
        public const int BUFFER_SIZE = 1024;
        private const string HEADER = "``";
        private const string FOOTER = "~~";
        
        #region PROPERTIES

        private Socket Server;        
        public SafeDictionary<string, Socket> ConnectionsbyCharacter;
        public SafeDictionary<string, Socket> ConnectionsbyAccount;        

        public SafeList<RequestModel> RequestQueue;

        #endregion PROPERTIES

        #region CONSTRUCTORS

        public NetworkModel(int port, SafeList<RequestModel> requests)
        {
            ConnectionsbyAccount = new SafeDictionary<string, Socket>();
            ConnectionsbyCharacter = new SafeDictionary<string, Socket>();
            RequestQueue = requests;
            
            Server = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            IPEndPoint EndPoint = new IPEndPoint(IPAddress.Any, port);
            Server.Bind(EndPoint);
        }

        #endregion CONSTRUCTORS

        #region METHODS
        
        public void Begin()
        {
            Server.Listen(20);
            Server.BeginAccept(new AsyncCallback(Accept), null);
        }
        
        private void Accept(IAsyncResult Result)
        {
            ClientInfo newClient = new ClientInfo(Server.EndAccept(Result), BUFFER_SIZE);
            Server.BeginAccept(new AsyncCallback(Accept), null);            
            newClient.WorkSocket.BeginReceive(newClient.Buffer, 0, 1024, SocketFlags.None,
               new AsyncCallback(ReceiveMessage), newClient);
        }

        public void Send(Socket client, string ToSend)
        {           
            byte[] Message = Encoding.ASCII.GetBytes(HEADER + ToSend + FOOTER);
            client.BeginSend(Message, 0, Message.Length, SocketFlags.None,
                    new AsyncCallback(SendMessage), client);
        }

        public void Send(string ID, string ToSend)
        {            
           Socket Client = ConnectionsbyCharacter[ID];
           byte[] Message = Encoding.ASCII.GetBytes(HEADER + ToSend + FOOTER);
           Client.BeginSend(Message, 0, Message.Length, SocketFlags.None,
                   new AsyncCallback(SendMessage), Client);            
        }

        public void SendToAllConnected(string ToSend)
        {
            foreach (Socket client in ConnectionsbyCharacter)
            {
                Send(client, ToSend);
            }
        }

        private void SendMessage(IAsyncResult Result)
        {
            Socket Client = (Socket)Result.AsyncState;
            int AmountSent = Client.EndSend(Result);
            //Look into comparing amount sent vs. size of original message?          
        }

        private void ReceiveMessage(IAsyncResult Result)
        {
            ClientInfo thisClient = (ClientInfo)Result.AsyncState;
            
            string Received = "";
            try
            {
                int AmountReceived = thisClient.WorkSocket.EndReceive(Result);

                //If we've received any sort of message...
                if (AmountReceived > 0)
                {
                    //Translate the message and determine begin points and end points
                    string Contents = Encoding.ASCII.GetString(thisClient.Buffer, 0, AmountReceived);
                    int HeadLoc = Contents.IndexOf(HEADER);
                    int TermLoc = Contents.IndexOf(FOOTER);

                    //If we have a begin point...
                    if (HeadLoc > -1)
                    {
                        //...and an end point, and begin comes first...
                        if (TermLoc > -1 & HeadLoc < TermLoc)
                        {
                            //...we've received one whole message!
                            Received = Contents.Substring(HeadLoc, TermLoc - HeadLoc + 1);

                            //Now we need to check if there is another message, partial or otherwise, also stored in the buffer
                            if (TermLoc != 1022 & thisClient.Buffer[TermLoc + 2] != null)
                            {
                                //If so, copy the unused buffer portion to temp storage while we clear the main buffer, then
                                //copy back so we can begin receiving it again
                                int Len = thisClient.Buffer.Length - TermLoc - 1;
                                Array.Copy(thisClient.Buffer, TermLoc + 2, thisClient.BackBuffer, 0, Len);
                                thisClient.Buffer = new byte[BUFFER_SIZE];
                                Array.Copy(thisClient.BackBuffer, thisClient.Buffer, Len);
                                thisClient.BackBuffer = new byte[BUFFER_SIZE];
                                thisClient.WorkSocket.BeginReceive(thisClient.Buffer, 0, 1024, SocketFlags.None,
                                    new AsyncCallback(ReceiveMessage), thisClient);
                            }
                            else
                            {
                                //Otherwise, just clear the main buffer
                                thisClient.Buffer = new byte[BUFFER_SIZE];
                            }
                        }
                        //...and an end point, but end comes first...
                        else if (TermLoc > -1 & TermLoc < HeadLoc)
                        {
                            //This means we are looking at the end of a previous message, so get the last bit of the last message
                            //and add it to the previous.
                            thisClient.RecIncom += Contents.Substring(0, TermLoc + 2);
                            Received = thisClient.RecIncom;
                            thisClient.RecIncom = "";
                            //Now we need to move the next message to a temp buffer to clear the main buffer, then copy it all back
                            //and start receiving again
                            int Len = thisClient.Buffer.Length - TermLoc - 1;
                            Array.Copy(thisClient.Buffer, TermLoc + 2, thisClient.BackBuffer, 0, Len);
                            thisClient.Buffer = new byte[BUFFER_SIZE];
                            Array.Copy(thisClient.BackBuffer, thisClient.Buffer, Len);
                            thisClient.BackBuffer = new byte[BUFFER_SIZE];
                            thisClient.WorkSocket.BeginReceive(thisClient.Buffer, 0, 1024, SocketFlags.None,
                                    new AsyncCallback(ReceiveMessage), thisClient);
                        }
                        //...but no end point
                        else
                        {
                            //This means we have a partial message.  This partial goes into a holding string while we 
                            //clear our buffer and start receiving the rest of the message
                            thisClient.RecIncom = Contents.Substring(0, AmountReceived);
                            thisClient.Buffer = new byte[BUFFER_SIZE];
                            thisClient.WorkSocket.BeginReceive(thisClient.Buffer, 0, 1024, SocketFlags.None,
                                    new AsyncCallback(ReceiveMessage), thisClient);
                        }
                    }
                    //If we DON'T have a begin point...
                    else
                    {
                        //...but we DO have an end point
                        if (TermLoc > -1)
                        {
                            //This is the rest of a partial message, so we just add the two together and clear our buffer!
                            Received = thisClient.RecIncom + Contents.Substring(0, TermLoc + 1);
                            thisClient.RecIncom = "";
                            thisClient.Buffer = new byte[BUFFER_SIZE];                            
                        }
                        //...and we don't have an end point
                        else
                        {
                            //This is more of a partial message, so just add it, clear the buffer and start receiving again
                            thisClient.RecIncom += Contents;
                            thisClient.Buffer = new byte[BUFFER_SIZE];
                            thisClient.WorkSocket.BeginReceive(thisClient.Buffer, 0, 1024, SocketFlags.None,
                                    new AsyncCallback(ReceiveMessage), thisClient);
                        }
                    }
                }

                //If a complete message was received, parse it out into RequestModel object and add it to the game loop
                if (Received != "")
                {
                    Received = Received.Substring(2, Received.Length - 2);
                    string[] Split = Received.Split('`');
                    string[] AccountSplit = Split[0].Split('~');
                    string[] MessageSplit = Split[1].Split('~');

                    //Message received format = AccountID~CharacterID`ect~ect.
                    RequestModel Request = new RequestModel();
                    Request.Client = thisClient.WorkSocket;
                    Request.AccountName = AccountSplit[0];
                    Request.CharacterID = AccountSplit[1];
                    Request.Request = MessageSplit;
                    RequestQueue.Add(Request);
                }

                //Do I need this?
                thisClient.WorkSocket.BeginReceive(thisClient.Buffer, 0, 1024, SocketFlags.None,
                    new AsyncCallback(ReceiveMessage), thisClient);
            }
            catch
            {
            }            
        }            

        #endregion METHODS
    }

I plan on implementing something very similar to this client-side. I THINK I have all my message overlap problems ironed out, but I would like some advice from those with more experience. I couldn't find much more than theory when it came to solving message overlap, so I had to pretty much make this up as I went. Is there anywhere I can shore this up? Will it cause me problems in the future? Is it going to be too slow? Thanks in advance!

Advertisement

Traditionally MUDs just send a stream of text in each direction. The server delimits messages from the client based on line feeds, but the client just displays all the text from the server when it gets it.

If you need to have a message-based system where each message is an atomic package of information, then there are basically 2 choices:

  • Length-prefixed messages. The first few bytes, or some bytes in the header, tell the recipient how many bytes are in the message. The recipient then knows how much more it needs to read before trying to handle the message. Pros: easy to read messages. Cons: it can be tricky to know how long the message is until you've written it, which usually makes writing the length near the start a bit fiddly. Also you have a small degree of overhead for this length parameter each time (although this is negligible).
  • Delimiter-suffixed messages. The message is presumed to continue until the correct delimiter is found. Pros: very easy to write messages, as long as you never use the delimiter in the message. Cons: if you do use the delimiter, you have to think about how you would send that, eg. escaping it. And if you have an escape character, how will you send that? Then you need the parser at the other end to be able to handle it all. And what happens if a player includes the delimiters in their normal text (perhaps maliciously)?

More specific comments on your code:

  • 1024 bytes is a TINY size for a buffer in 2013. I might have it that small during debugging for test purposes but really it should be many times larger than that.
  • ASCII encoding is going to choke as soon as anybody uses a letter or character outside of the basic 127. This is a correctness risk at best and a security risk at worst. Currently your receive function will swallow the error silently but bear this in mind for the future.
  • Your buffer handling is overcomplicated for the job it needs to do. All you need to do is this: (a) read all incoming data into the buffer. (b) if you have a full message in the buffer, handle it. (c) remove that message from the buffer (eg. by shuffling the rest of the data along, or by moving the new start position to be after that message). (d) repeat from (b) until you have no more complete messages waiting.

First, thanks for the swift reply. It's given me quite a bit to think about.

Second, I've chosen to prefix my messages with `` and suffix them with ~~ because these are the separators I use in the command strings I pass between client and server. On the client side, I've disabled that key on the keyboard and include checks to make sure they haven't copy/pasted those keys into the command line. So that shouldn't be a problem.

Third, the buffer size for the server need not be very large, as the clients pass very little information up to the server, mostly just a parsed sentence indicating their chosen action. And, correct me if I'm wrong, as I could have any number of clients connected at once, keeping the server's buffer size small will keep the RAM usage down. As for the client side, I agree that it would be a good idea to increase the buffer size. Since the client program isn't very big anyway, this won't really hurt it.

Fourth, you say ASCII encoding is bad, but you didn't point me toward a better alternative. Thoughts?

Last, my updated buffer handling! Only took me about 20 minutes to fix it.


private void ReceiveMessage(IAsyncResult Result)
        {
            ClientInfo thisClient = (ClientInfo)Result.AsyncState;
            
            string Received = "";
            try
            {
                int AmountReceived = thisClient.WorkSocket.EndReceive(Result);

                //If we've received any sort of message...
                if (AmountReceived > 0)
                {
                    //Translate the message and determine begin points and end points
                    string Contents = Encoding.ASCII.GetString(thisClient.Buffer, 0, AmountReceived);
                    thisClient.Buffer = new byte[BUFFER_SIZE];

                    int HeadLoc = 0;
                    int TermLoc = 0;

                    while (true)
                    {
                        HeadLoc = Contents.IndexOf(HEADER);
                        TermLoc = Contents.IndexOf(FOOTER);

                        //If we have a begin point...
                        if (HeadLoc > -1)
                        {
                            //...and an end point, and begin comes first...
                            if (TermLoc > -1 & HeadLoc < TermLoc)
                            {
                                //...we've received one whole message!
                                Received = Contents.Substring(0, TermLoc + 2);
                                Contents = Contents.Remove(0, TermLoc + 2);
                            }
                            //...and an end point, but end comes first...
                            else if (TermLoc > -1 & TermLoc < HeadLoc)
                            {
                                Received = thisClient.RecIncom + Contents.Substring(0, TermLoc + 2);
                                Contents = Contents.Remove(0, TermLoc + 2);
                            }
                            //...but no end point...
                            else
                            {
                                //This is a partial message.  Load into ReceivedIncomplete and break the loop to refill the buffer.
                                thisClient.RecIncom = Contents;
                                break;
                            }
                        }
                        //If we DON'T have a begin point...
                        else
                        {
                            //...but we DO have an end point
                            if (TermLoc > -1)
                            {
                                //This is the rest of a partial message, so we just add the two together!
                                Received = thisClient.RecIncom + Contents.Substring(0, TermLoc + 1);
                                Contents = Contents.Substring(0, TermLoc + 1);
                            }
                            //...and we don't have an end point
                            else
                            {
                                //This is more of a partial message, so just add it and start receiving again
                                thisClient.RecIncom += Contents;
                                break;
                            }
                        }

                        //If a complete message was received, parse it out into RequestModel object and add it to the game loop
                        if (Received != "")
                        {
                            Received = Received.Substring(2, Received.Length - 2);
                            string[] Split = Received.Split('`');
                            string[] AccountSplit = Split[0].Split('~');
                            string[] MessageSplit = Split[1].Split('~');

                            //Message received format = AccountID~CharacterID`ect~ect.
                            RequestModel Request = new RequestModel();
                            Request.Client = thisClient.WorkSocket;
                            Request.AccountName = AccountSplit[0];
                            Request.CharacterID = AccountSplit[1];
                            Request.Request = MessageSplit;
                            RequestQueue.Add(Request);
                        }

                    }

                    thisClient.WorkSocket.BeginReceive(thisClient.Buffer, 0, BUFFER_SIZE, SocketFlags.None,
                    new AsyncCallback(ReceiveMessage), thisClient);
                }
            }
            catch
            {
            }            
        }            

Thanks again!

Buffer size: the size of the buffer limits the amount of data you can read in a single call, which means it's less efficient to read all your data, which can hurt your throughput and lead to the socket buffer overflowing and the connection getting dropped. Although hopefully with a MUD the incoming data will be small anyway, so it doesn't matter so much. If you ever switch to a more real-time game, you'll need something significantly bigger.

ASCII encoding: there is no true answer here. If you're sure your client will only ever send ASCII, then you can continue to decode the bytes as ASCII on the server. (Although I would kick off a client which raised an exception there, as an error signals either a bug or a hacker.) The important thing is that both sides agree on the encoding, and that you handle other encodings safely. Traditionally MUDs never handled encoding at all, so the data stayed in byte form throughout. No risk of encoding problems, but it does mean that different people might see different things for the same stream of bytes.

New buffer handling: no idea what it's doing there. It's still overcomplicated. You don't need a separate place to store incomplete data as well as the buffer. As I said above, it should literally be a case of first adding on the incoming data to the buffer, then extracting full messages from the buffer until you run out of them. You can do it with about half as much code (although your decision to have both headers and footers when only one of the two is necessary complicates matters).

Hmmm...okay, maybe I don't exactly understand how a buffer works then. Here's how I THINK it is working...

Program calls ReceiveMessage, where it then waits for an incoming message. Once a message is detected, it adds this message into the buffer, then continues through the ReceiveMessage method. The buffer is stored as a stream of bytes, bytes which represent an encoded string. The bytes themselves do nothing for me. What I'm wanting is the string that they represent.

So I decode this message. Here's where I think I'm confused. I don't know if this is the whole message or not. I'm thinking that if the message is longer than my buffer, I only get the first part of the message, hence why I decode this part and stored it in a decoded buffer. Then, I ReceivedMessage again to get the rest of the message. Is this not the case? If part of a message doesn't end up in the buffer the first go around, does it just disappear? If that's the case, I understand now why I would need to have a bigger buffer, and I wouldn't then need to check for partial messages, reducing the complexity of my method. Is this correct?

The reason I've included headers and footers goes back to the above paragraph, and that many of the examples I've seen floating around the interwebs includes both. But...if I understand what you are telling me, I can see how it would only be necessary to have a footer only.

I've adjusted my code to reflect what I think you are saying, and you're right, it's much shorter:


private void ReceiveMessage(IAsyncResult Result)
        {
            ClientInfo thisClient = (ClientInfo)Result.AsyncState;
            
            string Received = "";
            try
            {
                int AmountReceived = thisClient.WorkSocket.EndReceive(Result);

                //If we've received any sort of message...
                if (AmountReceived > 0)
                {
                    //Translate the message
                    string Contents = Encoding.UTF8.GetString(thisClient.Buffer, 0, AmountReceived);
                    
                    //Clear buffer and begin receiving again
                    thisClient.Buffer = new byte[BUFFER_SIZE];  //Changed to 4096
                    thisClient.WorkSocket.BeginReceive(thisClient.Buffer, 0, BUFFER_SIZE, SocketFlags.None,
                    new AsyncCallback(ReceiveMessage), thisClient);

                    int TermLoc = 0;

                    while (true)
                    {                     
                        TermLoc = Contents.IndexOf(FOOTER);
                        
                        if (TermLoc == -1)
                        {
                            break;
                        }

                        //Get the message
                        Received = Contents.Substring(0, TermLoc);

                        string[] Split = Received.Split('`');
                        string[] AccountSplit = Split[0].Split('~');
                        string[] MessageSplit = Split[1].Split('~');

                        //Message received format = AccountID~CharacterID`ect~ect.
                        RequestModel Request = new RequestModel();
                        Request.Client = thisClient.WorkSocket;
                        Request.AccountName = AccountSplit[0];
                        Request.CharacterID = AccountSplit[1];
                        Request.Request = MessageSplit;
                        RequestQueue.Add(Request);

                        //Removed received message and look for more
                        Contents = Contents.Substring(TermLoc + 2, Contents.Length - (TermLoc + 2));
                    }
                }
            }
            catch
            {
            }            
        }    

EDIT - Well, I've tested everything, and it seems to be working just perfectly. I give it a month before I start noticing more odd behavior, lol.

Part of your problem is that you're talking about a Message in an ambiguous way. Your ReceiveMessage doesn't receive a message. It receives data. That data could be a message. It could also be the start of a message. It could be the end of a previous message. It could be an end followed by a start. It could be both of those things with a complete message in between - or more than one! It can scan the data and make an educated guess as to which of these many scenarios it could be, and it can look at what arrived previously and hasn't been handled yet in order to try and process a new message or messages, but there is no need for this bit of code to perform such a complex decision.

Your new code is better-looking but still broken. Imagine your first call to ReceiveMessage() gets 1000 bytes of a 2000 byte message. I presume you read it directly into thisClient.Buffer, then encode it into Contents and empty the buffer. You look for a footer, don't find it yet. Then you leave the ReceiveMessage function and boom, everything in Contents is now destroyed. The next call to ReceiveMessage might get the last half of the message, but you've already lost the first half!

The buffer must persist outside of the function that receives data because its purpose is to gather together data that arrives in multiple receive calls. It's not there just for individual messages. The buffer should be a large storage space designed to fit many messages in. It's basically a queue for bytes, with the network pushing them in at one end and your application pulling them off in message-sided chunks at the other end.

You're trying to mix network reads with data buffering and message decoding and parsing which is a nightmare. It should be a simple pipeline passing data along, performing one simple step each way. Let me reiterate:

The receive callback should do precisely 2 things, no more, no less:

  1. Take data from the network and add it to the end of your incoming data buffer. It doesn't matter whether it's 1 byte, or a million. It doesn't matter whether it's a whole message, part of one, multiples, whatever. This part of your program does not know or care about messages. Just add it to the buffer.
  2. Tell your program that there is more data in the buffer and that it should look for completed messages.

Now you know you have a bunch of bytes in the buffer. Your program has a CollectMessages() function or something similarly named. This function has nothing to do with the network at all, and just exists to pull messages out of bytes. It does this:

  1. Read from the start of the buffer onwards to see if a complete message has been formed yet.
  2. If a complete message is found:
    1. create the relevant message object and hand it to the app for processing (eg. a HandleMessage() function)
    2. remove the data for that message from the buffer (eg. replace the buffer array with a copy of everything from the buffer array except the data for the first message)
    3. Go back to the beginning and look for the next message.
  3. No more messages in the buffer: just stop and leave whatever is in the buffer for next time.

(Note that there are more efficient ways to implement step 2.2 than copying the whole buffer and that experienced people will cringe at this implementation. Note also that for a MUD you are not going to need a more efficient method.)

To summarise, it's a 3 - step process:

  1. One function to take bytes from the network and enqueue them in your application. This function doesn't know or care about messages.
  2. One function to examine the front of the byte queue and form messages as soon as enough bytes are there, and to remove those bytes from the queue. This function doesn't know or care about the network.
  3. One function to process a message in whatever way your app needs to. This function doesn't know or care about the network or the buffer of bytes.

The reason I've included headers and footers goes back to the above paragraph, and that many of the examples I've seen floating around the interwebs includes both. But...if I understand what you are telling me, I can see how it would only be necessary to have a footer only.

You never need delimiters at each end, just like you don't need a period at each end of a sentence. In real life, you know the first word you read or see is the start of a sentence, because by definition the first word is the start. All you need to know is when the sentence ends, so you wait for the delimiter. So it is with networking.

By having 2 delimiters, you add in the extra case of having to decide what to do after a footer and before a header - which is most likely an error condition. It's best to eliminate this possibility entirely by removing the redundant delimiter.

Whatever you've seen "floating around the interwebs" suggesting that you need headers and footers is not likely to be a reputable source, so proceed with caution there.

Whatever you've seen "floating around the interwebs" ... is not likely to be a reputable source, so proceed with caution there.

Herein lies most of my problems. NOTHING I've found ANYWHERE on the webs looks anything like what I'm working on right now. It's like there's some massive conspiracy to prevent new programmers from fully grasping how socket programming works. I'm worried for you, Kylotan. The Socket Police are reading this thread. They know what you've done. And they are NOT happy. smile.png

Okay...so I spent about half an hour staring at your post, trying to get my head around it, trying to work past all the bad or incomplete information I already had...and here's what I've come up with. It SEEMS to follow everything you've said, but I'm sure you'll see something I don't. I think, by the time I'm done, I'll have myself an Enterprise-level network protocol at my disposal lol.


private void ReceiveMessage(IAsyncResult Result)
        {
            ClientInfo thisClient = (ClientInfo)Result.AsyncState;
            
            int AmountReceived = thisClient.WorkSocket.EndReceive(Result);

           //If we've received any sort of message...
           if (AmountReceived > 0)
           {
               GetMessage(thisClient, AmountReceived);            
           }             
        }

        private void GetMessage(ClientInfo Client, int DataLength)
        {
            //Translate the message
            string Contents = Encoding.UTF8.GetString(Client.Buffer, 0, DataLength);

            int TermLoc = 0;

            while (true)
            {                     
                TermLoc = Contents.IndexOf(FOOTER);
                
                if (TermLoc == -1 & Contents.Length == 0)
                {
                    Client.WorkSocket.BeginReceive(Client.Buffer, 0, BUFFER_SIZE, SocketFlags.None,
                        new AsyncCallback(ReceiveMessage), Client);
                    return;
                }
                else if (TermLoc == -1 & Contents.Length > 0)
                {              
                    int StartIndex = DataLength - Contents.Length;

                    Array.Copy(Client.Buffer, StartIndex, Client.Buffer, 0,  Contents.Length);
                    Client.WorkSocket.BeginReceive(Client.Buffer, StartIndex, BUFFER_SIZE - StartIndex, SocketFlags.None,
                        new AsyncCallback(ReceiveMessage), Client);
                    return;
                }
                
                //Parse message and send it to game loop
                PassMessage(Client, Contents.Substring(0, TermLoc));

                //Removed received message and look for more
                Contents = Contents.Substring(TermLoc + 2, Contents.Length - (TermLoc + 2));
            }
        }

        private void PassMessage(ClientInfo Client, string Message)
        {
            string[] Split = Message.Split('`');
            string[] AccountSplit = Split[0].Split('~');
            string[] MessageSplit = Split[1].Split('~');

            //Message received format = AccountID~CharacterID`ect~ect.
            RequestModel Request = new RequestModel();
            Request.Client = Client.WorkSocket;
            Request.AccountName = AccountSplit[0];
            Request.CharacterID = AccountSplit[1];
            Request.Request = MessageSplit;
            RequestQueue.Add(Request);
        }

EDIT - I just wanted to reiterate how grateful I am that you are taking time out of your day to help me out with this. You've been both patient and eloquent, and I thank you.

Better! Here are the remaining bugs I can see:

  • string Contents = Encoding.UTF8.GetString(Client.Buffer, 0, DataLength); - This ignores any data that was previously in Client.Buffer. If you received 100 bytes followed by 25 bytes, you'd wrongly attempt to construct a string out of the first 25 bytes out of 125! Buffer handling shouldn't care how much data your last receive operation returned, which is what DataLength is. It shouldn't care about receive operations at all. It only cares about what's in the buffer. So that DataLength needs to be replaced with the size of the buffer.
  • GetMessage can't actually call Encoding.UTF8.GetString safely. I should have noticed this earlier, but basically it's an unsafe operation because you can't be guaranteed to always have a proper UTF-8 character at the end of your buffer, as a UTF-8 character could be several bytes long and you might not have all of it yet. 99.999% of the time it will be fine. One day it won't be, though. (I suggest a fixed width encoding like ISO-Latin-1 is safer, eg. Encoding.GetEncoding(28591).)
  • It looks like you try and keep the end of the buffer data persistent via the Array.Copy call to shuffle it down - but because you're using DataLength to mean the length of the buffer when really it means the length of the last receive operation (see point 1 above), you'll lose data here.

Here's what you should do instead, which solves these problems with your GetMessage function:

  • When you receive into the buffer, track the buffer size by adding the number of bytes received each time.
  • Don't pull anything into a Contents string or decode it; work directly with the Client.Buffer bytes. Forget the Contents string entirely.
  • You need to search the byte array itself for the footer. This is a bit trickier than looking for a substring unfortunately. (It's one reason why most people don't use delimiters like this.) Maybe do one IndexOf call to find the first character of the footer, then check directly if the next character is the end of the footer. Be aware of possible index out of bounds error here.
  • When you successfully find a whole footer, then you can use GetString on the bytes up to that point and hand that off to PassMessage.
  • Use Array.Copy to shuffle the remainder of the buffer down, much like you're doing already. Watch out for off-by-one errors here. Then decrease buffer size by the appropriate amount.
I'm wondering why message overlap is even a problem in a TCP stream? The data is always in order and guaranteed.

I'm wondering why message overlap is even a problem in a TCP stream? The data is always in order and guaranteed.


TCP data is always in order and guaranteed, this is true. TCP is, however, a stream, so what is not guaranteed is data grouping.

Let us say that I generate 5 50-byte logical packets. Each is sent during a separate frame.

I could get all five at once (a single 250 byte blob). I could get 250 one byte receives, I could get anything in between.

This is why we need some kind of framing in the logical packet itself; there is nothing in the TCP stream that does it for us because there is no guaranteed correlation between the number and size of TCP sends and the number and size of the corresponding TCP receives.

This topic is closed to new replies.

Advertisement