Bryce Wagner Bryce Wagner - 1 month ago 17
C# Question

Asynchronous NetworkStream continuous reading design pattern

I'm trying to convert a class which uses multiple threads to use overlapping I/O. It's almost working, but it seems to randomly hit a threading issue, and I'm not sure why.

Too much code to post directly, but here's the basic pattern. The goal is to sit there reading data off the connection until the connection get disposed, so when each

EndRead()
completes, it should start a new
BeginRead()
.

public enum State
{
Idle,
BeforeRead,
PendingRead,
FinishingRead,
Died,
}

private int state;
private IAsyncResult asyncResult;
private byte[] readBuffer = new byte[4096];
private System.Net.Sockets.NetworkStream stream;
public void Connect(System.Net.Sockets.TcpClient client, string host, int port)
{
client.Connect(host, port);
this.stream = client.GetStream();
}

private bool SetState(State expectedState, State newState)
{
return Interlocked.CompareExchange(ref this.state, (int)newState, (int)expectedState) == expectedState;
}
public void BeginRead()
{
try
{
while (true)
{
if (!SetState(State.Idle, State.BeforeRead))
return;
IAsyncResult async;
async = stream.BeginRead(readBuffer, 0, readBuffer.Length, x => EndRead(true), null);
if (async == null)
return;
SetState(State.BeforeRead, State.PendingRead);
lock (this)
this.asyncResult = async;
if (async.AsyncWaitHandle.WaitOne(0))
EndRead(false);
}
}
catch { this.state = State.Died; }
}
private void EndRead(bool asynchronousCallback)
{
try
{
if (!SetState(State.PendingRead, State.FinishingRead))
return;
IAsyncResult async;
lock (this)
{
async = this.asyncResult;
this.asyncResult = null;
}
if (async == null)
return;
int bytesRead = stream.EndRead(async);
HandleData(bytesRead, readBuffer);
SetState(State.FinishingRead, State.Idle);
if (asynchronousCallback)
BeginRead();
}
catch { this.state = State.Died; }
}


Most of the time it works, but occasionally it does one of several things:


  • Stops receiving messages

  • Throws an exception that I the
    asyncResult has already been handled: "EndReceive can only be called once for each asynchronous operation"
    .



I should also mention that there is synchronous writing going on from another thread (stream.Write, not
stream.BeginWrite
). I think reading and writing should be independent of each other, so it shouldn't affect the behavior.

Is my design fundamentally flawed? This is a stripped down example, so it's possible the stuff I stripped out could be causing the problem, but I need to know if my basic design is valid or not. What is the proper way to chain read asynchronously?

(And in case the suggestion is to use
async/await
, this code needs to run on Windows XP, so that's not an option.)

Answer

You have a race condition:

        IAsyncResult async;
        async = stream.BeginRead(readBuffer, 0, readBuffer.Length, x => EndRead(true), null);

        /* Race condition here */   

        if (async == null)
            return;
        SetState(State.BeforeRead, State.PendingRead);
        lock (this)
            this.asyncResult = async;

Your EndRead can execute before the SetState and/or before the this.asyncResult = async executes. You cannot do this. The state must be set before the BeginRead is issued, and reset in case of failure. Do not retain and use a member asyncResult, but instead pass the callback to BeginRead and get the async result on the callback:

  SetState(State.BeforeRead, State.PendingRead);
  stream.BeginRead(readBuffer, 0, readBuffer.Length, EndRead);

 ...

  private void EndRead(IAsyncResult asyncResult) {
     int bytesRead = stream.EndRead(asyncResult);
     ...
  }
Comments