4
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$
  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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – shredder8910 Feb 22 '14 at 2:13
  • \$\begingroup\$ 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). \$\endgroup\$ – svick Feb 22 '14 at 12:57
  • \$\begingroup\$ 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. \$\endgroup\$ – svick Feb 22 '14 at 12:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.