HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Async TcpListener

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
asynctcplistenerstackoverflow

Problem

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 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 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(""))
{
progress.Report(decoded.Replace("", string.Empty));
}

connection.Read = await stream.ReadAsync(connection.Buffer, 0, connecti

Solution

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

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

-
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));
}


-
I think the way you're dealing with ` is wrong. If there is always going to be only one message per connection, then you should somehow indicate that after reading . 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.

Code Snippets

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

Context

StackExchange Code Review Q#42480, answer score: 4

Revisions (0)

No revisions yet.