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, 2021 at 22:58
  • 1
    \$\begingroup\$ new ASCIIEncoding();, the better one is Encoding.ASCII. \$\endgroup\$
    – aepot
    Dec 19, 2021 at 22:59
  • 1
    \$\begingroup\$ @aepot, Fair point! \$\endgroup\$
    – nop
    Dec 19, 2021 at 23:52

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Browse other questions tagged or ask your own question.