VB.NET - Data from TCP Client isn't in order

Question

I've been working on this project for about a year now. It's a basic client\server chat program. After a long time of improvement, I decided to test out the strength of my server.

On the client, I fired off 200 chat messages ("FLOOD# 1"... "FLOOD# 200") to the server as fast as possible. The result: Server immediately crashes. After some slight tampering, I was able to get the server to process 135 of the 200 messages before giving up. It no longer crashes, but something different happens. The data from the client is received in order, but when I pass that message to a function (myForm.OnLineReceived), The data is completely out of order. If I add a slight delay between the calling of the OnLineRecieved function, The messages are in perfect order.

Each message from the client is first encrypted, then encoded in base64. An "-" is appended to the end so that the server can easily find the end of each data "packet".

I'm sure that it's some stupid mistake that you guys will easily find and point out to me. Thanks for taking a look ;)

Server Code:

Imports System.Net.Sockets
Imports System.Text

' The UserConnection class encapsulates the functionality of a TcpClient connection
' with streaming for a single user.
Public Class UserConnection

Private client As TcpClient
Private readBuffer(READ_BUFFER_SIZE) As Byte

Public UID As String = ""
Public isAdmin As Boolean
Public IpAddress As String
Public username As String = ""
Public Country As String = ""
Public ServerID As String = ""
Public Status As String = ""
Public UserComp As String = ""
Public OS As String = ""

Public SessionKey As String = ""
Public UsePublicKeyEncryption As Boolean = True

Public Version As Decimal = 0.0

Const READ_BUFFER_SIZE As Integer = 500

Private _commands As New System.Text.StringBuilder
Private command_count As Integer = 1

' Overload the New operator to set up a read thread.
Public Sub New(ByVal client As TcpClient) 'this runs every time a new client is added
    Me.client = client
    IpAddress = Me.client.Client.RemoteEndPoint.ToString.Substring(0, Me.client.Client.RemoteEndPoint.ToString.LastIndexOf(":")) 'ip address of client
    ' This starts the asynchronous read thread.  The data will be saved into
    ' readBuffer.
    Call Worker()
End Sub

Public Sub ForceKill()
    On Error Resume Next
    client.GetStream.Close()
    client.Close()
    client = Nothing
End Sub

Private Sub Worker()
    Try
        SyncLock client
            Dim tmp_byte(client.ReceiveBufferSize) As Byte
            Me.client.GetStream.BeginRead(tmp_byte, 0, client.ReceiveBufferSize, AddressOf RecieveDataAndSplit, Nothing)
            readBuffer = tmp_byte
        End SyncLock

    Catch
        Call myForm.OnLineReceived(Me, "D") 'this also calls ForceKill()
    End Try
End Sub


Public Event LineReceived(ByVal sender As UserConnection, ByVal Data As String)

' This subroutine uses a StreamWriter to send a message to the user.
Public Sub SendData(ByVal Data As String)
    ' Synclock ensure that no other threads try to use the stream at the same time.
    SyncLock client
        Dim writer As New IO.StreamWriter(client.GetStream)
        writer.Write(ToBase64(AES_Encrypt(Data, SessionKey)) & "-")
        ' Make sure all data is sent now.
        writer.Flush()
    End SyncLock
End Sub



Public Sub RecieveDataAndSplit(ByVal ar As IAsyncResult) 'this is the FIRST function that incoming data is ran through
    Dim BytesRead As Integer
    Dim Content As String

    Try
        ' Ensure that no other threads try to use the stream at the same time.
        SyncLock client
            ' Finish asynchronous read into readBuffer and get number of bytes read.
            BytesRead = client.GetStream.EndRead(ar)
        End SyncLock
    Catch e As Exception
        Call myForm.OnLineReceived(Me, "D") 'couldn't read the stream from the client. Kill our connection with them :P
        Exit Sub
    End Try



    Try
        Content = Encoding.ASCII.GetString(readBuffer, 0, BytesRead)
    Catch ex As Exception
        Call Worker()
        Exit Sub
    End Try


    Dim commands() As String
    Try
        commands = LineTrim(Content).Split("-")
    Catch
    End Try


    Dim i As Integer = 0

    For i = 0 To commands.Length - 1

        If commands(i) <> "" Then

            Dim decrypted_content As String = AES_Decrypt(FromBase64(commands(i)), SessionKey)
            If decrypted_content <> "" Then

                'If decrypted_content = "D" Or Nothing Then
                '    client.GetStream.Close()
                '    client.Close()
                '    Call myForm.OnLineReceived(Me, decrypted_content)
                'Else

                Call myForm.OnLineReceived(Me, decrypted_content)
                Call Worker() 'reads the stream again
                'End If
            End If
        End If
    Next

End Sub
End Class

---

Client Code:

Public Sub SendData(ByVal data As String)
    Try
        If data = "D" Then 'telling server that we're closing
            ForceDisconnect(False)
        Else 'any other message
            Dim sendBytes As [Byte]()

            sendBytes = Encoding.ASCII.GetBytes(ToBase64(AES_Encrypt(data, SessionKey)) & "-")

            Dim networkStream As NetworkStream = tcp_client.GetStream()
            networkStream.Write(sendBytes, 0, sendBytes.Length)
            networkStream.Flush()
        End If
    Catch ex As Exception
        connection_state_toggle(False)

        Label1.ForeColor = Color.Black
        Label1.Text = "Idle"
    End Try



End Sub

Answer 1

Classic TCP/IP networking mistake. You are assuming that the data sent is in messages or packets, but it really is a stream. Let's say your client send message1-message2-message3-message4. On the server side on your read callback you may get:

message1-m

or

message1-message2-

or

message1-message2-message3-message4

or just

m

Think about what happens to your parsing code (the splitting of commands) when you get messages fragmented like this. Good TCP/IP code should be able to survive receiving data a single byte per read. If it cannot then you are bound to run into problems.

The typical approach is to keep adding to a buffer and inspecting the buffer each time for a completed message and then poping off just that message, leaving any partial message trailing in the buffer to get filled out later. Checks for DOS attacks/problems like discarding the buffer if it gets too large (based on your protocol) should be added at some point as well.

Answer 2

            Dim thrd As New System.Threading.Thread(AddressOf RecieveDataAndSplit)
            thrd.Start(bytesFrom)
            thrd.Join()

You're creating new threads to process the data, there is no guarantee to the order of how these threads will get CPU time, thus the reason they are added out of order.

---

    SyncLock client.GetStream
        Dim tmp_byte(client.ReceiveBufferSize) As Byte
        Me.client.GetStream.BeginRead(tmp_byte, 0, client.ReceiveBufferSize, AddressOf RecieveDataAndSplit, Nothing)
        readBuffer = tmp_byte
    End SyncLock

I don't have anything to debug with me right now, but I'm wondering if GetStream may be returning a different object reference each time you call it, thus making the Synclock ineffective for what you're trying to do. I would try doing Synclock just on client.

Answer 3

Ok, thanks to the input of everyone (mostly tcarvin's idea of making a 'buffer' of partial commands), I've managed to get some spiffy code working!

Hopefully this will save others the days of agony that I went through myself.

Here is the 100% working code:

Imports System.Net.Sockets
Imports System.Text

' The UserConnection class encapsulates the functionality of a TcpClient connection
' with streaming for a single user.
Public Class UserConnection

Private client As TcpClient

Private income_message_buffer As New System.Text.StringBuilder 'all new messages are added onto the end of this. messages are pulled from the beginning in a timely manner

Public Sub Run(ByVal client As TcpClient)
    Me.client = client
    Call MessageParser()
End Sub

Public Sub ForceKill()
    On Error Resume Next
    client.GetStream.Close()
    client.Close()
    client = Nothing
End Sub

Private Sub MessageParser()
    Do

        If client.Connected = True Then
            If client.GetStream.DataAvailable = True Then
                Dim tmp_byte(client.ReceiveBufferSize) As Byte
                Dim BytesRead As Integer
                Dim content As String

                SyncLock client
                    BytesRead = Me.client.GetStream.Read(tmp_byte, 0, client.ReceiveBufferSize)
                End SyncLock

                Try
                    content = Encoding.ASCII.GetString(tmp_byte, 0, BytesRead)
                    income_message_buffer.Append(LineTrim(content))
                Catch ex As Exception

                End Try


            End If
        End If


        Dim EndOfFirstMessage As Integer = income_message_buffer.ToString.IndexOf("-") 'gets the first occurace of "-" in the buffer
        If EndOfFirstMessage >= 0 Then
            Dim message As String = income_message_buffer.ToString.Substring(0, EndOfFirstMessage) 'gets everything before the "-"
            income_message_buffer.Remove(0, EndOfFirstMessage + 1) 'removes the first message AND the "-"
            Call ParseMessage(message)
        End If



    Loop
End Sub

Public Event LineReceived(ByVal sender As UserConnection, ByVal Data As String)

' This subroutine uses a StreamWriter to send a message to the user.
Public Sub SendData(ByVal Data As String)
    ' Synclock ensure that no other threads try to use the stream at the same time.
    SyncLock client
        Dim writer As New IO.StreamWriter(client.GetStream)
        writer.Write(ToBase64(AES_Encrypt(Data, SessionKey)) & "-")
        ' Make sure all data is sent now.
        writer.Flush()
    End SyncLock
End Sub



Public Sub ParseMessage(ByVal message As String) 'this is the FIRST function that incoming data is ran through

    Dim decrypted_content As String = AES_Decrypt(FromBase64(message), SessionKey)

    If decrypted_content <> "" Then
        Call myForm.OnLineReceived(Me, decrypted_content)
    End If


End Sub
End Class