TCPClient Losing Data

[DrZoop]

I apologize in advance, but I'm quite pissed that not only do I have to deal with this same bug for nearly a year now, but the forums just ate my beautiful post explaining my entire problem in complete detail.

Long story short: I am misusing the TCPClient class. I am sending 400-500 byte messages, and data is being lost.

How can the TCPClient class be misused to lose data?

Random notes:
-I know the TCP protocol doesn't lose data. Don't tell me that. I am educated.
-I am buffering all data being received before checking for messages and reading them, so don't tell me this either. I have quite a spiffy message system tried and proven. It's just these large chunks of data screwing me over.
-I am using BeginRead/EndRead to receive my data.
-I am using a huge receive and send buffers, this simply can't be the problem unless I'm misusing them.
-Changing the receive buffer does affect how long before it breaks. However, how many bytes it takes before it breaks is inconsistent, even with the same bytes being sent each time.
-Through tedious byte by byte comparison, it appears that "older" bytes are being overwritten with newer, more recently received bytes. By comparing the sent and received bytes, there is a gap of missing bytes in the center. It's as if the buffer is too small, though, 3 megabytes for 500 bytes should be plenty of space... plus, the EndRead function should be called anyway!
-I have read that the ReceiveTimeOut property should be set to something else than zero to ensure no loss of data. Why is this? What if it ran out of time and no bytes were received? Wouldn't this return a length of zero bytes and appear to be a disconnection?
-Where it breaks is inconsistent. I can do the same login procedure time and time again, and it will break at different places.

-Kinda unrelated: What is the State argument for in the BeginRead function!? MSDN says you should "at the minimum pass the NetworkStream class." One example passes the TCPClient class. Another example passes seemingly random things (the form, a button on a toolstrip). The example I based mine off of passed nothing/null.

Thanks; I'm sorry for the ugliness of this post, but losing my well-written post was the last thing I needed right now.

 

[JohnH]

The State argument for asynchronous methods is an Object where you can put anything you want, you can use it to distinguish between different method calls that use the same callback, it is also commonly used to pass specific instances that started the async method for the callback to use for end calls.

Do you check if the number of bytes requested by BeginRead is actually read? EndRead returns the number of bytes read. If it didn't read the number of bytes you expect you need subsequent reads while you buffer up received data, and only handle it when you know you have received the full package.

Not knowing the code or type of data it's hard to tell if you do wrong or if perhaps there are better ways to do it than your current implementation.

 

[DrZoop]

I'm just going to copy and paste it. I will add further comments so it's easier for you all to read.

It sends and receives byte arrays. It's pretty bleedingly straightforward. I have a fancy message class (my own binary stream that actually counts booleans as a single bit, among other things which can make messages smaller) that uses message length UShorts attached to the front of each message with checksums to make sure they were not tampered with. This class works fine. I have tested it on its own. It works fine with small messages through TCP (and UDP). It is the large messages that are screwing me over. I am doing something wrong in my usage of the .NET TCPClient class that is causing me to lose bytes.

Private Sub BeginRead()
        If Connected() Then
            SyncLock myClient.GetStream 'original example used synclocks
                If myByteBuffer.Length <> myClient.ReceiveBufferSize Then 'resizes the application byte buffer.  Do not worry, this buffer is only used for one read.  It is set earlier to set the ReceiveBufferSize to an incredible unnecessary size.
                    ReDim myByteBuffer(myClient.ReceiveBufferSize + 1)
                End If

                Try
                    myClient.GetStream.BeginRead(myByteBuffer, 0, myClient.ReceiveBufferSize, New AsyncCallback(AddressOf ReceivedData), myClient.GetStream) 'earlier I didn't have "New AsyncCallBack."  My original example had only "AddressOf ReceivedData" (ReceivedData is the function with EndRead.  Either way, the code seems to work.  Data is received.
                Catch
                    Console.WriteLine("BeginRead broke.")
                End Try
            End SyncLock
        End If
    End Sub

    Private Sub ReceivedData(ByVal ar As IAsyncResult)
        If HasConnected Then
            Dim Length As Integer
            Dim NS As NetworkStream = CType(ar.AsyncState, NetworkStream) 'A newer example used this opposed to what is commented out below.
            Length = NS.EndRead(ar)
            'Try
            '    SyncLock myClient.GetStream
            '        Length = myClient.GetStream.EndRead(ar)
            '    End SyncLock
            'Catch
            'will be caught by length <= 0 below
            'End Try

            If Length <= 0 Then
                Close("Connection interrupted.")
                Exit Sub
            Else
                SyncLock myReceiveBuffer 'is the synclock necessary?  myReceiveBuffer is the true buffer that buffers everything.
                    'update byte counts
                    myBytesReceived += Length 'just useful to know, for debugging; you may ignore
                    myBytesReceivedLast = Length

                    If ThreadSafe Then 'set flag if thread safe 'you may ignore this as well
                        myBytesReceivedChanged = True
                    Else
                        RaiseEvent BytesReceived(myBytesReceived, Length)
                    End If

                    Dim Shortened(Length - 1) As Byte 'this shortens myByteBuffer to the length it really received
                    Array.Copy(myByteBuffer, Shortened, Length) 'same as previous
                    myReceiveBuffer.PushBytes(Shortened) 'puts the trimmed data into my special message class buffer.  Do not question this class, because I had the same problem before using this version of it.  This new version has been tested with smaller messages and works just dandy fine.

                    'only pop messages and raise events here if not threadsafe.
                    If Not ThreadSafe Then
                        PopMessages() 'pops complete messages from the myReceiveBuffer class.  I told you I was buffering data.  Whenever I ask this question, everyone assumes I have never done TCP before.
                    End If
                    'End If
                End SyncLock

                BeginRead() 'begin reading again after we have read message
            End If
        End If
    End Sub

I apologize again for the exasperation of my posts, and I thank you for any brainstorming you have on this problem. [Rant]I have been dealing with this bug for a year now just about. It seems like if I fiddle with it enough, it disappears... but if I change anything surrounding it, it will happen again. I have seen recently seen some of the weirdest things in Visual Studio .NET ever recently. I am a veteran programmer of twelve years, it's not like I don't know anything. I'm just do not know the full complexity of the .NET TCPClient class, even though it feels like I have read just about every document out there on the web. Whenever I ask questions, everyone assumes the most simple (newbie) problems such as not buffering, or using message lengths... it drives me nuts when I'm already nuts due to the frustration of the coding problem at hand![/RANT]

 

[JohnH]

I don't see any data losing bits here. You don't need SyncLocks unless you have other threads running these BeginRead-ReceivedData methods, which would be strange since you only operate with one tcpclient/networkstream instance. BytesReceived event is just debug info? PushBytes and PopMessages methods aren't asynchronous? If they are you need to use thread synchronization to prevent myReceiveBuffer from being modified from these different threads at the same time.

Also look into using serialization with the BinaryFormatter (in namespace System.Runtime.Serialization.Formatters.Binary) to transfer complete class instances ("packages") over stream with "just 2 lines of code", and BinaryWriter/Reader that also simplifies transfer of basic data types over stream. Both these options is very convenient and you don't have to think about handling the raw byte stream at all.

 

[DrZoop]

I avoid threads as much as possible because, despite all the hype, they seem to cause more problems than solve for me. I do not have threads running the BeginRead-ReceivedData methods, besides the main thread where my main loop is. BeginRead is called from the ReceivedData method and once a connection is made.

PushBytes and PopMessages are not asynchronous... and I have little clue how to make them asynchronous, if they need to be at all. To my knowledge, the BeginRead/ReceivedData methods are "linear." In that, BeginRead is called, then eventually, ReceivedData is called, which ReceivedData called BeginRead again. It's essentially a feed back loop of sorts that only breaks out when the connection has an exception or disconnects. I will look it up in the meantime, but, how do you do thread synchronization with those methods? It really doesn't seem like it's necessary since I avoid creating my own threads as much as possible. I have played with all sorts of things, and read extensively... I still don't get .NET's thread management.

I took a quick look at the BinaryFormatter, looks impressive. Unfortunately, I need to save every bit I can. I can't just mob together the whole class and send it. It is simple and easy it appears, however, it's inefficient as well. Perhaps I'd use it while saving and loading files instead, when space isn't as critical.

I use the BinaryWriter/Reader extensively for files, but never for network. If I recall correctly, it uses an entire byte (or was it four bytes?) for booleans. That's seven or more wasted bits. With my own binary stream class, booleans take one bit, I can write nearly any of the basic variable types, and I can even specify an unusual number of bits to send, such as a 3 bit number, or a 35 bit number.

I will look into all of this.

I did spend the last few hours watching every message being sent and received by both the client and server. It was all working fine, until I took out my breakpoints. Then it broke, just like usual. It definitely seems time related. It also doesn't break as quickly if I set huge receive buffers.

Thanks for the brainstorming!

 

[JohnH]

Could the data sender be the problem? It is not doing async writes that may overlap?

You could also add acknowledge to your protocol; the receiver sends a byte value back to signal that one transmission was received and sender doesn't send next until it gets this. This issue was up here recently and was causing OutOfMemoryException there.

With a local test I sent a one gigabyte file several times through the networkstream using the BeginRead loop and I didn't have any problems with that here.

 

[DrZoop]

Okay, I think you were on the right path with the thread synchronization. I added some Threading.Monitor.Enter/Exit on the myReceiveBuffer object during the popping and pushing, and suddenly the normal breaks disappeared. It appears to be retrieving all the bytes now.

Now, occassionally, it breaks when using Exit on the myReceiveBuffer when popping, but that's it.

You definitely hit it on the head I think. Any further tips on managing the monitor would be great since apparently I'm very new to it. However, with the missing bytes fixed, I'll probably keep my sanity!

You're a God-send, JohnH! Thank you very much! I was losing my mind until then!

 

[JohnH]

Monitor shouldn't have any effect when push+pop runs synchronous with the reads (and other threads not changing the object also).

"breaks when using Exit on the myReceiveBuffer", you mean exception on monitor.exit? I assumed myReceiveBuffer was a reference type, but it could of course be a structure value type, which can't be monitored. I also presumed you didn't change the myReceiveBuffer object instance when popping, only its content array, but if you were resetting it when content array was empty by just declaring a new myReceiveBuffer instance then monitor exit would break.

 

[DrZoop]

There is a PopMessages call for when my socket class is not thread safe. In this scenario, there must be a Tick function called in order to read data. This Tick function is being called! This Tick function is called by the main loop.

This was a way to get around conflicting threads. You can see I had it coded to raise an event saying that data was received and/or a message was received. If that event fired, and caused a change on the windows forms (let's say change the text on a label), it would crash saying that one thread cannot manipulate another thread's data. I know there is a way of solving it, I looked into it in the past, but it seemed ridiculously difficult. It seemed like it required a ton of code. My Tick function idea seemed like it was easier. Unless you can enlighten me, I am probably going to stick to my Tick functions.

Anyway, the point is, PopMessages can not only be called within the second Read function, it can also be called in the Tick function. It is within the PopMessages function where I Enter and Exit the myReceiveBuffer along with when I push bytes into it.

 

[JohnH]

If "tick function" is a Timer Tick event handler you are indeed accessing the object from different threads, so then the Monitor is needed or SyncLock in PopMessages.

 

[DrZoop]

No, no. I didn't mean Tick function as in the Timer's tick. Hehe, I need more power than a silly .NET timer.

I mean, the code that I showed you is part of my own customized socket class. Within that socket class, I have two "modes" it can be in, "thread safe" and not. In thread safe mode, you have to call my own Tick function that I made within my customized socket class. So: mySpecialTCPSocket.Tick. This Tick function within the socket is what calls events and stuff so that everything is within the same thread.

Otherwise, in "thread safe" mode, it raises events automatically whenever a message is receved, etc.

The socket function's tick function is called from the main loop, which is started in Sub Main.

 

[JohnH]

Neither of the three .Net Timer classes are silly, they are very serious!
Anyway, you have at least two threads here that both interact with the same object and you have to synchronize access to it.

 

[DrZoop]

I found where my synchronization exception was coming from I believe.

This is within my close socket function, which occurs either by an outside call or if the socket fails for whatever reason:

myReceiveBuffer = New CBinaryStreamQueue

How do I monitor-i-tize that? I was thinking of putting Enters and Exits just before it... though technically a thread could sneak in just after the exit and before the new one is declared.

Then I started thinking about declaring another variable to hold myReceiveBuffer temporarily while I redeclare myReceiveBuffer... but that doesn't seem like it'd work either since Monitor uses address values.

And then, if I made a copy, doesn't that need to be Monitored as well? As you can see, this is getting complicated.

Rather than pasting code snippets of my shots in the dark, how do you propose Monitoring the code above?

--

Unrelated to the above issue. Do you know the logic behind how to modify data in one thread to another? Particularly, the problem of a new thread (not the main thread) changing something in a Windows Form (which was declared in the main thread). It would be nice to have my socket class be able to raise events without the Tick function being called. I remember reading about it in the past, but it seemed ridiculously complicated. I think I implemented it once, but it was way too much work.

 

[JohnH]

I told you so:

I also presumed you didn't change the myReceiveBuffer object instance when popping, only its content array, but if you were resetting it when content array was empty by just declaring a new myReceiveBuffer instance then monitor exit would break.

If "tick function" is a Timer Tick event handler you are indeed accessing the object from different threads, so then the Monitor is needed or SyncLock in PopMessages.

So you have several options;

• Introduce a Clear method in CBinaryStreamQueue class and use this instead of creating a new instance.
• Use SyncLock in Popmessages to define a critical section since this is the common place where your threads cross. Using a wait handle here would have the same effect.
• Declare a class level sync object and use this to Monitor, Private syncObject As New Object. A Mutex would have the same effect here.
• Make the async Read methods access thread-safe by regular cross thread invoke method. (this is what you ask in added post, one of many examples) Here you also have the options of using BackgroundWorker or implement similar functionality yourself with AsyncOperationManager/AsyncOperation.