Issues with sending serialized objects through Socket

[WildBamaBoy]

I'm trying to send data back and forth between two computers using a Socket (TCP). The data is in the form of serialized Packet objects.

At an early point in the application, it must send 25 or so Packets of the same type to the other side. See Case 1 in HandlePacket(). After a few Packets of this type are sent (2 to 5, it's random), the other side crashes with a SerializationException: "Binary stream '0' does not contain a valid BinaryHeader."

I've confirmed that I am always reading the same amount of data from the socket that was sent from the other computer, and I've also confirmed with an MD5 hash that, when the exception is thrown, the data received is not the same actual data that was sent.

Keep in mind that some packets of the type that is failing to deserialize actually do make it through and are processed correctly. I can't figure out why this is happening.

Also, Packets are typically responded to as soon as they are received, so the Send and Read methods usually run on the same thread. Here's my code to send and receive Packets, can anyone see anything wrong?

        public void SendPacket(Packet P)
        {
            using (MemoryStream MS = new MemoryStream())
            {
                BinaryFormatter BF = new BinaryFormatter();
                BF.Serialize(MS, P);

                byte[] Payload = MS.ToArray();
                int PayloadSize = Payload.Length;

                SocketMain.Send(BitConverter.GetBytes(PayloadSize));
                SocketMain.Send(Payload);

                Console.WriteLine("Sent:" + P.GetType().ToString() + ", " + Payload.Length + " | " + Utility.GetMD5Hash(Payload));
            }
        }

        public void ReadPacket()
        {
            byte[] BufferArray = new byte[131072];
            SocketMain.Receive(BufferArray, sizeof(int), SocketFlags.None);

            int PayloadSize = BitConverter.ToInt32(BufferArray, 0);
            SocketMain.Receive(BufferArray, PayloadSize, SocketFlags.None);

            byte[] Payload = BufferArray.Take(PayloadSize).ToArray();

            Console.WriteLine("Received:" + PayloadSize.ToString() + " | " + Utility.GetMD5Hash(Payload));
            using (MemoryStream MS = new MemoryStream(Payload))
            {
                BinaryFormatter BF = new BinaryFormatter();
                HandlePacket((Packet)BF.Deserialize(MS));
            }
        }

        private void HandlePacket(Packet P)
        {
            switch (P.ID)
            {
                case 1:
                    SendPacket(new Packet2InitializeData(SystemInformation.PrimaryMonitorSize.Width, SystemInformation.PrimaryMonitorSize.Height));
                    InitializeScreenBlockList();

                    int Index = 0;
                    foreach (ScreenBlock SB in ScreenBlocks)
                    {
                        SB.UpdateScreenBlock();
                        //------------------------------------------------------
                        SendPacket(new Packet3BlockUpdate(Index, SB)); //While sending these packets, the exception occurs.
                        //------------------------------------------------------
                        Index++;
                    }
                    WasInitializationDataSent = true;
                    break;
                case 2:
                    HandlePacket2InitializeData((Packet2InitializeData)P); break;
                case 3:
                    HandlePacket3BlockUpdate((Packet3BlockUpdate)P); break;
                case 4:
                    HandlePacket4BlockVerify((Packet4BlockVerify)P); break;
                case 5:
                    HandlePacket5BlockRequest((Packet5BlockRequest)P); break;
            }
        }            

Here's the packet object:

    [Serializable()]
    public abstract class Packet : ISerializable
    {
        public byte ID;

        public Packet()
        {
        }

        public Packet(byte ID)
        {
            this.ID = ID;
        }

        public abstract void GetObjectData(SerializationInfo info, StreamingContext ctxt);
    }

And the packet that always causes the exception. A few packets of this type do make it through.

[Serializable()]
    public class Packet3BlockUpdate : Packet, ISerializable
    {
        public ushort Index;
        public Image BlockImage;
        public string MD5;

        public Packet3BlockUpdate(int Index, ScreenBlock Block): base(3)
        {
            this.Index = (ushort)Index;
            this.BlockImage = Block.Image;

            MemoryStream MS = new MemoryStream();
            this.BlockImage.Save(MS, ImageFormat.Jpeg);
            MD5 = Utility.GetMD5Hash(MS.ToArray());
        }

        protected Packet3BlockUpdate(SerializationInfo info, StreamingContext context)
        {
            Image A = new Bitmap(1, 1);

            this.ID = info.GetByte("ID");
            this.Index = info.GetUInt16("Index");
            this.BlockImage = (Image)info.GetValue("Image", A.GetType());
            this.MD5 = info.GetString("MD5");
        }

        public override void GetObjectData(SerializationInfo info, StreamingContext context)
        {
            info.AddValue("ID", this.ID);
            info.AddValue("Index", this.Index);
            info.AddValue("Image", this.BlockImage);
            info.AddValue("MD5", this.MD5);
        }
    }

[Ketsuekiame]

Socket.Receive even with the byte length argument, isn't guaranteed to actually have that much data. Essentially, the byte length argument is a "max limit". It will receive as much data as possible up to that value. So if your socket has sent 15 of 30 bytes in the first TCP window, your receive method will pick up the 15 bytes and return. It will not wait for the second 15.

Essentially, I think this is what's happening to you. Although you've sent say 100bytes, if Windows or the network driver decides to only send 98 for example, your receive will be short.

For reliability purposes, you should loop your receive method (it will return to you exactly how many bytes it placed into your buffer) until you have received all data or a timeout occurs.

[WildBamaBoy]

Thanks! Didn't know that. It works for a short period of time now, but it still ends up crashing with the same sort of exception on the same Packet when testing over my local network. Testing by connecting to myself, though, works great, finally.

These Packets are anywhere from 16kb to around 100kb, by the way.

I've changed ReadPacket() to the following. Is this what you meant?:

        public void ReadPacket()
        {
            byte[] BufferArray = new byte[131072];
            SocketMain.Receive(BufferArray, sizeof(int), SocketFlags.None);

            int PayloadSize = BitConverter.ToInt32(BufferArray, 0);
            int BytesReceived = 0;

            while (BytesReceived < PayloadSize)
            {
                BytesReceived += SocketMain.Receive(BufferArray, BytesReceived, PayloadSize - BytesReceived, SocketFlags.None);
            }

            byte[] Payload = BufferArray.Take(PayloadSize).ToArray();

            Console.WriteLine("Received:" + PayloadSize.ToString() + " | " + Utility.GetMD5Hash(Payload));
            using (MemoryStream MS = new MemoryStream(Payload))
            {
                BinaryFormatter BF = new BinaryFormatter();
                HandlePacket((Packet)BF.Deserialize(MS));
            }
        }

Any ideas where to go next or is my logic wrong?

[Ketsuekiame]

When you write how many bytes are received on line 16, does this match up with the number of bytes sent?

[WildBamaBoy]

Yes, it does. In my most recent try, one of the smallest packets (always 194 bytes) caused the issue. I seemingly received all data, but MD5 hashes were different.

I've tried to handle the exception and just keep going. One packet screwing up out of 100 is tolerable in the application. But since I send the size before the payload, one failed packet causes all of the others to be read incorrectly. I end up reading an absurd payload size like 9557463 or even -594831. If there is a way around this, I'd gladly take it.

Also, I noticed that Socket.Send returns an int which I assume is bytes that were sent. Does Socket.Send actually send that much data each time or does it work the same way as Socket.Recieve?

[Ketsuekiame]

No, send will block until you've sent the entire buffer to the system buffer, unless you have it in "non-blocking mode", in which case it's not guaranteed to send the entire buffer to the system buffer, but only the amount of space available in the system buffer.

It would be good practice in your case (as you're having issues) to check whether you're sending the correct amount of data before we continue debugging, but I've put millions of bytes into the buffer before and not had issues...

The fact that you end up with skewed numbers indiciates that either you're reading too much or too little data on one of your read operations. It may be that the payload calculation size is off. Have you stepped through and checked all the buffer sizes etc.

As an aside, your first receive isn't looping. Whilst I realise that it's only 4 bytes, there's no guarantee that you received all 4 straight away.

To make it more efficient, the system will attempt to fill as much data as possible into the buffer, until it reaches a certain thresh-hold. If this thresh-hold is not reached, the system will delay the send until a certain period of time, or, there is more data to send. So if your thresh-hold is 128 bytes, your last packet filled 126 and then your payload size for the next item goes on, hits 128, the driver sends the buffer and the rest of your int (the final 2 bytes) goes into the (now empty) buffer and sits there, because there isn't enough data to send yet.

PS. This is a gross over simplification; the way it works underneath is quite complex and dynamic.

[WildBamaBoy]

I think I've found the issue after looking at console output with these changes:

public void SendPacket(Packet P)
        {
            using (MemoryStream MS = new MemoryStream())
            {
                Console.WriteLine("SEND START");
                BinaryFormatter BF = new BinaryFormatter();
                BF.Serialize(MS, P);

                byte[] Payload = MS.ToArray();
                int PayloadSize = Payload.Length;
                int SentSize = 0;

                SentSize += SocketMain.Send(BitConverter.GetBytes(PayloadSize));
                Console.WriteLine("\tSocket.Send (PayloadSize): " + SentSize);

                SentSize = 0;

                SentSize += SocketMain.Send(Payload);
                Console.WriteLine("\tSocket.Send (Payload): " + SentSize);

                Console.WriteLine("\tSent payload:" + P.GetType().ToString() + "," + SentSize + " | " + Utility.GetMD5Hash(Payload));
            }
        }

        public void ReadPacket()
        {
            Console.WriteLine("READ START");
            byte[] BufferArray = new byte[131072];
            int BytesReceived = 0;

            while (BytesReceived < sizeof(int))
            {
                BytesReceived += SocketMain.Receive(BufferArray, BytesReceived, sizeof(int) - BytesReceived, SocketFlags.None);
                Console.WriteLine("\tSocket.Receive (PayloadSize): " + BytesReceived);
            }

            int PayloadSize = BitConverter.ToInt32(BufferArray, 0);
            Console.WriteLine("\tReceived payload size: " + PayloadSize);
            BytesReceived = 0;

            while (BytesReceived < PayloadSize)
            {
                BytesReceived += SocketMain.Receive(BufferArray, BytesReceived, PayloadSize - BytesReceived, SocketFlags.None);
                Console.WriteLine("\tSocket.Receive (Payload): " + BytesReceived);
            }

            byte[] Payload = BufferArray.Take(PayloadSize).ToArray();

            Console.WriteLine("\tReceived payload:" + PayloadSize.ToString() + " | " + Utility.GetMD5Hash(Payload));
            using (MemoryStream MS = new MemoryStream(Payload))
            {
                BinaryFormatter BF = new BinaryFormatter();

                try
                {
                    HandlePacket((Packet)BF.Deserialize(MS));
                }

                catch (SerializationException E)
                {
                    Console.WriteLine("------------------ STOP ------------------");
                    SendPacket(new Packet6StopError());
                    Console.WriteLine("------------------ STOP ------------------");
                    throw E;
                }
            }
        }

It looks like I've got packets sending from somewhere else (no idea where) at the same time as other packets. Several times on the console I can see:

SEND START
SEND START
    Socket.Send (PayloadSize): 4
    Socket.Send (Payload): 38398
    Socket.Send (PayloadSize): 4
    Socket.Send (Payload): 18710
  [...]

When the crash happens on my end, I noticed this on the other end for the packet that was sent. I don't see it anywhere else in the console, but it's a lot of data to go through even for just a few seconds of being connected:

SEND START
SEND START
    Socket.Send (PayloadSize): 4
    Socket.Send (PayloadSize): 4
    Socket.Send (Payload): 20974
    Socket.Send (Payload): 194
  [...]

Data is being sent out of order, is it not?

[Ketsuekiame]

It shouldn't be crashing in the first place, but yes, your subsequent sends are out of sync. This could indicate a threading issue with your code.

If you want, PM me with a location that I can download your entire source from and I'll take a look and debug through it. At this point, I think this is the fastest way to help you because I can't see anything wrong now.

[WildBamaBoy]

Thanks for offering, but I've got it sorted out now. Thanks for all of your help.

[Ketsuekiame]

Out of interest, what was your solution?

[WildBamaBoy]

I found the threading issue - turns out it really needs to stay that way, so I just make any thread that wants to send or read a packet check and see if a packet is being handled at that moment. If it is, it'll sleep and try again until it goes through.

Code if anyone needs to do something similar:

        public void SendPacket(Packet P)
        {
            while (IsPacketBeingSent)
            {
                Thread.Sleep(5);
            }

            IsPacketBeingSent = true;
            {
                using (MemoryStream MS = new MemoryStream())
                {
                    //Console.WriteLine("SEND START");
                    BinaryFormatter BF = new BinaryFormatter();
                    BF.Serialize(MS, P);

                    byte[] Payload = MS.ToArray();
                    int PayloadSize = Payload.Length;
                    int SentSize = 0;

                    SentSize += SocketMain.Send(BitConverter.GetBytes(PayloadSize));
                    //Console.WriteLine("\tSocket.Send (PayloadSize): " + SentSize);

                    SentSize = 0;

                    SentSize += SocketMain.Send(Payload);
                    //Console.WriteLine("\tSocket.Send (Payload): " + SentSize);

                    //Console.WriteLine("\tSent payload:" + P.GetType().ToString() + "," + SentSize + " | " + Utility.GetMD5Hash(Payload));
                }
            }
            IsPacketBeingSent = false;
        }

        public void ReadPacket()
        {
            while (IsPacketBeingRead)
            {
                Thread.Sleep(5);
            }

            IsPacketBeingRead = true;
            {
                //Console.WriteLine("READ START");
                byte[] BufferArray = new byte[131072];
                int BytesReceived = 0;

                while (BytesReceived < sizeof(int))
                {
                    BytesReceived += SocketMain.Receive(BufferArray, BytesReceived, sizeof(int) - BytesReceived, SocketFlags.None);
                    //Console.WriteLine("\tSocket.Receive (PayloadSize): " + BytesReceived);
                }

                int PayloadSize = BitConverter.ToInt32(BufferArray, 0);
                //Console.WriteLine("\tReceived payload size: " + PayloadSize);
                BytesReceived = 0;

                while (BytesReceived < PayloadSize)
                {
                    BytesReceived += SocketMain.Receive(BufferArray, BytesReceived, PayloadSize - BytesReceived, SocketFlags.None);
                    //Console.WriteLine("\tSocket.Receive (Payload): " + BytesReceived);
                }

                byte[] Payload = BufferArray.Take(PayloadSize).ToArray();

                //Console.WriteLine("\tReceived payload:" + PayloadSize.ToString() + " | " + Utility.GetMD5Hash(Payload));
                using (MemoryStream MS = new MemoryStream(Payload))
                {
                    BinaryFormatter BF = new BinaryFormatter();

                    try
                    {
                        HandlePacket((Packet)BF.Deserialize(MS));
                    }

                    catch (SerializationException E)
                    {
                        //Console.WriteLine("------------------ STOP ------------------");
                        SendPacket(new Packet6StopError("Fatal exception. " + E.Message));
                        //Console.WriteLine("------------------ STOP ------------------");
                        throw E;
                    }
                }
            }
            IsPacketBeingRead = false;
        }

[Ketsuekiame]

Why don't you use a Mutex instead (or even a simple lock)?

In your class declaration, create a static readonly object
private static readonly object _padLock = new object();

Then in your code, rather than having the "IsPacket...." etc you lock on the object.

lock(_padlock)

Because you put your code inside it's own scope you don't need to both with extra brackets.

private static readonly object _padlock = new object();

public void SendPacket(Packet P)
{
    lock(_padlock)
    {
        using (MemoryStream MS = new MemoryStream())
        {
            //Console.WriteLine("SEND START");
            BinaryFormatter BF = new BinaryFormatter();
            BF.Serialize(MS, P);

            byte[] Payload = MS.ToArray();
            int PayloadSize = Payload.Length;
            int SentSize = 0;

            SentSize += SocketMain.Send(BitConverter.GetBytes(PayloadSize));
            //Console.WriteLine("\tSocket.Send (PayloadSize): " + SentSize);

            SentSize = 0;

            SentSize += SocketMain.Send(Payload);
            //Console.WriteLine("\tSocket.Send (Payload): " + SentSize);

            //Console.WriteLine("\tSent payload:" + P.GetType().ToString() + "," + SentSize + " | " + Utility.GetMD5Hash(Payload));
        }
    }
}

This will give you your thread safety. You can do the same thing on ReadPacket but you will need to use a different object.

[WildBamaBoy]

The brackets are a horrible habit I've picked up from Java OpenGL programming. I hate it, and I hate that I do it.

I didn't know about lock - I'll use it. I've transitioned from Java to C# without bothering to read up on language specs.

Thanks again.

[Ketsuekiame]

There's nothing wrong with local scoping, can actually be quite useful :)