Protocol to parse packets

Question

I coded this simple protocol but I'm sure that it can be better, this is the process:

• Reads incoming data
• Waits until payload size can be read
• Reads the full message
• Checks if there are mixed messages
• Reads incoming data again

Code:

private async void ReadPackets(StreamSocket socket, SocketInformation socketInformation)
{
    byte[] data = new byte[BufferSize];

    IInputStream inputStream = socket.InputStream;
    IBuffer buffer = data.AsBuffer();

    while (socketInformation.open)
    {
        IBuffer bytesRead;

        try
        {
            bytesRead = await inputStream.ReadAsync(buffer, BufferSize, InputStreamOptions.Partial);

            if (bytesRead.Length > 0)
            {
                PrintByteArray("Data", data);
            }
        }
        catch (Exception)
        {
            break;
        }

        // Wait for payload size
        if (buffer.Length >= 4)
        {
            int commandType = (int)BitConverter.ToInt16(data, 0);
            int payloadSize = (int)BitConverter.ToInt16(data, 2);
            int packetSize = PacketHeaderSize + payloadSize;

            // Wait for full packet
            if (buffer.Length >= packetSize)
            {
                byte[] packet = new byte[packetSize];
                System.Buffer.BlockCopy(data, 0, packet, 0, packetSize);

                Debug.WriteLine("Packet received.");

                ConcurrentQueue<byte[]> packetReceiveQueue = socketInformation.GetPacketReceiveQueue();

                packetReceiveQueue.Enqueue(packet);

                if (buffer.Length > packetSize)
                {
                    int bufferLength = (int)buffer.Length - packetSize;

                    byte[] newData = new byte[BufferSize];
                    System.Buffer.BlockCopy(data, packetSize, newData, 0, bufferLength);
                    data = newData;

                    buffer.Length = (uint)bufferLength;
                }
            }
        }
    }

    Debug.WriteLine("Socket closed.");
    inputStream.Dispose();
    socket.Dispose();
}

Answer 1

I'm going to preface this with "I didn't actually try this, and the code may not even compile.", but I think you can greatly simplify your code by utilizing the DataReader class instead of parsing everything "by hand".

I'm imagining your routine looking something like this.

using (var dataReader = new Windows.Storage.Streams.DataReader(input.InputStream))
{
    // use network order to read the packet
    dataReader.ByteOrder = Windows.Storage.Streams.ByteOrder.BigEndian;

    //load the 2 byte command and 2 byte payload size into the buffer
    await dataReader.LoadAsync(4);

    short command = dataReader.ReadUint16();
    short payloadSize = dataReader.ReadUint16();

    // now we know how big the payload is, and can load it in full into the buffer.
    await dataReader.LoadAsync(payloadSize);

    byte[] data;
    dataReader.ReadBytes(out data);

    // package the whole shebang into a struct
    var myStruct = new MyStruct(command, payloadSize, data);

    // raise packet received event.
    Invoke?.PacketReceived(this, new PacketReceivedEventArgs(myStruct);

}

There are a couple of benefits to this approach.

1. Relies on existing abstractions and framework code instead of rolling your own.
2. We've defined an actual data structure for your packets. Client code can use this higher level abstraction without knowing all the knitty gritty details of how the packet was passed over the network.
3. Removes the need for the "other" infinite loop on a separate thread that's reading your data. It can listen for the PacketReceived event and react to it and the explicitly passed packet instead. This should lower the risk of race conditions.

---

One last note, don't use an infinite loop to receive the packets either. It's much nicer (and user less CPU) to use a System.Threading.Timer to do this kind of work. It has a 15ms resolution but, then again, so does Task.Delay. You're waiting longer than that resolution anyway.