Asynchronously reading off a TcpSocket the best way

[Dave A]

I have an application that does lots of socket communications all asynchronously via the TcpClient class.

The code has been working 99.9999% of the time (yeah one of those bugs) but occasionally the receiving thread would get 'stuck'.

The pattern that I have used from the reading is quite common...

AutoResetEvent waitForReadToComplete;
NetworkStream stream;

.... code to start the socket, send some data and then read the data by calling...

private void ReceiveDataOfTheSocket()
{
try
{

// Convert the request into a byte array
waitForReadToComplete = new AutoResetEvent(false);

IAsyncResult ar = stream.BeginRead(receiveBytes, 0, tcpClient.ReceiveBufferSize, new AsyncCallback(ReadAsync), stream);

// Wait for the receive to complete...
waitForReadToComplete.WaitOne();

...

}
catch(Exception exp)
{
.. does something meaningful
}

}



private void ReadAsync(IAsyncResult ar)
{
try
{
... read data off the socket

if (amountOfDataReadOfTheSocket > 0)
{
// Recall this routine
streamAsync.BeginRead(receiveBytes, 0, tcpClient.ReceiveBufferSize, new AsyncCallback(ReadAsync), streamAsync);

}
else
{
waitForReadToComplete.Set();
}

}
catch(Exception exp)
{
.. do some stuff and then..
waitForReadToComplete.Set();
}
}

After maybe a day or two of running successfully the application would appear to be stopped. In the debugger I was able to see that one thread was stopped at waitForReadToComplete.WaitOne(); in the ReceiveData() function yet there was no other thread running in ReadAsync(). There were other threads in the application but they were not affecting this code and were working as expected. Thread starvation was not an issue.

It was as if .BeginRead() had been called successfully but either ReadAsync() had either not been run or it had run and somehow waitForReadToComplete.Set(); had not been called despite the exception handler.

I took a bit of a guess and thought that BeginRead() had somehow failed yet no exception had been thrown.

I changed the line...

waitForReadToComplete.WaitOne();

to

while ((waitForReadToComplete.WaitOne(5000, true) == false) && (ar.IsCompleted == false))
{
// Do nothing
}

....which effectively waits for the asyncread to complete or the ar.IsCompleted to be set to true - whatever comes first.

The program has now been running for days without a problem.

I am not one to say "lets not worry about it since it is working". I want to know why this as.IsCompleted is required when waitforReadToComplete.WaitOne() should be more than sufficient.

This program is following the HTTP protocol. The only thing that I can think of is that the HTTP send has worked and then the .BeginRead() is called. At this moment the socket is connected so no exception is thrown. However, it is then closed in the moments thereafter and ReadAsync() is therefore never called. The only indication is that ar.IsCompleted is set to true.

Another idea that I have is that the MSDN help suggests that the code in ReadAsync() that re-sets up the async read should be...

// message received may be larger than buffer size so loop through until you have it all.
while(myNetworkStream.DataAvailable){

myNetworkStream.BeginRead(myReadBuffer, 0, myReadBuffer.Length,
new AsyncCallback(ReadAsync),
myNetworkStream);

}


Maybe if I did that then it would work better. ...but I can't see this as being very good code. It is saying that in the place where data is received off the socket, repeatedly asynchronously re-read off the socket and make a callback to this routhine until you have read all of the data. Surely the "while(myNetworkStream.DataAvailable){" is totally redundant and possibly quite foolish?

Is there a better way to code this? Does anyone have any better ideas about what maybe causing it or how to check for it? So many questions.

Thanks in advance.

Dave A

 

[Vadym Stetsyak]

Where are you calling stream.EndRead(...)?

streamAsync.BeginRead(...) in the ReadAsync(...) method returns value, why
there should be exception.
You can add finally {} block

The code you've posted can be replaced with the synchronous equivalent
without no loss to performance.
Let me explain why.

After you issue stream.BeginRead(...) in the ReceiveDataOfTheSocket(), you
start waiting on the event, that will signal the end of receive, right?

So, ReceiveDataOfTheSocket would block until data is read. If that is the
behavior you expect - it is far more simpler to call stream.Read(...) until
no data will be available on the srtream.
This approach will is free of deadlocks and threading issues.

--
Vadym Stetsyak aka Vadmyst
http://vadmyst.blogspot.com

I have an application that does lots of socket communications all
asynchronously via the TcpClient class.

The code has been working 99.9999% of the time (yeah one of those bugs) but
occasionally the receiving thread would get 'stuck'.

The pattern that I have used from the reading is quite common...

AutoResetEvent waitForReadToComplete;
NetworkStream stream;

.... code to start the socket, send some data and then read the data by
calling...

private void ReceiveDataOfTheSocket()
{
try
{

// Convert the request into a byte array
waitForReadToComplete = new AutoResetEvent(false);

IAsyncResult ar = stream.BeginRead(receiveBytes, 0,
tcpClient.ReceiveBufferSize, new AsyncCallback(ReadAsync), stream);

// Wait for the receive to complete...
waitForReadToComplete.WaitOne();

...

}
catch(Exception exp)
{
.. does something meaningful
}

}



private void ReadAsync(IAsyncResult ar)
{
try
{
... read data off the socket

if (amountOfDataReadOfTheSocket > 0)
{
// Recall this routine
streamAsync.BeginRead(receiveBytes, 0,
tcpClient.ReceiveBufferSize, new AsyncCallback(ReadAsync), streamAsync);

}
else
{
waitForReadToComplete.Set();
}

}
catch(Exception exp)
{
.. do some stuff and then..
waitForReadToComplete.Set();
}
}

After maybe a day or two of running successfully the application would
appear to be stopped. In the debugger I was able to see that one thread was
stopped at waitForReadToComplete.WaitOne(); in the ReceiveData() function
yet there was no other thread running in ReadAsync(). There were other
threads in the application but they were not affecting this code and were
working as expected. Thread starvation was not an issue.

It was as if .BeginRead() had been called successfully but either
ReadAsync() had either not been run or it had run and somehow
waitForReadToComplete.Set(); had not been called despite the exception
handler.

I took a bit of a guess and thought that BeginRead() had somehow failed yet
no exception had been thrown.

I changed the line...

waitForReadToComplete.WaitOne();

to

while ((waitForReadToComplete.WaitOne(5000, true) == false) &&
(ar.IsCompleted == false))
{
// Do nothing
}

....which effectively waits for the asyncread to complete or the
ar.IsCompleted to be set to true - whatever comes first.

The program has now been running for days without a problem.

I am not one to say "lets not worry about it since it is working". I want
to know why this as.IsCompleted is required when
waitforReadToComplete.WaitOne() should be more than sufficient.

This program is following the HTTP protocol. The only thing that I can
think of is that the HTTP send has worked and then the .BeginRead() is
called. At this moment the socket is connected so no exception is thrown.
However, it is then closed in the moments thereafter and ReadAsync() is
therefore never called. The only indication is that ar.IsCompleted is set
to true.

Another idea that I have is that the MSDN help suggests that the code in
ReadAsync() that re-sets up the async read should be...

// message received may be larger than buffer size so loop through until
you have it all.
while(myNetworkStream.DataAvailable){

myNetworkStream.BeginRead(myReadBuffer, 0, myReadBuffer.Length,
new AsyncCallback(ReadAsync),
myNetworkStream);

}


Maybe if I did that then it would work better. ...but I can't see this as
being very good code. It is saying that in the place where data is received
off the socket, repeatedly asynchronously re-read off the socket and make a
callback to this routhine until you have read all of the data. Surely the
"while(myNetworkStream.DataAvailable){" is totally redundant and possibly
quite foolish?

Is there a better way to code this? Does anyone have any better ideas about
what maybe causing it or how to check for it? So many questions.

Thanks in advance.

Dave A

 

[Dave A]

Vadym,

The code posted was abbreviated somewhat for the sake of brevity.

I am calling "stream.EndRead(...)" as part of the note " ... read data off
the socket" in the function ReadAsync(). The details of this code are
irrelevant to the conversation at hand and were omitted to highlight the
important aspects.

It was written asynchronously since I require this function to exhibit an
optional timeout behaviour. Again it was not explicitly described in the
example code. When a timeout is specified I pass this timeout into the
WaitOne() function. If I were to have implemented a synchronous mode of
operation as you suggested than an extra thread would need to automatically
close the socket if the timeout expires. This pattern is not as elegant as
the one described in my code.

Having said all of that everyone of my original questions remain.

Regards
Dave A

 

[Vadym Stetsyak]

One of the reasons why the call to BeginRead(...) hasn't ended up with
exception is that operation was scheduled for completion.
When you call on BeginRead in Network stream BeginReceive(...) of the
underlying socket is called. After that unmanaged WSARecv func is called.
This func can return either 0 or SOCKET_ERROR with underlying "I/O pending"
error. In this case BeginRead/BeginReceive will not return exception.

So, to handle this correctly you can either check IsCompleted of
IAsyncResult or DataAvailable property of the NetworkStream

About the way how to improve the code: instead of
while ((waitForReadToComplete.WaitOne(5000, true) == false) &&
(ar.IsCompleted == false))
{
// Do nothing
}

you can use ar.AsyncWaitHandle.WaitOne(5000, true), it is more elegant and
you do not need additional wait object ( waitForReadToComplete ).

private void ReceiveDataOfTheSocket()
{
try
{
// Convert the request into a byte array
IAsyncResult ar = stream.BeginRead(receiveBytes, 0,
tcpClient.ReceiveBufferSize, new AsyncCallback(ReadAsync), stream);

// Wait for the receive to complete...
ar.AsyncWaitHandle.WaitOne(5000, true),
...
}
catch(Exception exp)
{
.. does something meaningful
}
}

 

[Dave A]

I agree with you.

If we are right then the MSDN help at
http://msdn2.microsoft.com/system.net.sockets.networkstream.beginread.aspx
in the context of reading data off a socket is probably incorrect. It
simply does the .WaitOne() (and on its own WaitHandle instead of the
ar.AsyncWaitHandle)

So is the Help wrong? and what do we need to do get it fixed?

Dave A

 

[Vadym Stetsyak]

Another MS doc says that
"The return value allows the client to wait for an asynchronous operation to
complete instead of polling IsCompleted until the operation concludes. The
return value can be used to perform a WaitOne, WaitAny, or WaitAll
operation."

(
http://msdn.microsoft.com/library/d...stemIAsyncResultClassAsyncWaitHandleTopic.asp )

So, the best way will be to make code change and test :