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.

• shredder8910: So about #1, perhaps I should use a ManualResetEvent to signal to the main thread to listen for another connection if one is active? Also, how often should I send Keep-Alive packets?
• svick: I don't think MRE is a good solution here, especially since it's not async. What I would do is to move the while loop from AcceptClientAsync() to a separate method and not await that. But if you do that, be very careful about exceptions (i.e. you should probably add a catch around that while).
• svick: Regarding the timing of keep alive packets, that depends on you, specifically, on how soon do you want to learn about the other side disappearing.