Async / await, TAP and EAP

Question

I'm trying to move from async socket code which is a bit messy (BeginSend/EndSend, BeginReceive/EndReceive and lots of internal 'administration') to an async TcpClient. I'm pretty new to async/await, so please bear with me...

Assume the following code (irrelevant code stripped):

public async void StartReceive()
{
    while (true)
    {
        var stream = this.MyInternalTcpClient.GetStream();
        if (stream == null) return;

        var buffer = new byte[BUFFERSIZE];
        var bytesread = await stream.ReadAsync(buffer, 0, BUFFERSIZE);
        if (bytesread == 0)
        {
            if (Closed != null)
                Closed(this, new ClosedEventArgs());
            return;
        }

        var message = this.Encoding.GetString(buffer, 0, bytesread);
        this.MyInternalStringBuilder.Append(message);
        // ... message processing here ...

        foreach (var p in parts) {
            //Raise event per message-"part"
            if (MessageReceived != null)
                MessageReceived(this, new MessageReceivedEventArgs(p));
        }
    }
}

My class has an internal stringbuilder that gets appended to each time data is received (this is because messages can be split up in more than one receive 'event'). Then, when some conditions are met, the stringbuilder (the "running buffer") is processed and the message is split into message-"parts". For each "part" an event is raised. Many instances of this class can be running in the system.

My questions are:

1. Am I correct in assuming/understanding the MyInternalStringBuilder.Append is never called "out of order"? Each time the TcpListener receives data it will be added, "in order", to the (internal) stringbuilder? I don't need to use locks?
2. Since this StartReceive method uses an internal ("infinite") loop and raises events I don't see the point in making the StartReceive method async, but I have to (for obviously being able to use await at all). I know I'm mixing TAP/EAP but I have to for reasons not relevant to this question. However, it feels "dirty" since "async shouldn't be void" is what I gathered so far. Maybe there's a better way to solve this (except for moving to TAP alltogether)?

Answer 1

Am I correct in assuming/understanding the MyInternalStringBuilder.Append is never called "out of order"? Each time the TcpListener receives data it will be added, "in order", to the (internal) stringbuilder? I don't need to use locks?

That is correct. async code is naturally sequential though asynchronous. So, even though under the covers StartReceive keeps returning and resuming, it will not resume multiple times simultaneously.

Since this StartReceive method uses an internal ("infinite") loop and raises events I don't see the point in making the StartReceive method async, but I have to (for obviously being able to use await at all). I know I'm mixing TAP/EAP but I have to for reasons not relevant to this question. However, it feels "dirty" since "async shouldn't be void" is what I gathered so far. Maybe there's a better way to solve this (except for moving to TAP alltogether)?

I'm not a big fan of async void. First off, I assume you have a top-level try/catch within StartReceive that will initiate a socket shutdown if any exception is observed (if not, then you need one). Personally, I would write it as an async Task method and consider exposing the Task as a property (if you have events all set up for error handling as well, then you don't need the Task property).

There is one other thing to watch out for; there are problems with the strict read-only/write-only loop that socket newbies often write: during the read-only portion, the reader cannot detect a half-open scenario; and during the write-only portion, there is a (tiny) chance of deadlock, particularly with a malicious client. Ideally, a socket should always have a read operation going (which you do), and it should also periodically be sending something (e.g., keepalives). I have a sockets FAQ that goes into more detail.

To fix this, I recommend that you add ConfigureAwait(false) to all awaits, and post your EAP events to a SynchronizationContext captured in the constructor (using new SynchronizationContext() if there isn't one). Then you'll need another independent loop of some kind that either sends keepalive messages periodically (if the protocol allows for it), or terminates the socket if it hasn't read anything for a period of time.

Answer 2

Completing Remus' answer, you might want to ensure that StartReceive can only be called once, or you're going to have serious problems and locks will be needed.

Regarding the void return, I personally think that's one of the cases this is fine, "top-level" asynchronous methods starting with Start or Begin, correctly conveying the meaning that this is a fire-and-forget method. That said, if you have such a method, you probably want to redesign things.

To do this, I'll use a library such as TPL Dataflow, where different actions are represented as asynchronous blocks. In your case, there will be a "Read from socket block" → "Process block" → "Send response block", each one triggered asynchronously after the other, allowing you to continue reading while you're processing. By doing this, you won't have this big loop, and the void return won't exist anymore. Many things have to be changed though.

Answer 3

Yes, it will be in order. Because you only have one active buffer posted*, then you process it before your post the next one, event order is deterministic (post->receive->process->post->receive...). There is no need for locking if you only use the SB in this loop as you described.

However, there are obviously big 'if's with regard to what happens in the MessageReceived event. Assuming you do the decent thing (eg. process and post a response), should be OK. If you attempt to receive more from the event all hell will break loose (eg. send a response and then wait for response to the response, that will be bad). If your processing is event driven state machine ('received message 'foo' in state 'bar, respond with 'spam' and change state to 'bam', return into loop wait for more events) then it normally should be OK. Obviously, is hard to give a verdict w/o code, is all base don your claims and in my understanding of what you understand by making that claims (that is OK, you seem to know what you're talking about).

The processing you describe is not the fastest possible (because incoming bytes have no room to buffer while you process current buffer) but achieving high throughput would be a lot more tricky, exactly because of the order issue(s) you hint at. Besides, if the traffic is request-response then it really doesn't matter much.

posted buffer: a buffer was presented to the network to fill it up. In .Net there are about 1 Million layers of abstraction between the stream operation and AFD/tcp.sys, but the concept applies pretty much the same.

Answer 4

You definitely need to use locks if you share the StringBuilder between multiple threads.

Also, I don't understand what you mean by in order. Append will be called in an order. It may not be the order you want, but then there is nothing in this code that ensures any particular order.

I can say that the calls to Append inside that method will be called in the order the code executes them, which should behave exactly like you expect from the code, but again, if multiple threads call this at the same time, the order between the threads is not given.

As for your second question, why not simply write the whole thing without using the async keyword, and calling it in a task or a thread instead?