2
\$\begingroup\$

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();
}
\$\endgroup\$
9
  • 1
    \$\begingroup\$ Why 33 in Task.Delay(33, ... ? \$\endgroup\$ – thesyndarn May 22 '16 at 7:51
  • 1
    \$\begingroup\$ @thesyndarn 30 ticks per second \$\endgroup\$ – Kenny May 22 '16 at 12:49
  • 1
    \$\begingroup\$ @RubberDuck Sure! [2 Bytes Command][2 Bytes Payload Size][4 Bytes Sequence][Payload] \$\endgroup\$ – Kenny May 22 '16 at 12:52
  • 1
    \$\begingroup\$ @Neel I need to wait until I can read the first 4 bytes of each packet, which contains the command and payload size. So I can tell when the packet ends or if I've to wait for more data. \$\endgroup\$ – Kenny May 22 '16 at 13:00
  • 1
    \$\begingroup\$ @RubberDuck this code parses all the data that it receives and splits each packet. There's another loop running at the same that reads the list of packets parsed. \$\endgroup\$ – Kenny May 22 '16 at 14:35
2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Whoa, that's really simple. I'm using this code both on Android and Windows, works well! Thanks, I got a couple questions: LoadAsync calls ReadAsync to receive more bytes? Should I use Timer for game clients too? \$\endgroup\$ – Kenny May 22 '16 at 14:56
  • \$\begingroup\$ My guess is that it does call ReadAsync internally, but that's kind of the point. It's an implementation detail of the DataReader class that I don't really need to know about. Not sure what you mean about using Timer for game clients. Like I said, it's just an idea, I didn't actually try any of this. \$\endgroup\$ – RubberDuck May 22 '16 at 15:00
  • \$\begingroup\$ I updated the code above with a few fixes as it was a little clusterfuck. I'll try your code, thank you so much. \$\endgroup\$ – Kenny May 22 '16 at 15:05
  • \$\begingroup\$ @MiguelRipoll please don't update your code after receiving an answer. It's against site policy. I would wait a bit to see if any more answers come in, fix up your code based on those answers, and then ask a new question if you still have concerns. You're very welcome. Thank you for posting an interesting question. \$\endgroup\$ – RubberDuck May 22 '16 at 15:07
  • 1
    \$\begingroup\$ Reading your comment made my day, thanks for the question and for stopping by. One of my goals for this summer was to make a multiplayer game and while I'm not 100% sure of what I'm doing is correct I got it working. I had to change the code above lot of times to be able to parse incomplete packets (started to become a huge problem) and I was thinking of going back here with the updated version to try to do it the best way possible but I needed to fix lot of unrelated bugs. Here is the updated version (line 145) github.com/MiguelRipoll23/uwp-multiplayer/blob/master/Game/… \$\endgroup\$ – Kenny Jul 29 '17 at 21:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.