Send messages in TCP-based communication system

Question

I am working on a TCP-based Communication System.

I wrote a working function for sending Textmessages and remotely opening a MessageBox. I would like to know how elegant this piece of code is and how I may improve it, cause it seems messy to me.

Client:

private void btnSend_Click(object sender, EventArgs e)
{
    NetworkStream stream = client.GetStream();

    BinaryWriter w = new BinaryWriter(stream);

        byte[] msg = new byte[4096];
        msg = Encoding.ASCII.GetBytes("%" + txtMsg.Text + "&");

        w.Write(msg, 0, msg.Length);

}

private void btnMsgBox_Click(object sender, EventArgs e)
{
    NetworkStream stream = client.GetStream();

    byte[] msg = new byte[4096];          

    msg = Encoding.ASCII.GetBytes("%DisplayRemoteMessageBox&" + txtMsg.Text + "#");
    stream.Write(msg, 0, msg.Length);            
}

Server:

while(true)
{
    while(client.Connected)
    {
        byte[] msg = new byte[4096];
        var count = stream.Read(msg, 0, msg.Length);

        string returnData = Encoding.ASCII.GetString(msg, 0, count);

        string command = returnData.Substring((returnData.IndexOf("%") + 1), (returnData.IndexOf("&") -1));


        switch(command)
        { 
        case "DisplayRemoteMessageBox":
                returnData = returnData.Replace("%DisplayRemoteMessageBox", "");
                int startInd = returnData.IndexOf("&");
                int len = returnData.IndexOf("#");                            

                returnData = returnData.Substring((startInd + 1), (len - 1));
                MessageBox((IntPtr)0, returnData, "TestBox", 0);
                 break;
        case "":                            
                client.Client.Disconnect(true);                        
                Console.WriteLine("User disconnected");
                 break;
        default:
                returnData = returnData.Substring(1, (returnData.Length - 2));
                Console.WriteLine(returnData);
                 break;
        }                 
    }
}

Answer 1

Networking Concerns

I'm going to assume that stream is a NetworkStream in the Server. It is really really important that you understand that stuff which 'just happens to work' with a 'normal' stream, won't work with a NetworkStream, which also doesn't provide a 'complete' Stream implementation (being non-seeking and such it can't).

Your code currently could behave very oddly and somewhat non-deterministically depending on the interface between the server and client computers, and likely the lengths of messages.

I'm going to provide a full-enough answer here, because I can't find any that I really like on SO (I'm sure there are good ones, I just can't find them). Networked code is hard enough to write that I won't say anything about buffering and the performance implications thereof (not least because I don't really understand it myself, and have never bothered to properly profile any of my code).

Note that there are loads of examples of this deficiency all over the web (and not enough explanations of the problem, I hope mine isn't too painful).

---

Unfortunately, network programming isn't quite so easy: because TCP is stream based, you never really know when a 'message' has ended. The problem with your code is a result of the lone call to stream.Read(byte[], int, int)... and unfortunately the MSDN example has the same problem!

Essentially, this code is insufficient:

byte[] msg = new byte[4096];
var count = stream.Read(msg, 0, msg.Length);
string returnData = Encoding.ASCII.GetString(msg, 0, count);
string command = returnData.Substring((returnData.IndexOf("%") + 1), (returnData.IndexOf("&") -1));

I imagine you will never see this problem if you test the code on a single machine; however, over the internet I suspect it will become somewhat apparent with longer messages. As I said, Read can't know when the message your client has sent ends... so when should it return? The answer is... I don't know, it just does at some point: I don't know if there are any guarantees, and it's most certainly OS/Configuration dependant, so you shouldn't make any assumptions.

Currently you are just hoping that when calling "Read" you will receive a full message: you could well receive only part of a message, or even two messages (or parts of two messages, etc.). TCP ensures that you will receive the stream of bytes in the right order, or know that you have lost communications, but it has no concept of 'packets' (or messages) like UDP does, which is why TcpClient exposes a NetworkStream.

Example:

Client Sends "%DisplayRemoteMessageBox&Hello, World!#"
Server Received "%DisplayRemoteMessageB" -> print to console
Server Received "ox&Hello, World!#" -> print to console

The server can cut-up the stream any way it likes, so you have to design a protocol so that you can work out where stuff starts and ends (if necessary, which it is for you).

How exactly you want to 'fix' this depends on your system. You are using ASCII explicitly, so you could easily enough do a byte-by-byte (char-by-char) 'text' stream analysis to extract messages (e.g. look for % to start a message, and wait for # to end it). This is how some older protocols like IRC work, where they use some method to identify new-lines in communications, which defines the separation between messages. This involves looking at each byte you read, deciding if it has a special meaning, and acting on it accordingly. This can be fiddly and is easy to make inefficient.

A cleaner solution is to define a simple protocol which allows you to determine the length of a 'message' before you read it. The main advantage of this is that you no longer need to know what a message 'looks like' to read it: this means it can look like )anything_ (note that your current code doesn't allow you to send a # as part of a message, because # has meaning). This will (probably) be a binary protocol, where you define each message as a 'packet', which starts by declaring its length, and then streams the message.

On the client, you simply have to work out how long your messages are, and send the length before you send the message. On the server, you start by reading the length, and then you read that many bytes so you know that you have the full message. You could process the message as you receive it (e.g. for file transfer, you wouldn't wait until you've received all the bytes to start writing it disk), but for small messages you can just accumulate all the bytes in a buffer and process it in one.

I always use this binary 'packet' method for networking - not least because I have no desire to think about Unicode and its implications - so here is a rough sketch of how you might implement it to send individual strings (with lots of comments for in-line explanation). I'll make no guarantees that this code will work out-of-the-box.

Notice about BinaryWriter and friends

Much of what I have just described (and below implement) is facilitated by the BinaryReader and BinaryWriter classes in the BCL. However, both these classes assume/enforce little-endian byte order, which is not ideal when most of the networking world has agreed on big-endian. I present the below because I forgot that BinaryWriter was little-endian to show the concepts above explicitly, and to show a viable solution for big-endian communications (e.g. when interoping with existing big-endian protocols / variable endian protocols (I'm sure they exist))

Client Side

As suggested by BKSpurgeon, factor out the message writing in your two event handlers: the code is basically the same at the moment. What you want is a new method to send a string to the server, a very simple API indeed (which is no bad thing).

/// <summary> Sends a length-prepended (Pascal) string over the network </summary>
private void SendMessage(string message)
{
    NetworkStream networkStream = client.GetStream();

    // turn the string message into a byte[] (encode)
    byte[] messageBytes = Encoding.ASCII.GetBytes(message); // a UTF-8 encoder would be 'better', as this is the standard for network communications

    // determine length of message
    int length = messageBytes.Length;

    // convert the length into bytes using BitConverter (encode)
    byte[] lengthBytes = System.BitConverter.GetBytes(length);

    // flip the bytes if we are a little-endian system: reverse the bytes in lengthBytes to do so
    if (System.BitConverter.IsLittleEndian)
    {
        Array.Reverse(lengthBytes);
    }

    // send length
    networkStream.Write(lengthBytes, 0, lengthBytes.Length);

    // send message
    networkStream.Write(messageBytes, 0, length);
}

// example calls
private void btnSend_Click(object sender, EventArgs e)
{
    msg = "%" + txtMsg.Text + "&";
    SendMessage(msg);
}

// example calls
private void btnMsgBox_Click(object sender, EventArgs e)
{
    msg = "%DisplayRemoteMessageBox&" + txtMsg.Text + "#";
    SendMessage(msg);
}

Hopefully this is mostly pretty easy to follow. Note the 'endian flip' of the length: Endianness is a nightmare on its own, and the .NET libraries do not help much. Note the remarks for BitConverter.GetBytes(int): to be efficient, this will just be a blit, which means the result depends on the hardware you are using. This is fine if the destination has the same endianness, but with networking you can't assume that, so we agree to send big-endian, which means flipping the bytes if we are on a little-endian machine. We don't use a BinaryWriter because it is little-endian only, so it's harder to change the endianness (useful for demonstration and compatibility). This is only necessary for primitives like integers; ASCII (and UTF) are both defined the same way for a big- or little-endian system, so there is no issue with the string.

When you start writing more and more code like this, you'll end up writing dedicated methods for sending integers, strings (the protocol above is basically just sending a length-prefix 'Pascal' string), and other primitives; similarly for the server.

Server Side

on the client, we provide a method to 'send' a string. Duly, on the server, we should probably provide a message to 'receive' a string. I will sketch a very simple blocking solution. For real code, you probably won't something asynchronous, and that facilitates buffer reuse.

Reading a string requires reading the length, and then the string itself. This is two separate reads, so let's write one method to do this:

Essentially, because we can't trust NetworkStream.Read(,,) to give us the right number of bytes, we have to repeatedly call NetworkStream.Read(,,) and accumulate the bytes it gives us into a buffer. We only return the buffer to the caller when it is full (i.e. we have read exactly the requested number of bytes).

/// <summary> Reads a number of bytes from the stream </summary>
private byte[] ReadBytes(int count)
{
    NetworkStream networkStream = client.GetStream();

    byte[] bytes = new byte[count]; // buffer to fill (and later return)
    int readCount = 0; // bytes is empty at the start

    // while the buffer is not full
    while (readCount < count)
    {
        // ask for no-more than the number of bytes left to fill our byte[]
        int left = count - ReadCount; // we will ask for `left` bytes
        int r = networkStream.Read(bytes, readCount, left); // but we are given `r` bytes (`r` <= `left`)

        if (r == 0)
        { // I lied, in the default configuration, a read of 0 can be taken to indicate a lost connection
            throw new Exception("Lost Connection during read");
        }

        readCount += r; // advance by however many bytes we read
    }

    return bytes;
}

There are few ways that the code above can go wrong depending on the configuration (timeout, lost client, etc.), most of which involving a complete breakdown in communication, so the exception thrown will be a welcome one, and can be handled as appropriate. It is 'blocking', which means that it will not return until it has read count many bytes. Asynchronous versions can be written using async or simple call-backs (again, I can't find a good SO reference for this at the moment, but I'm sure I've seen one in the past). With the while loop, it looks like this code will cook your computer, but in reality NetworkStream plays 'nicely' with the TCP stack, and will block until at least the first byte is read.

Using this ReadBytes(int) method, we can write the ReadMessage method we really want:

/// <summary> Reads the next message from the stream </summary>
private string ReadMessage()
{
    // read length bytes, and flip if necessary
    byte[] lengthBytes = ReadBytes(sizeof(int)); // int is 4 bytes
    if (System.BitConverter.IsLittleEndian)
    {
        Array.Reverse(lengthBytes);
    }

    // decode length
    int length = System.BitConverter.ToInt32(lengthBytes, 0);

    // read message bytes
    byte[] messageBytes = ReadBytes(length);

    // decode the message
    string message = System.Text.Encoding.ASCII.GetString(messageBytes);

    return message;
}

Note that there is an obvious read -> decode -> consume process going on here. For anything but the most simple system, I would write a separate ReadInt method which the ReadMessage method would call.

With ReadMessage written, you can refit the server code, which becomes much tidier:

while(client.Connected)
{
    string message = ReadMessage();

    string command = message.Substring((message.IndexOf("%") + 1), (message.IndexOf("&") -1));

    switch(command)
    {
        // ...
    }
}

Note that not much has changed in the server or client: they are still using the same protocol you have set up for sending a command: all that has been added is a robust mechanism for sending and receiving a single string. Breaking the code up so that each unit of work is its own method makes it much more maintainable.

As this is really meant to be a code review, I should point out that the second argument for message.Substring(,): it is meant to be a length, so you need to subtract the start position for this to work as intended). I have also taken the liberty of renaming returnData as suggested in BKSpurgeon's answer.

I put the above into a petty test program to content myself that it compiles and isn't completely broken: example gist; it starts a simple server which just prints messages to the screen, and demonstrates simple lost connection handling (which can be jolly annoying)

More comments on BinaryWriter and friends in the context of the question

Essentially, the above SendMessage and ReadMessage methods perform a big-endian 32bit-length-prefix string transmission. If you can accept little-endian encoding, and the particulr prefix regime that they use, then you can get away with just wrapping your NetworkStream with a BinaryReader/BinaryWriter and using ReadString() and Write(String). Note that these classes use UTF-8 by default, but this is configurable in the constructors. These methods use a variable prefix length for efficiency purposes: you can see an implementation on source.dot.net. Note also that length-prefixing is invaluable when sending any discrete packet, and using these built-ins will only help if you are exclusively sending strings. If you do only need strings (e.g. if you are serialising everything as text/XML/JSON before transmitting it), and you can get way with the other constraints (e.g. if you control the protocol so can choose the endianness and prefix regime), then the following may suffice for you:

public void SendMessage(string message)
{
    NetworkStream networkStream = TcpClient.GetStream();
    using (var binaryWriter = new System.IO.BinaryWriter(networkStream, System.Text.Encoding.UTF8, leaveOpen: true))
    {
        binaryWriter.Write(message);
    }
}

public string ReadMessage()
{
    NetworkStream networkStream = TcpClient.GetStream();
    using (var binaryReader = new System.IO.BinaryReader(networkStream, System.Text.Encoding.UTF8, leaveOpen: true))
    {
        return binaryReader.ReadString();
    }
}

The parameter leaveOpen is important here, because the code creates a new BinaryReader/Writer each call, and otherwise it would close the stream when it is disposed. Ideally you would create and hold onto a single instance of each, and dispose them along with the TCPClient.

Other

This SO answer about detecting a lost TCP connection while reading may be of general interest: note that it uses a combination of BinaryRead and a ReadBytes style loop to read a length-prefixed packet for a specific purpose.

Answer 2

Use appropriate names

• what is returnData? What is msg?

Don’t overwrite the same variable:

 byte[] msg = new byte[4096];
            msg = Encoding.ASCII.GetBytes("%" + txtMsg.Text + "&");

// You could simply do this instead:
   byte[] msg = Encoding.ASCII.GetBytes("%" + txtMsg.Text + "&");

// Also you’re reusing variables and overwriting. I don’t recommend this type of thing:

  string returnData = Encoding.ASCII.GetString(msg, 0, count);
            switch(command)
            { 
            case "DisplayRemoteMessageBox":
                    returnData = returnData.Replace("%DisplayRemoteMessageBox", ""); // don’t reset the returnData variable
                    returnData = returnData.Substring((startInd + 1), (len - 1));
            case "":                            
            default:
                    returnData = returnData.Substring(1, (returnData.Length - 2));
            }     

Personally I wouldn’t change anything with your code.

But if you wanted to do something more OOP then here are some ideas. It was a nice exercise - i'll post it here otherwise it seems a waste - but I think it needlessly complicated the code. Leave your code it as it is.

internal class Program
{
    private static void Main(string[] args)
    {
    }

    #region Client Classes

    private ClientImplementation client = new ClientImplementation();
    private TextImplementation txtMsg = new TextImplementation();

    private void btnSend_Click_GiveAppropriateName1(object sender, EventArgs e)
    {
        // could use use a factory to get the writer
        Writer writer = new StandardWriter(client.GetStream(), txtMsg);
        writer.Write();
    }

    private void btnSend_ClickGiveAppropriateName2(object sender, EventArgs e)
    {
        Writer writer = new RemoteMessageBoxWriter(client.GetStream(), txtMsg);
        writer.Write();
    }

    public abstract class Writer
    {
        private BinaryWriter w;
        protected byte[] msg;
        protected TextImplementation txtMsg;

        public Writer(NetworkStream stream, TextImplementation txtMsg)
        {
            this.w = new BinaryWriter(stream);
            this.txtMsg = txtMsg;
            this.msg = GetBytes();
        }

        protected abstract byte[] GetBytes();

        public void Write()
        {
            w.Write(msg, 0, msg.Length);
        }
    }

    public class StandardWriter : Writer
    {
        public StandardWriter(NetworkStream stream, TextImplementation txtMsg)
            : base(stream, txtMsg)
        {
        }

        protected override byte[] GetBytes()
        {
            return Encoding.ASCII.GetBytes("%" + txtMsg.Text + "&");
        }
    }

    public class RemoteMessageBoxWriter : Writer
    {
        public RemoteMessageBoxWriter(NetworkStream stream, TextImplementation txtMsg)
            : base(stream, txtMsg)
        {
        }

        protected override byte[] GetBytes()
        {
            return Encoding.ASCII.GetBytes("%DisplayRemoteMessageBox&" + txtMsg.Text + "#");
        }
    }

    #endregion Client Classes

    #region Server Code

    public void Server()
    {
        while (true)
        {
            while (client.Connected)
            {
                HandleCommand command = new HandleCommandFactory().ReturnHandle(client.GetStream());
                command.DisplayData();
            }
        }
    }

    public class HandleCommandFactory
    {
        private NetworkStream stream;

        public HandleCommand ReturnHandle(NetworkStream stream)
        {
            this.stream = stream;
            string command = GetCommand(stream);

            switch (command)
            {
                case "DisplayRemoteMessageBox":
                    return new DisplayRemoteMsgBox();

                case "":
                    return new Disconnect();

                default:
                    return new Default();
            }
        }

        private string GetCommand(NetworkStream stream)
        {
            byte[] msg = new byte[4096];
            var count = this.stream.Read(msg, 0, msg.Length);
            string returnData = Encoding.ASCII.GetString(msg, 0, count);

            return returnData;
        }
    }

    abstract public class HandleCommand
    {
        protected string returnData;
        protected ClientImplementation client;

        abstract public void DisplayData();

        public HandleCommand()
        {
        }

        public HandleCommand(string returnData, ClientImplementation client)
        {
            this.returnData = returnData;
            this.client = client;
        }
    }

    public class Default : HandleCommand
    {
        public override void DisplayData()
        {
            string formattedData = returnData.Substring(1, (returnData.Length - 2));
            Console.WriteLine(formattedData);
        }
    }

    public class Disconnect : HandleCommand
    {
        public override void DisplayData()
        {
            client.Client.Disconnect(true);
            Console.WriteLine("User disconnected");
        }
    }

    public class DisplayRemoteMsgBox : HandleCommand
    {
        public override void DisplayData()
        {
            returnData = returnData.Replace("%DisplayRemoteMessageBox", "");
            int startInd = returnData.IndexOf("&");
            int len = returnData.IndexOf("#");

            string formattedData = returnData.Substring((startInd + 1), (len - 1));
            MessageBox((IntPtr)0, formattedData, "TestBox", 0);
        }

        private void MessageBox(IntPtr intPtr, string returnData, string p1, int p2)
        {
            throw new NotImplementedException();
        }
    }

    #endregion Server Code

    #region Additions just to make it compile

    public class ClientImplementation
    {
        public NetworkStream GetStream()
        {
            return new NetworkStream(GetSocket("blah", 0));
        }

        public bool Connected
        {
            get { return true; }
        }
    }

    public class TextImplementation
    {
        public string Text { get; set; }
    }

    private static Socket GetSocket(string server, int port)
    {
        Socket s = null;
        IPHostEntry hostEntry = null;

        // Get host related information.
        hostEntry = Dns.GetHostEntry(server);

        // Loop through the AddressList to obtain the supported AddressFamily. This is to avoid
        // an exception that occurs when the host IP Address is not compatible with the address family
        // (typical in the IPv6 case).
        foreach (IPAddress address in hostEntry.AddressList)
        {
            IPEndPoint ipe = new IPEndPoint(address, port);
            Socket tempSocket =
                new Socket(ipe.AddressFamily, SocketType.Stream, ProtocolType.Tcp);

            tempSocket.Connect(ipe);

            if (tempSocket.Connected)
            {
                s = tempSocket;
                break;
            }
            else
            {
                continue;
            }
        }
        return s;
    }

    #endregion Additions just to make it compile
}