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
EventHandler
s 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/orStopAsync()
/Dispose()
.- I'm struggling to implement
SendMessage()
andSendMessageToAll()
, 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);
}
}
```
while (!cancellationToken.IsCancellationRequested)
is a kind of redundant check in case existent async operatins inside that accepting the same token, dowhile (true)
. I think, you don't need the explanation. :) Also fornew byte[2048];
consider to useArrayPool
for performance purpose. Also you may create the buffer outside of the loop. \$\endgroup\$new ASCIIEncoding();
, the better one isEncoding.ASCII
. \$\endgroup\$