Simple TCP Server - String Bulding Problem

[Moiv]

Hi, I am building an Async socket server class in Visual C# express 2008.
I am using the example here
http://www.codeguru.com/csharp/csharp/cs_network/sockets/article.php/c7695

What I'm trying to do is build a string from received data and upon receipt
of an end string identifier (in this case "#*") raise an event which my main
program will have an event handler to do something with the string. The
problem is ReceivedString only ever gets the first characer received added
to it and the following chars are not appended. I'm assuming this has
something to do with threading so I've locked the code that appends to the
ReceivedData string but this doesnt help.

What am I doing wrong?

Callback function is listed below and I've attached the full source in case
it's needed.

// This the call back function which will be invoked when the socket
// detects any client writing of data on the stream
void OnDataReceived(IAsyncResult asyn)
{
try
{
SocketPacket socketData = (SocketPacket)asyn.AsyncState;

int iRx = 0 ;
// Complete the BeginReceive() asynchronous call by
EndReceive() method
// which will return the number of characters written to the
stream
// by the client
iRx = socketData.m_currentSocket.EndReceive (asyn);
char[] chars = new char[iRx + 1];
System.Text.Decoder d =
System.Text.Encoding.UTF8.GetDecoder();
int charLen = d.GetChars(socketData.dataBuffer,
0, iRx, chars, 0);
System.String szData = new System.String(chars);

Object objData = szData.ToString();
SendString(objData);
lock (ReceivedString)
{
ReceivedString = ReceivedString + szData;
//MessageBox.Show(ReceivedString);
}

// Continue the waiting for data on the Socket
WaitForData( socketData.m_currentSocket );

MyEventArgs e = new MyEventArgs(szData);
EventHandler<MyEventArgs> handler = DataReceived;
if (handler != null) handler(this, e);
/*
if (ReceivedString.Length > 2)
{
int stringstart = ReceivedString.Length - 2;
if (ReceivedString.Substring(stringstart) == "#*")
{
MyEventArgs e = new MyEventArgs(ReceivedString);
EventHandler<MyEventArgs> handler = DataReceived;
if (handler != null) handler(this, e);
}
}*/
}
catch (ObjectDisposedException )
{
System.Diagnostics.Debugger.Log(0,"1","\nOnDataReceived:
Socket has been closed\n");
}
catch(SocketException se)
{
MessageBox.Show (se.Message );
}
}

 

[Moiv]

Hi, I am building an Async socket server class in Visual C# express 2008.
I am using the example here
http://www.codeguru.com/csharp/csharp/cs_network/sockets/article.php/c7695


What I'm trying to do is build a string from received data and upon
receipt of an end string identifier (in this case "#*") raise an event
which my main program will have an event handler to do something with
the string. The problem is ReceivedString only ever gets the first
characer received added to it and the following chars are not appended.
I'm assuming this has something to do with threading so I've locked the
code that appends to the ReceivedData string but this doesnt help.

What am I doing wrong?

Unfortunately, your code example is far too cluttered for it to be
practical for me to spend time fully analyzing it. At the same time,
it's not even a complete code example, so there's no way to tell just
how your sockets are getting connected.

But here are some problems that I noticed just skimming through your code:

• You are creating a new instance of Decoder for each block of data
received, which in fact you should create a single instance and use it
for the duration of the connection.

• The code receiving data is decoding from UTF8, but you have other code
that is sending as ASCII. It's not clear at all from the example whether
the receiving part is connected to the sending part, or if these are two
completely independent connections, but if the former it's not a very
good idea to use different encodings at each end. (In reality, you can
decode ASCII using UTF8 and not have a problem, but it's still not a
good idea).

• You're not checking the return value of the Send() call. You need to
see how many bytes were sent, because it is allowed for the Send()
method to send fewer than you asked it to, in which case you need to try
to send the remaining bytes again later.

• You don't call Decoder.GetCharCount() before calling GetChars(). You
need to call that in order to determine the length of the char[] buffer
you need. With UTF8 (the encoding you're using to decode), you cannot
assume that the number of characters and the number of bytes are the same.

• You are not taking the return value of Decoder.GetChars() into account
when you convert to string and append to your message.

The first problem above _might_ be causing your problem, but I don't
really know. It depends on what text you're actually sending. I think
it's most likely that the last problem I describe above is the real
issue: you're creating a String instance that has nulls embedded in it.
So the string gets concatenated properly as it's received, but because
it has null characters in it, you only see on output the very first
characters that were received.

Alternatively, it may be that you never see the terminator because
you're examining the end of the string, which is always padded with null
characters, again because of that same problem. So the magic character
pair is sent and received, but you never detect it because it's not
where your code needs it to be.

The other issues are things you need to fix, but which are probably not
causing serious problems in the basic example.

If you provide a concise-but-complete code example that reliably
reproduces the problem, maybe more specific answers can be provided.
Note that "concise" means that the code example contains only the _bare
minimum_ of code required in order to reproduce the problem. Nothing else.

Pete

Thanks for your response Pete.
Most of the problems you have listed are problems with the original code
that I have copied from the example, I've only made a small addition
myself (events & ReceivedString).
I'll try fixing the points you have mentioned and see how it goes.

Thanks again

 

[Moiv]

Hi, I am building an Async socket server class in Visual C# express
2008.
I am using the example here
http://www.codeguru.com/csharp/csharp/cs_network/sockets/article.php/c7695



What I'm trying to do is build a string from received data and upon
receipt of an end string identifier (in this case "#*") raise an event
which my main program will have an event handler to do something with
the string. The problem is ReceivedString only ever gets the first
characer received added to it and the following chars are not appended.
I'm assuming this has something to do with threading so I've locked the
code that appends to the ReceivedData string but this doesnt help.

What am I doing wrong?

Unfortunately, your code example is far too cluttered for it to be
practical for me to spend time fully analyzing it. At the same time,
it's not even a complete code example, so there's no way to tell just
how your sockets are getting connected.

But here are some problems that I noticed just skimming through your
code:

• You are creating a new instance of Decoder for each block of data
received, which in fact you should create a single instance and use it
for the duration of the connection.

• The code receiving data is decoding from UTF8, but you have other code
that is sending as ASCII. It's not clear at all from the example whether
the receiving part is connected to the sending part, or if these are two
completely independent connections, but if the former it's not a very
good idea to use different encodings at each end. (In reality, you can
decode ASCII using UTF8 and not have a problem, but it's still not a
good idea).

• You're not checking the return value of the Send() call. You need to
see how many bytes were sent, because it is allowed for the Send()
method to send fewer than you asked it to, in which case you need to try
to send the remaining bytes again later.

• You don't call Decoder.GetCharCount() before calling GetChars(). You
need to call that in order to determine the length of the char[] buffer
you need. With UTF8 (the encoding you're using to decode), you cannot
assume that the number of characters and the number of bytes are the
same.

• You are not taking the return value of Decoder.GetChars() into account
when you convert to string and append to your message.

The first problem above _might_ be causing your problem, but I don't
really know. It depends on what text you're actually sending. I think
it's most likely that the last problem I describe above is the real
issue: you're creating a String instance that has nulls embedded in it.
So the string gets concatenated properly as it's received, but because
it has null characters in it, you only see on output the very first
characters that were received.

Alternatively, it may be that you never see the terminator because
you're examining the end of the string, which is always padded with null
characters, again because of that same problem. So the magic character
pair is sent and received, but you never detect it because it's not
where your code needs it to be.

The other issues are things you need to fix, but which are probably not
causing serious problems in the basic example.

If you provide a concise-but-complete code example that reliably
reproduces the problem, maybe more specific answers can be provided.
Note that "concise" means that the code example contains only the _bare
minimum_ of code required in order to reproduce the problem. Nothing
else.

Pete

Thanks for your response Pete.
Most of the problems you have listed are problems with the original code
that I have copied from the example, I've only made a small addition
myself (events & ReceivedString).
I'll try fixing the points you have mentioned and see how it goes.

Thanks again

Pete, if you're interested to know the outcome it looks like it was
nulls in the string. For some reason the example was creating a data
buffer of only 1 byte in the SocketPacket class and using iRx + 1 for
the byte length for the char buffer in the OnDataReceived callback so I
assume that means I was getting a null after each byte. I changed the
buffer in SocketPacket to 1024 bytes and got rid of the + 1 and now it
works a treat.

I'm also using UTF8 at both ends which only makes sense!

I don't think calling Decoder.GetCharCount() is needed as the
Decoder.GetChars() overload that I'm using only requests the byte length
which is stored in iRx from the EndReceive() call.

Having a single instance of the decoder is a good solution to string
building as it retains state information between calls so I will use
that instead of building a ReceivedString and flush it when I need to.

All that's left is to do send() checking on the client code.

Thanks again for your assistance, much appreciated.

 

[Moiv]

Pete, if you're interested to know the outcome it looks like it was
nulls in the string.

Thanks for confirming my guess.

For some reason the example was creating a data
buffer of only 1 byte in the SocketPacket class and using iRx + 1 for
the byte length for the char buffer in the OnDataReceived callback so I
assume that means I was getting a null after each byte. I changed the
buffer in SocketPacket to 1024 bytes and got rid of the + 1 and now it
works a treat.

For now. You are getting lucky that your sends are not getting
regrouped, as is allowed (and happens commonly) when using the TCP
protocol.

Like I said, the correct way to fix the issue is to actually take into
account the number of actual characters decoded.

I'm also using UTF8 at both ends which only makes sense!

Using UTF8, it is especially important to check the number of characters
decoded.

I don't think calling Decoder.GetCharCount() is needed as the
Decoder.GetChars() overload that I'm using only requests the byte length
which is stored in iRx from the EndReceive() call.

The byte length is not the same as the number of characters. It is
actually wrong to set your char[] buffer to the length of the received
number of bytes (or even that number plus one), but GetChars() doesn't
care as long as the buffer is long enough. But the String(char[])
constructor does care; it will use every character that is in the array.

The first time you send a character that is not within the ASCII range,
your char[] buffer will be longer than needed, and will have null
characters at the end, putting you back to the problem you were having
before.

You either need to set the char[] buffer to the correct length by
calling GetCharCount() to determine the correct length, or you need to
pass the "charLen" as the length to the String(char[], int, int)
constructor overload.

Having a single instance of the decoder is a good solution to string
building as it retains state information between calls so I will use
that instead of building a ReceivedString and flush it when I need to.

The Decoder and your string accumulation are two completely different
issues. You can't replace string accumulation with the Decoder class,
unless you instead are accumulating bytes instead.

It is better to use StringBuilder than to keep concatenating strings.
But neither relates at all to the need to use the Decoder class and to
use a single instance of the Decoder class. And assuming you are
decoding the bytes correctly, it doesn't matter whether from a
correctness standpoint whether you use string concatenation or
StringBuilder.

Pete

I see where I've gone wrong with the char[] buffer. I was thinking the
length I need to pass to the char[] buffer was the byte length, I see
now that it is supposed to be the number of char's required.

I'll use the StringBuilder class as you have suggested. That looks like
a more correct approach than concatenating strings.

The only thing that leaves is the regrouping issue that you have
mentioned. I was thinking of having my client terminating all strings
with an EOT control char at the end? Then on the server it looks for the
EOT char.
I think that would work fine if things are regrouped but still sent in
order. How on earth would I be able to regroup things if they are not
sent in their original order? IIRC the TCP layer itself will make sure
the data transfer is ordered, so I shouldnt need to worry about out of
order data?