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

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 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.