Async TcpListener

Question

I'm trying to implement a TcpListener using the TPL. I'm not sure If I'm doing everything correctly though. I'm trying to keep the code as small as possible as well. It all works fine in a console application.

public async Task RunAsync(CancellationToken cancellationToken)
{
    var listener = new TcpListener(ip, port);
    listener.Start();

    while (!cancellationToken.IsCancellationRequested)
    {
        await AcceptClientAsync(listener, encoding, progress, cancellationToken);
    }
    listener.Stop();
}

Here is the AcceptClientAsync method: (Sorry for the weird formatting. StyleCop likes to do that.)

private async Task AcceptClientAsync(TcpListener tcpListener, Encoding encoding, IProgress<string> progress, CancellationToken cancellationToken)
    {
    var client = await tcpListener.AcceptTcpClientAsync();
    this.Connection = new ConnectionState { TcpClient = client };
    while (client.Connected && !cancellationToken.IsCancellationRequested)
    {
        await
            this.ReadStringAsync(this.Connection, encoding, progress)
                .ContinueWith(
                    task =>
                    progress.Report(
                        string.Format("Client {0} disconnected.", Connection.TcpClient.Client.RemoteEndPoint)),
                    TaskContinuationOptions.OnlyOnFaulted);
    }
}

And the ReadStringAsync method:

    private async Task ReadStringAsync(ConnectionState connection, Encoding encoding, IProgress<string> progress)
    {
        var stream = connection.TcpClient.GetStream();
        if (connection.Read > 0)
        {
            var encoded = encoding.GetString(connection.Buffer, 0, connection.Read);
            connection.StringBuilder.Append(encoded);
        }

        var decoded = connection.StringBuilder.ToString();
        if (decoded.Contains("<End>"))
        {
            progress.Report(decoded.Replace("<End>", string.Empty));
        }

        connection.Read = await stream.ReadAsync(connection.Buffer, 0, connection.Buffer.Length);
    }

I'd also like to keep using the IProgress<> interface and support cancellation via CancellationTokens.

Answer 1

1. If you immediately await everything, you're not going to get any concurrency. This means that at any moment, there can be only one connection to this listener. Is this intentional?

2. Because of the way TCP works, the Connected property may return true even if the other side already disconnected. This means that you should send keep-alive packets, even if all you logically want to do is reading.

3. ContinueWith() is useful only rarely when you can use async. OnlyOnFaulted can be easily rewritten using try-catch. Though you should be catching only the specific exception that you need, not all of them. So your code could look something like:

   try
   {
       await this.ReadStringAsync(this.Connection, encoding, progress);
   }
   catch (NotSureWhichException ex)
   {
       progress.Report(
           string.Format("Client {0} disconnected.", Connection.TcpClient.Client.RemoteEndPoint));
   }
   

4. I think the way you're dealing with <End> is wrong. If there is always going to be only one message per connection, then you should somehow indicate that after reading <End>. If there can be multiple messages, then an end of one message and start of the following one could be read in the same ReadAsync(), which means you're not going to report partial message.