2
\$\begingroup\$

I'm working on an asynchronous TCP server which is supposed to be used for a chat. I want to get a review for the TCP server part because I'm struggling to find a good design to fulfill the following technical requirements.

Technical Requirements

  • Start()
  • Stop()
  • Dispose() - disconnect all connected clients.
  • StartReceiving() - start receiving messages from all connected clients
  • SendMessage() - send message to a specific connected client
  • SendMessageToAll() - send message to all connected clients
  • List of the connected clients (thread-safe) - for ex. clients should be able to send a message to all connected clients
  • Is Server Running (boolean)
  • CancellationToken
  • Exception Handling
  • TcpNetServer class should be capable of being inherited, so custom EventHandlers can be added

Known bugs

  • not able to connect more than one client at the same time
  • EventHandler.ClientDisconnected is currently unused, because the current code is bad and doesn't handle properly the exceptions and/or StopAsync() / Dispose().
  • I'm struggling to implement SendMessage() and SendMessageToAll(), because of the current code

TCP server (review this)

public interface IServer : IDisposable
{
}

/// <summary>
///     Event-driven TCP Server implementation.
/// </summary>
public class TcpNetServer : IServer
{
    private readonly ILogger<TcpNetServer> _logger;
    private readonly TcpListener _tcpListener;
    private CancellationTokenSource? _cts;

    /// <summary>
    ///     Initializes a new instance of the <see cref="TcpNetServer" /> class.
    /// </summary>
    /// <param name="address">An <see cref="IPAddress" /> that represents the local IP address.</param>
    /// <param name="port">The port on which to listen for incoming connection attempts.</param>
    public TcpNetServer(IPAddress address, int port)
        : this(NullLoggerFactory.Instance, new IPEndPoint(address, port))
    {
    }

    /// <summary>
    ///     Initializes a new instance of the <see cref="TcpNetServer" /> class.
    /// </summary>
    /// <param name="localEndpoint">
    ///     An <see cref="IPEndPoint" /> that represents the local endpoint to which to bind the
    ///     listener.
    /// </param>
    public TcpNetServer(IPEndPoint localEndpoint) : this(NullLoggerFactory.Instance, localEndpoint)
    {
    }

    /// <summary>
    ///     Initializes a new instance of the <see cref="TcpNetServer" /> class.
    /// </summary>
    /// <param name="loggerFactory"></param>
    /// <param name="address">An <see cref="IPAddress" /> that represents the local IP address.</param>
    /// <param name="port">The port on which to listen for incoming connection attempts.</param>
    public TcpNetServer(ILoggerFactory loggerFactory, IPAddress address, int port)
        : this(loggerFactory, new IPEndPoint(address, port))
    {
    }

    /// <summary>
    ///     Initializes a new instance of the <see cref="TcpNetServer" /> class.
    /// </summary>
    /// <param name="loggerFactory"></param>
    /// <param name="localEndpoint">
    ///     An <see cref="IPEndPoint" /> that represents the local endpoint to which to bind the
    ///     listener.
    /// </param>
    /// <exception cref="ArgumentNullException"></exception>
    public TcpNetServer(ILoggerFactory loggerFactory, IPEndPoint localEndpoint)
    {
        if (loggerFactory == null)
        {
            throw new ArgumentNullException(nameof(loggerFactory));
        }

        if (localEndpoint == null)
        {
            throw new ArgumentNullException(nameof(localEndpoint));
        }

        _logger = loggerFactory.CreateLogger<TcpNetServer>();

        _tcpListener = new TcpListener(localEndpoint);
    }

    private ConcurrentBag<TcpClient> ConnectedClients { get; } = new();

    internal Task Running { get; private set; } = Task.CompletedTask;

    #region Dispose method implementation

    /// <inheritdoc />
    public void Dispose()
    {
        foreach (var client in ConnectedClients)
        {
            client.Close();
        }
    }

    #endregion

    /// <summary>
    ///     Event that is fired when a client connects to the server.
    /// </summary>
    public event EventHandler<ConnectedEventArgs>? ClientConnected;

    /// <summary>
    ///     Event that is fired when a client disconnects from server.
    /// </summary>
    public event EventHandler? ClientDisconnected;

    /// <summary>
    ///     Event that is fired when server receives data from a client.
    /// </summary>
    public event EventHandler<MessageReceivedEventArgs>? MessageReceived;

    /// <summary>
    ///     Event that is fired when an (internal) error occurs within the server or a connected client.
    /// </summary>
    public event EventHandler<ErrorEventArgs>? Error;

    public async Task StartAsync()
    {
        if (!Running.IsCompleted)
        {
            throw new InvalidOperationException("The server has already started.");
        }

        try
        {
            _tcpListener.Start();
        }
        catch (SocketException ex)
        {
            Error?.Invoke(this, new ErrorEventArgs(ex));
            return;
        }

        // TODO: I think `using (cancellationToken.Register(...)` should be somewhere below
        _cts = new CancellationTokenSource();
        _cts.Token.Register(() => _tcpListener.Stop());

        Running = StartAcceptingAsync(_cts.Token);

        await Running.ConfigureAwait(false);

        _logger.LogInformation("On server ready");
    }

    public Task StopAsync()
    {
        if (Running.IsCompleted)
        {
            throw new InvalidOperationException("The web socket hasn't started yet.");
        }

        _cts?.Cancel();
        _cts?.Dispose();
        _cts = null;

        return Task.CompletedTask;
    }

    private async Task StartAcceptingAsync(CancellationToken cancellationToken)
    {
        while (!cancellationToken.IsCancellationRequested)
        {
            try
            {
                using var tcpClient =
                    await _tcpListener.AcceptTcpClientAsync(cancellationToken).ConfigureAwait(false);

                ConnectedClients.Add(tcpClient);

                ClientConnected?.Invoke(this, new ConnectedEventArgs(tcpClient));

                //using SslStream stream = await EstablishSslStream(tcpClient.GetStream(), cancellationToken);
                //await HandleConnection(stream, tcpClient, cancellationToken);

                await StartReceivingAsync(tcpClient, cancellationToken).ConfigureAwait(false);
            }
            catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
            {
                _logger.LogInformation("TcpListener stopped listening because cancellation was requested.");
            }
            catch (Exception ex)
            {
                Error?.Invoke(this, new ErrorEventArgs(ex));
            }
        }
    }

    public async Task StartReceivingAsync(TcpClient tcpClient, CancellationToken cancellationToken)
    {
        await using var stream = tcpClient.GetStream();

        while (!cancellationToken.IsCancellationRequested)
        {
            var buffer = new byte[2048];

            var readCount = await stream.ReadAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false);

            if (readCount == 0)
            {
                await Task.Delay(100, cancellationToken).ConfigureAwait(false);
                continue;
            }

            var data = Encoding.UTF8.GetString(buffer);

            //ReceivedBytesCount += readCount;

            MessageReceived?.Invoke(this, new MessageReceivedEventArgs(data));
        }
    }
}

public class ConnectedEventArgs : System.EventArgs
{
    public ConnectedEventArgs(TcpClient tcpClient) => TcpClient = tcpClient;

    public TcpClient TcpClient { get; set; }
}

public class DisconnectedEventArgs
{
    public DisconnectedEventArgs(TcpClient tcpClient) => TcpClient = tcpClient;

    public TcpClient TcpClient { get; set; }
}

public class ErrorEventArgs : System.EventArgs
{
    public ErrorEventArgs(Exception exception) => Exception = exception;

    public Exception Exception { get; set; }
}

public class MessageReceivedEventArgs : System.EventArgs
{
    public MessageReceivedEventArgs(string message) => Message = message;

    public string Message { get; set; }
}

TCP client (Don't review this one = Test purposes only)

public static void Main()
{
    try
    {
        var tcpClient = new TcpClient();

        Console.WriteLine("Connecting.....");

        tcpClient.Connect("127.0.0.1", 6000);

        Console.WriteLine("Connected");
        Console.Write("Enter the string to be transmitted: ");

        var line = Console.ReadLine();
        Stream stream = tcpClient.GetStream();

        var asen = new ASCIIEncoding();
        var ba = asen.GetBytes(line);
        Console.WriteLine("Transmitting.....");

        stream.Write(ba, 0, ba.Length);

        var bb = new byte[100];
        var k = stream.Read(bb, 0, 100);

        for (var i = 0; i < k; i++)
            Console.Write(Convert.ToChar(bb[i]));

        tcpClient.Close();
    }

    catch (Exception e)
    {
        Console.WriteLine("Error..... " + e.StackTrace);
    }
}
```
\$\endgroup\$
3
  • 1
    \$\begingroup\$ A short tip while (!cancellationToken.IsCancellationRequested) is a kind of redundant check in case existent async operatins inside that accepting the same token, do while (true). I think, you don't need the explanation. :) Also for new byte[2048]; consider to use ArrayPool for performance purpose. Also you may create the buffer outside of the loop. \$\endgroup\$
    – aepot
    Dec 19 '21 at 22:58
  • 1
    \$\begingroup\$ new ASCIIEncoding();, the better one is Encoding.ASCII. \$\endgroup\$
    – aepot
    Dec 19 '21 at 22:59
  • 1
    \$\begingroup\$ @aepot, Fair point! \$\endgroup\$
    – nop
    Dec 19 '21 at 23:52

Your Answer

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

Browse other questions tagged or ask your own question.