Console chat server

Question

I'm just looking for feedback on correctness of my understanding of async/await. I'm curious about the Task.Run inside of the StartReceive() method. Resharper (or maybe just the compiler) warns against doing this without an await, but I think in this case, it's ok.

After running, one can connect by telnetting to localhost 7776.

using System;
using System.Collections.Generic;
using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Threading.Tasks;

namespace ChatTutorial.ConsoleServer
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                var server = new ChatServer();
                server.Start();
                Console.WriteLine("Listening....");
                Console.ReadLine();
            }
            catch (Exception ex)
            {
                Logger.PrintException(ex, "Main");
            }
        }
    }

    internal class ChatServer
    {
        private readonly TcpListener _listener = new TcpListener(IPAddress.Parse("0.0.0.0"), 7776);            
        private readonly Dictionary<string, LocalClient> _clients = new Dictionary<string, LocalClient>();

        public async void Start()
        {
            try
            {
                _listener.Start();
                while (true)
                {
                    try
                    {
                        Console.WriteLine("Listening...");
                        var client = await _listener.AcceptTcpClientAsync();

                        Console.WriteLine("Client connected");
                        var id = Guid.NewGuid().ToString();
                        _clients.Add(id, new LocalClient(client, ReceivedMessage, ClientClosed, id));
                    }
                    catch (Exception ex)
                    {
                        Console.WriteLine("Start() While() '{0}', '{1}'", ex.Message, ex.StackTrace);
                    }
                }
            }
            catch (Exception ex)
            {
                Logger.PrintException(ex, "Start");
            }
        }

        public void ClientClosed(string id)
        {
            Console.WriteLine("Client {0} disconnected", id);
            var client = _clients[id];
            _clients.Remove(id);
            client.Close();
            Console.WriteLine("Client {0} Closed", id);
            ReceivedMessage("Client disconnected", id);
        }

        public void ReceivedMessage(string message, string id)
        {
            try
            {
                var clientsToRemove = new List<string>();
                foreach (var client in _clients)
                {
                    if (client.Value != null && client.Value.Client.Connected)
                    {
                        if (client.Key != id)
                            client.Value.Send(id + "> " + message);
                    }
                    else
                        clientsToRemove.Add(client.Key);
                }

                foreach (var client in clientsToRemove)
                    _clients.Remove(client);
            }
            catch (Exception ex)
            {
                Logger.PrintException(ex, "ReceivedMessage");
            }
        }

        internal class LocalClient
        {
            private readonly TcpClient _client;
            public TcpClient Client { get { return _client; } }

            private readonly Action<string> _closedCallback;            
            private readonly Action<string,string> _recvCallback;

            private readonly StreamReader _reader;
            private readonly StreamWriter _writer;

            private readonly string _id;
            public string Id { get { return _id; } }            

            public LocalClient(TcpClient client, Action<string,string> recvCallback, Action<string> closedCallback, string id)
            {
                _closedCallback = closedCallback;
                try
                {
                    _client = client;
                    _recvCallback = recvCallback;
                    _id = id;

                    _reader = new StreamReader(_client.GetStream());
                    _writer = new StreamWriter(_client.GetStream()) {AutoFlush = true};

                    StartReceive();

                    Console.WriteLine("Local client {0} receiving...", id);
                }
                catch (Exception ex)
                {
                    Logger.PrintException(ex, "LocalClient");
                }
            }

            public async void StartReceive()
            {
                try
                {
                    while (true)
                    {                        
                        var message = await _reader.ReadLineAsync();

                        if (String.IsNullOrEmpty(message))
                        {
                            Task.Run(() => _closedCallback(_id));
                            return;
                        }

                        _recvCallback(message, _id);
                        Console.WriteLine("{0} > {1}", _id, message);
                    }
                }
                catch (Exception ex)
                {
                    Logger.PrintException(ex, "StartReceive");
                }
            }

            public void Close()
            {
                if (_reader != null)
                    _reader.Dispose();           
            }

            public async void Send(string message)
            {
                try
                {                    
                    if (String.IsNullOrEmpty(message)) return;

                    await _writer.WriteLineAsync(message);                    
                }
                catch (Exception ex)
                {
                    Logger.PrintException(ex, "Send");
                }
            }
        }
    }

    public static class Logger
    {
        public static void PrintException(Exception ex, string methodName)
        {
            Console.WriteLine("{0}: \n '{1}' \n '{2}'", methodName, ex.Message, ex.StackTrace);
        }
    }
}

Answer 1

As the latecomer to the party, I'll take it from V3...

First, async void should only be used for event handlers. I'd much rather see Start return a Task representing the listening loop.

For a simple example, you don't need to do any cleanup at all. Once your app exits, the OS will clean up after it. Doing cleanup just before application exit is just a waste of effort.

If you do want to cleanly stop an asynchronous system, you should be using CancellationToken - create a CTS in Main and then pass the token to Start. From there things get tricky, as many operations (such as AcceptTcpClientAsync) do not take a CancellationToken. There's an old trick in Windows programming where you can close the underlying handle (in this case, Stop the TcpListener) which will cause any outstanding asynchronous operations on that handle to complete with an error.

On a side note, it is almost impossible to shut down a socket without seeing exceptions; the general rule of thumb is that once you decide to shut down, just ignore all errors until it's done.

You can find out more about socket programming in my TCP/IP .NET Sockets FAQ. However, I highly recommend that you find another sample project to explore async and await (if you like to learn by example, there's a good tutorial built in to LINQPad). There is no such thing as a "simple" TCP/IP chat server. I've had many people ask me for sample async socket code, but I haven't given any out for a very simple reason: it's not as hard as it used to be, but it's still really hard to get right!

Answer 2

static void Main(string[] args)

If you're not using args, you can just remove them: static void Main().

server.Start();
Console.WriteLine("Listening....");
Console.ReadLine();

Running a server until Enter is pressed like this might be okay in a testing application, but not in a production-quality one. Probably the biggest problem with this is that you don't give the worker thread any chance to complete the current operation or to perform cleanup.

To fix that, you could change your application into a Windows service. Or you could use a CancellationToken that's somehow triggered by the user's action and then wait for the whole Task to complete.

catch (Exception ex)
{
    Logger.PrintException(ex, "Main");
}

I don't see the point of this in your Main(). I can't think of any situation where this catch would be triggered.

Also, since you're using C# 5.0, you could use caller information to avoid having to spell out the name of the current method.

new TcpListener(IPAddress.Parse("0.0.0.0"), 7776)

According to the documentation, you should use IPAddress.Any if you don't want to specify which IP address to use. That is equivalent to your code (Any returns 0.0.0.0), but I think using Any would make your code clearer.

public async void Start()

Normally, you should use async void methods only in event handlers and nowhere else. I guess it could be considered okay here (since you log all exceptions from this method), assuming you don't care about the cleanup problems I mentioned above.

public void ClientClosed(string id)

I don't see any reason to use just id here. Instead, the parameter of this method should be the LocalClient directly.

_clients.Remove(id);

This line could be run in parallel with other calls to Remove() or with the call to Add() in Start(), which means the code is not thread-safe. You should either use locking or use ConcurrentDictionary instead of normal Dictionary.

foreach (var client in _clients)
{
    if (client.Value != null && client.Value.Client.Connected)

This is very confusing naming. Here, client is not any kind of client, it's a KeyValuePair<LocalClient>. And client.Value.Client is a TcpClient, which is not clear from the name at all. I would suggest you to rename LocalClient to something else. If you think that it's the best name you can come up with, use names to make the distinction between LocalClient and TcpClient clear.

---

There are probably other issues with the rest of the code, but this is where I stopped looking.