Using async method for message loop in C#

Question

I'm making an online communication application, and I'd like to process messages asynchronously. I found async-await pattern useful in implementing message loop.

Below is what I have got so far:

CancellationTokenSource cts=new CancellationTokenSource(); //This is used to disconnect the client.

public Action<Member> OnNewMember; //Callback field

async void NewMemberCallback(ConnectionController c, Member m, Stream stream){
    //This is called when a connection with a new member is established.
    //The class ConnectionController is used to absorb the difference in protocol between TCP and UDP.

    MessageLoop(c, m,stream,cts.Token);
    if(OnNewMember!=null)OnNewMember(m);
}

async Task MessageLoop(ConnectionController c, Member m, Stream stream, CancellationToken ct){
    MemoryStream msgbuffer=new MemoryStream();
    MemoryStream buffer2=new MemoryStream();

    while(true){
        try{
            await ReceiveandSplitMessage(stream, msgbuffer,buffer2,ct); //This stops until data is received.
            DecodeandProcessMessage(msgbuffer);
        catch( ...Exception ex){
            //When the client disconnects
            c.ClientDisconnected(m);
            return;
        }

    }
}

Then I got some warning saying that in NewMemberCallback, the call to MessageLoop is not awaited. I actually don't need to await the MessageLoop method because the method does not return until the connection is disconnected. Is it considered a good practice to use async like this? I heard that not awaiting an async method is not good, but I also heard that I should eliminate unnecessary await's. Or is it even considered bad to use async pattern for message loop?

Answer 1

Usually you want to keep track of the task(s) you've started, to avoid re-entrancy and handle exceptions. Example:

Task _messageLoopTask = null;

async void NewMemberCallback(ConnectionController c, Member m, Stream stream)
{
    if (_messageLoopTask != null)
    {
        // handle re-entrancy
        MessageBox.Show("Already started!");
        return;
    }

    _messageLoopTask = MessageLoop(c, m,stream,cts.Token);

    if (OnNewMember!=null) OnNewMember(m);

    try
    {
        await _messageLoopTask;
    }
    catch (OperationCanceledException ex)
    {
        // swallow cancelation
    }
    catch (AggregateException ex) 
    { 
        // swallow cancelation
        ex.Handle(ex => ex is OperationCanceledException);
    }
    finally
    {
        _messageLoopTask = null;
    }
}

Check Lucian Wischik's "Async re-entrancy, and the patterns to deal with it".

If you can have multiple MessageLoop instances, then you wouldn't have to worry about re-entrancy, but you'd still want to observe exceptions.

Answer 2

If you don't await MessageLoop(c, m,stream,cts.Token); the method will return as soon as the first await is encountered and then execute the rest of the method. It will be a fire and forget scenario. Exceptions will not be raised on the UI thread, so if c.ClientDisconnected(m); throws it will be raised on a background thread and result in an exception which is explicitly swallowed, because any exceptions stored in the Task returned from the method are not observed. You can find out more about it in this post by @Noseratio

Seems a little unconventional, to be honest.

Is there some better way of determining whether the client has disconnected? You will miss out on any important exceptions that you may want to show to the user or log.