Jumbled byte array after using TcpClient and TcpListener

Question

I want to use the TcpClient and TcpListener to send an mp3 file over a network. I implemented a solution of this using sockets, but there were some issues so I am investigating a new/better way to send a file.

I create a byte array which looks like this: length_of_filename|filename|file

This should then be transmitted using the above mentioned classes, yet on the server side the byte array I read is completely messed up and I'm not sure why.

The method I use to send:

 public static void Send(String filePath)
    {
        try
        {
            IPEndPoint endPoint = new IPEndPoint(Settings.IpAddress, Settings.Port + 1);
            Byte[] fileData = File.ReadAllBytes(filePath);
            FileInfo fi = new FileInfo(filePath);

            List<byte> dataToSend = new List<byte>();
            dataToSend.AddRange(BitConverter.GetBytes(Encoding.Unicode.GetByteCount(fi.Name))); // length of filename
            dataToSend.AddRange(Encoding.Unicode.GetBytes(fi.Name)); // filename
            dataToSend.AddRange(fileData); // file binary data


            using (TcpClient client = new TcpClient())
            {
                client.Connect(Settings.IpAddress, Settings.Port + 1);

                // Get a client stream for reading and writing.
                using (NetworkStream stream = client.GetStream())
                {
                    // server is ready 
                    stream.Write(dataToSend.ToArray(), 0, dataToSend.ToArray().Length);
                }
            }

        }
        catch (ArgumentNullException e)
        {
            Debug.WriteLine(e);
        }
        catch (SocketException e)
        {
            Debug.WriteLine(e);
        }
    }
}

Then on the server side it looks as follows:

    private void Listen()
    {
        TcpListener server = null;
        try
        {
            // Setup the TcpListener
            Int32 port = Settings.Port + 1;
            IPAddress localAddr = IPAddress.Parse("127.0.0.1");

            // TcpListener server = new TcpListener(port);
            server = new TcpListener(localAddr, port);

            // Start listening for client requests.
            server.Start();

            // Buffer for reading data
            Byte[] bytes = new Byte[1024];
            List<byte> data;

            // Enter the listening loop.
            while (true)
            {
                Debug.WriteLine("Waiting for a connection... ");
                string filePath = string.Empty;

                // Perform a blocking call to accept requests.
                // You could also user server.AcceptSocket() here.
                using (TcpClient client = server.AcceptTcpClient())
                {
                    Debug.WriteLine("Connected to client!");
                    data = new List<byte>();

                    // Get a stream object for reading and writing
                    using (NetworkStream stream = client.GetStream())
                    {
                        // Loop to receive all the data sent by the client.
                        while ((stream.Read(bytes, 0, bytes.Length)) != 0)
                        {
                            data.AddRange(bytes);
                        }
                    }
                }

                int fileNameLength = BitConverter.ToInt32(data.ToArray(), 0);
                filePath = Encoding.Unicode.GetString(data.ToArray(), 4, fileNameLength);
                var binary = data.GetRange(4 + fileNameLength, data.Count - 4 - fileNameLength);

                Debug.WriteLine("File successfully downloaded!");

                // write it to disk
                using (BinaryWriter writer = new BinaryWriter(File.Open(filePath, FileMode.Append)))
                {
                    writer.Write(binary.ToArray(), 0, binary.Count);
                }
            }
        }
        catch (Exception ex)
        {
            Debug.WriteLine(ex);
        }
        finally
        {
            // Stop listening for new clients.
            server.Stop();
        }
    }

Can anyone see something that I am missing/doing wrong?

Answer 1

The corruption is caused by the following code on the server:

// Loop to receive all the data sent by the client.
while ((stream.Read(bytes, 0, bytes.Length)) != 0)
{
    data.AddRange(bytes);
}

stream.Read will not always fill the bytes buffer. It won't be filled if the TCP socket has no more data available, or when reading the last chunk of the message (unless it is an exact multiple of the buffer size).

The data.AddRange call adds everything from bytes (making the assumption that it is always full). As a result, this will occasionally end up adding data from the previous call to stream.Read. To rectify this, you need to store the number of bytes returned by Read and add only this number of bytes:

int length;

while ((length = stream.Read(bytes, 0, bytes.Length)) != 0)
{
    var copy = new byte[length];
    Array.Copy(bytes, 0, copy, 0, length);
    data.AddRange(copy);
}

Note that you might want to restructure your code to improve performance and memory usage (and probably make it easier to read as a result). Instead of reading all the data into memory on the client before sending you can just write directly to the NetworkStream. On the server you don't need to copy everything from the stream into memory. You can read off the 4 byte file name length and decode it, then read and decode the file name and finally copy the remainder of the stream directly to a FileStream (the BinaryWriter is unnecessary).

It is also worth noting that you are creating the output file with FileMode.Append. This means that each file sent will be appended to the previous copy of the same name. You may want to use FileMode.Create instead, which will overwrite if the file already exists.