TCP client and server API
$begingroup$
Hello I made a server and client API for TCP connection. It is intended to shorten the work you need to do to prepare the server and client.
It's separeted in 3 projects - TCPServer, TCPClient, Common.
I've made an extended tcpclient type IdentifiableTcpClient, allowing for various modifications, the base additions are a Name and an ID. The class implements IComponentConsumer which allows for components based system to be created, further extending the flexibility of the client. There is also a DTO type used for serialization to pass through the network IdentifiableTcpClientDTO
All components are required to inherit a marker interface IComponent
IComponent
public interface IComponent
{
}
IComponentConsumer
public interface IComponentConsumer
{
IComponentConsumer AddComponent<T>(T component) where T : class, IComponent;
T GetComponent<T>() where T : class, IComponent;
}
IdentifiableTcpClient
public class IdentifiableTcpClient : IEquatable<IdentifiableTcpClient>, IComponentConsumer, ICloneable
{
private readonly Dictionary<Type, IComponent> _components = new Dictionary<Type, IComponent>();
private readonly Guid _internalID;
public string Name { get; set; }
public TcpClient TcpClient { get; }
public IdentifiableTcpClient(TcpClient tcpClient, string name)
: this(tcpClient, name, Guid.NewGuid())
{
}
public IdentifiableTcpClient(TcpClient tcpClient)
: this(tcpClient, Environment.UserName)
{
}
//Used for cloning
private IdentifiableTcpClient(TcpClient tcpClient, string name, Guid internalID)
{
_internalID = internalID;
TcpClient = tcpClient;
Name = name;
}
public bool Equals(IdentifiableTcpClient other)
{
if (ReferenceEquals(null, other)) return false;
if (ReferenceEquals(this, other)) return true;
return _internalID.Equals(other._internalID);
}
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
public override int GetHashCode()
{
return _internalID.GetHashCode();
}
public object Clone()
{
return new IdentifiableTcpClient(TcpClient, Name, _internalID);
}
public IComponentConsumer AddComponent<T>(T component)
where T : class, IComponent
{
var type = typeof(T);
if (_components.ContainsKey(type))
{
_components[type] = component;
}
else
{
_components.Add(type, component);
}
return this;
}
public T GetComponent<T>()
where T : class, IComponent
{
if (_components.TryGetValue(typeof(T), out var component))
{
return (T)component;
}
return null;
}
}
IdentifiableTcpClientDTO
[Serializable]
public class IdentifiableTcpClientDTO
{
public string Name { get; set; }
public int HashCode { get; set; }
public IdentifiableTcpClientDTO(IdentifiableTcpClient client)
{
Name = client.Name;
HashCode = client.GetHashCode();
}
public IdentifiableTcpClientDTO()
{
}
}
There is also support for messages, commands and direct modifications on the client:
Message
public enum MessageType
{
Message,
Command
}
[Serializable]
public class Message
{
public IdentifiableTcpClientDTO SenderDTO { get; }
public string Data { get; }
public MessageType MessageType { get; }
public IEnumerable<IModification<IdentifiableTcpClient>> Modifications { get; }
public Message(IdentifiableTcpClientDTO senderDTO, string data, MessageType messageType,
IEnumerable<IModification<IdentifiableTcpClient>> modifications)
{
SenderDTO = senderDTO;
Data = data;
MessageType = messageType;
Modifications = modifications;
}
}
Where Modifications are a couple of types allowing direct modifications to the client's properties/fields, they are not really relevant for this question so I will leave them at that.
Those messages are later serialized/deserialized by MessageSerializer
MessageSerializer
public static class MessageSerializer
{
public static byte Serialize(Message message)
{
using (var memoryStream = new MemoryStream())
{
new BinaryFormatter().Serialize(memoryStream, message);
return memoryStream.ToArray();
}
}
public static Message Deserialize(byte messageData)
{
using (var memoryStream = new MemoryStream(messageData))
{
return (Message) new BinaryFormatter().Deserialize(memoryStream);
}
}
}
Moving on to the tcp server:
ClientDataEventArgs
public class ClientDataEventArgs
{
public IdentifiableTcpClient Client { get; }
public string Message { get; }
public ClientDataEventArgs(IdentifiableTcpClient client, string message)
{
Client = client;
Message = message;
}
}
TCPServerMain
public class TCPServerMain
{
public event EventHandler<IdentifiableTcpClient> ClientConnected;
public event EventHandler<IdentifiableTcpClient> ClientDisconnected;
public event EventHandler<ClientDataEventArgs> ClientDataReceived;
public TimeSpan DisconnectInterval { get; set; } = TimeSpan.FromMilliseconds(100);
public IdentifiableTcpClientDTO Identity => SharedConfiguration.ServerIdentity;
private static readonly object _padlock = new object();
private readonly Timer _disconnectTimer;
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
public TCPServerMain()
{
_disconnectTimer = new Timer(
callback: DisconnectTimerCallback,
state: null,
dueTime: (int) DisconnectInterval.TotalMilliseconds,
period: (int) DisconnectInterval.TotalMilliseconds);
var localAdd = IPAddress.Parse(ConfigurationResources.IPAdress);
var listener = new TcpListener(localAdd, int.Parse(ConfigurationResources.PortNumber));
listener.Start();
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
private void DisconnectTimerCallback(object state)
{
lock (_padlock)
{
var faultedClients = new HashSet<IdentifiableTcpClient>();
foreach (var tcpClient in Clients)
{
if (!tcpClient.TcpClient.Client.IsConnected())
{
faultedClients.Add(tcpClient);
}
}
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
}
private void ClientConnectedCallback(IAsyncResult ar)
{
lock (_padlock)
{
var listener = (TcpListener) ar.AsyncState;
try
{
var client = new IdentifiableTcpClient(listener.EndAcceptTcpClient(ar));
OnClientConnected(client);
BeginReadClientDataCallback(client);
}
catch (Exception e)
{
Console.WriteLine(e);
}
finally
{
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
}
}
private void BeginReadClientDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
try
{
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
catch (InvalidOperationException e)
{
OnDisconnectedClient(client);
Console.WriteLine(e);
}
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient)ar.AsyncState;
try
{
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return;
}
var data = Encoding.GetEncoding("Windows-1251").GetString(buffer, 0, bytesRead);
OnClientDataReceived(client, data);
}
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
finally
{
BeginReadClientDataCallback(client);
}
}
private void OnClientConnected(IdentifiableTcpClient client)
{
Clients.Add(client);
ClientConnected?.Invoke(this, client);
}
private void OnClientDataReceived(IdentifiableTcpClient client, string message)
{
ClientDataReceived?.Invoke(this, new ClientDataEventArgs(client, message));
}
private void OnDisconnectedClient(IdentifiableTcpClient client)
{
Clients.Remove(client);
ClientDisconnected?.Invoke(this, client);
}
}
TCPClientMain
public class TCPClientMain
{
public event EventHandler<string> ServerDataReceived;
private readonly IdentifiableTcpClient _currentClient;
public IdentifiableTcpClientDTO Identity => new IdentifiableTcpClientDTO(_currentClient);
public TCPClientMain(string serverIP, int portNumber)
{
_currentClient = new IdentifiableTcpClient(new TcpClient(serverIP, portNumber));
}
public TCPClientMain()
: this(ConfigurationResources.IPAdress, int.Parse(ConfigurationResources.PortNumber))
{
}
public void SendMessageToServer(string message)
{
var bytesToSend = Encoding.GetEncoding("Windows-1251").GetBytes(message);
_currentClient.TcpClient.GetStream().Write(bytesToSend, 0, bytesToSend.Length);
}
public void BeginReadDataFromServer()
{
BeginReadServerDataCallback(_currentClient);
}
private void BeginReadServerDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient) ar.AsyncState;
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return; //disconnected
}
var message = MessageSerializer.Deserialize(buffer);
ProcessMessageFromServer(client, message);
BeginReadServerDataCallback(client);
}
private void ProcessMessageFromServer(IdentifiableTcpClient client, Message message)
{
if (message.MessageType == MessageType.Message)
{
OnServerDataReceived(message.SenderDTO, message.Data);
}
else
{
foreach (var modification in message.Modifications)
{
modification.Modify(client);
}
}
}
private void OnServerDataReceived(IdentifiableTcpClientDTO sender, string message)
{
ServerDataReceived?.Invoke(sender, message);
}
}
A couple of helper classes:
SharedConfiguration
public static class SharedConfiguration
{
public static IdentifiableTcpClientDTO ServerIdentity { get; }
static SharedConfiguration()
{
ServerIdentity = new IdentifiableTcpClientDTO { HashCode = -1, Name = "SERVER" };
}
}
SocketExtensions
public static class SocketExtensions
{
public static bool IsConnected(this Socket socket)
{
try
{
return !(socket.Poll(1, SelectMode.SelectRead) && socket.Available == 0);
}
catch (SocketException) { return false; }
}
}
This is my first TCP/IP client/server implementation so it's more than likely I've made some rookie mistakes and that's what I'd like the focus to be on. Code-style and optimizations are of course welcome, especially the latter, since it's pretty important.
c# networking server tcp client
asked 12 hours ago
DenisDenis
6,37021755
$endgroup$
add a comment |
$begingroup$
Hello I made a server and client API for TCP connection. It is intended to shorten the work you need to do to prepare the server and client.
It's separeted in 3 projects - TCPServer, TCPClient, Common.
I've made an extended tcpclient type IdentifiableTcpClient, allowing for various modifications, the base additions are a Name and an ID. The class implements IComponentConsumer which allows for components based system to be created, further extending the flexibility of the client. There is also a DTO type used for serialization to pass through the network IdentifiableTcpClientDTO
All components are required to inherit a marker interface IComponent
IComponent
public interface IComponent
{
}
IComponentConsumer
public interface IComponentConsumer
{
IComponentConsumer AddComponent<T>(T component) where T : class, IComponent;
T GetComponent<T>() where T : class, IComponent;
}
IdentifiableTcpClient
public class IdentifiableTcpClient : IEquatable<IdentifiableTcpClient>, IComponentConsumer, ICloneable
{
private readonly Dictionary<Type, IComponent> _components = new Dictionary<Type, IComponent>();
private readonly Guid _internalID;
public string Name { get; set; }
public TcpClient TcpClient { get; }
public IdentifiableTcpClient(TcpClient tcpClient, string name)
: this(tcpClient, name, Guid.NewGuid())
{
}
public IdentifiableTcpClient(TcpClient tcpClient)
: this(tcpClient, Environment.UserName)
{
}
//Used for cloning
private IdentifiableTcpClient(TcpClient tcpClient, string name, Guid internalID)
{
_internalID = internalID;
TcpClient = tcpClient;
Name = name;
}
public bool Equals(IdentifiableTcpClient other)
{
if (ReferenceEquals(null, other)) return false;
if (ReferenceEquals(this, other)) return true;
return _internalID.Equals(other._internalID);
}
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
public override int GetHashCode()
{
return _internalID.GetHashCode();
}
public object Clone()
{
return new IdentifiableTcpClient(TcpClient, Name, _internalID);
}
public IComponentConsumer AddComponent<T>(T component)
where T : class, IComponent
{
var type = typeof(T);
if (_components.ContainsKey(type))
{
_components[type] = component;
}
else
{
_components.Add(type, component);
}
return this;
}
public T GetComponent<T>()
where T : class, IComponent
{
if (_components.TryGetValue(typeof(T), out var component))
{
return (T)component;
}
return null;
}
}
IdentifiableTcpClientDTO
[Serializable]
public class IdentifiableTcpClientDTO
{
public string Name { get; set; }
public int HashCode { get; set; }
public IdentifiableTcpClientDTO(IdentifiableTcpClient client)
{
Name = client.Name;
HashCode = client.GetHashCode();
}
public IdentifiableTcpClientDTO()
{
}
}
There is also support for messages, commands and direct modifications on the client:
Message
public enum MessageType
{
Message,
Command
}
[Serializable]
public class Message
{
public IdentifiableTcpClientDTO SenderDTO { get; }
public string Data { get; }
public MessageType MessageType { get; }
public IEnumerable<IModification<IdentifiableTcpClient>> Modifications { get; }
public Message(IdentifiableTcpClientDTO senderDTO, string data, MessageType messageType,
IEnumerable<IModification<IdentifiableTcpClient>> modifications)
{
SenderDTO = senderDTO;
Data = data;
MessageType = messageType;
Modifications = modifications;
}
}
Where Modifications are a couple of types allowing direct modifications to the client's properties/fields, they are not really relevant for this question so I will leave them at that.
Those messages are later serialized/deserialized by MessageSerializer
MessageSerializer
public static class MessageSerializer
{
public static byte Serialize(Message message)
{
using (var memoryStream = new MemoryStream())
{
new BinaryFormatter().Serialize(memoryStream, message);
return memoryStream.ToArray();
}
}
public static Message Deserialize(byte messageData)
{
using (var memoryStream = new MemoryStream(messageData))
{
return (Message) new BinaryFormatter().Deserialize(memoryStream);
}
}
}
Moving on to the tcp server:
ClientDataEventArgs
public class ClientDataEventArgs
{
public IdentifiableTcpClient Client { get; }
public string Message { get; }
public ClientDataEventArgs(IdentifiableTcpClient client, string message)
{
Client = client;
Message = message;
}
}
TCPServerMain
public class TCPServerMain
{
public event EventHandler<IdentifiableTcpClient> ClientConnected;
public event EventHandler<IdentifiableTcpClient> ClientDisconnected;
public event EventHandler<ClientDataEventArgs> ClientDataReceived;
public TimeSpan DisconnectInterval { get; set; } = TimeSpan.FromMilliseconds(100);
public IdentifiableTcpClientDTO Identity => SharedConfiguration.ServerIdentity;
private static readonly object _padlock = new object();
private readonly Timer _disconnectTimer;
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
public TCPServerMain()
{
_disconnectTimer = new Timer(
callback: DisconnectTimerCallback,
state: null,
dueTime: (int) DisconnectInterval.TotalMilliseconds,
period: (int) DisconnectInterval.TotalMilliseconds);
var localAdd = IPAddress.Parse(ConfigurationResources.IPAdress);
var listener = new TcpListener(localAdd, int.Parse(ConfigurationResources.PortNumber));
listener.Start();
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
private void DisconnectTimerCallback(object state)
{
lock (_padlock)
{
var faultedClients = new HashSet<IdentifiableTcpClient>();
foreach (var tcpClient in Clients)
{
if (!tcpClient.TcpClient.Client.IsConnected())
{
faultedClients.Add(tcpClient);
}
}
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
}
private void ClientConnectedCallback(IAsyncResult ar)
{
lock (_padlock)
{
var listener = (TcpListener) ar.AsyncState;
try
{
var client = new IdentifiableTcpClient(listener.EndAcceptTcpClient(ar));
OnClientConnected(client);
BeginReadClientDataCallback(client);
}
catch (Exception e)
{
Console.WriteLine(e);
}
finally
{
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
}
}
private void BeginReadClientDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
try
{
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
catch (InvalidOperationException e)
{
OnDisconnectedClient(client);
Console.WriteLine(e);
}
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient)ar.AsyncState;
try
{
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return;
}
var data = Encoding.GetEncoding("Windows-1251").GetString(buffer, 0, bytesRead);
OnClientDataReceived(client, data);
}
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
finally
{
BeginReadClientDataCallback(client);
}
}
private void OnClientConnected(IdentifiableTcpClient client)
{
Clients.Add(client);
ClientConnected?.Invoke(this, client);
}
private void OnClientDataReceived(IdentifiableTcpClient client, string message)
{
ClientDataReceived?.Invoke(this, new ClientDataEventArgs(client, message));
}
private void OnDisconnectedClient(IdentifiableTcpClient client)
{
Clients.Remove(client);
ClientDisconnected?.Invoke(this, client);
}
}
TCPClientMain
public class TCPClientMain
{
public event EventHandler<string> ServerDataReceived;
private readonly IdentifiableTcpClient _currentClient;
public IdentifiableTcpClientDTO Identity => new IdentifiableTcpClientDTO(_currentClient);
public TCPClientMain(string serverIP, int portNumber)
{
_currentClient = new IdentifiableTcpClient(new TcpClient(serverIP, portNumber));
}
public TCPClientMain()
: this(ConfigurationResources.IPAdress, int.Parse(ConfigurationResources.PortNumber))
{
}
public void SendMessageToServer(string message)
{
var bytesToSend = Encoding.GetEncoding("Windows-1251").GetBytes(message);
_currentClient.TcpClient.GetStream().Write(bytesToSend, 0, bytesToSend.Length);
}
public void BeginReadDataFromServer()
{
BeginReadServerDataCallback(_currentClient);
}
private void BeginReadServerDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient) ar.AsyncState;
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return; //disconnected
}
var message = MessageSerializer.Deserialize(buffer);
ProcessMessageFromServer(client, message);
BeginReadServerDataCallback(client);
}
private void ProcessMessageFromServer(IdentifiableTcpClient client, Message message)
{
if (message.MessageType == MessageType.Message)
{
OnServerDataReceived(message.SenderDTO, message.Data);
}
else
{
foreach (var modification in message.Modifications)
{
modification.Modify(client);
}
}
}
private void OnServerDataReceived(IdentifiableTcpClientDTO sender, string message)
{
ServerDataReceived?.Invoke(sender, message);
}
}
A couple of helper classes:
SharedConfiguration
public static class SharedConfiguration
{
public static IdentifiableTcpClientDTO ServerIdentity { get; }
static SharedConfiguration()
{
ServerIdentity = new IdentifiableTcpClientDTO { HashCode = -1, Name = "SERVER" };
}
}
SocketExtensions
public static class SocketExtensions
{
public static bool IsConnected(this Socket socket)
{
try
{
return !(socket.Poll(1, SelectMode.SelectRead) && socket.Available == 0);
}
catch (SocketException) { return false; }
}
}
This is my first TCP/IP client/server implementation so it's more than likely I've made some rookie mistakes and that's what I'd like the focus to be on. Code-style and optimizations are of course welcome, especially the latter, since it's pretty important.
c# networking server tcp client
asked 12 hours ago
DenisDenis
6,37021755
$endgroup$
$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
12 hours ago
$begingroup$
Not really, since I was playing around with it I left it at that, probably should parameterize it. @PieterWitvoet I've included theSharedConfigurationclass it's inside the Common project, the resources are simply resource files also located in the afore mentionted assembly. If the API is public, those should probably be parameters as well.
$endgroup$
– Denis
12 hours ago
$begingroup$
Just a few more it seems:IModificationandIdentifiableTcpClient.
$endgroup$
– Pieter Witvoet
11 hours ago
$begingroup$
@PieterWitvoet theIdentifiableTcpClientis already included,IModificationreally isn't important and would add a lot of code which is of no significance for this question. Wherever it's used in the code, could be omitted, if you want to test it out.
$endgroup$
– Denis
11 hours ago
$begingroup$
One thing I'd suggest adding to the wire format is an ApiVersion field. Having written several RPCs, and then wishing I'd added an ApiVersion, when I then want to tweak API, but maintain backwards compatibility at both ends…
$endgroup$
– CSM
7 hours ago
add a comment |
$begingroup$
Hello I made a server and client API for TCP connection. It is intended to shorten the work you need to do to prepare the server and client.
It's separeted in 3 projects - TCPServer, TCPClient, Common.
I've made an extended tcpclient type IdentifiableTcpClient, allowing for various modifications, the base additions are a Name and an ID. The class implements IComponentConsumer which allows for components based system to be created, further extending the flexibility of the client. There is also a DTO type used for serialization to pass through the network IdentifiableTcpClientDTO
All components are required to inherit a marker interface IComponent
IComponent
public interface IComponent
{
}
IComponentConsumer
public interface IComponentConsumer
{
IComponentConsumer AddComponent<T>(T component) where T : class, IComponent;
T GetComponent<T>() where T : class, IComponent;
}
IdentifiableTcpClient
public class IdentifiableTcpClient : IEquatable<IdentifiableTcpClient>, IComponentConsumer, ICloneable
{
private readonly Dictionary<Type, IComponent> _components = new Dictionary<Type, IComponent>();
private readonly Guid _internalID;
public string Name { get; set; }
public TcpClient TcpClient { get; }
public IdentifiableTcpClient(TcpClient tcpClient, string name)
: this(tcpClient, name, Guid.NewGuid())
{
}
public IdentifiableTcpClient(TcpClient tcpClient)
: this(tcpClient, Environment.UserName)
{
}
//Used for cloning
private IdentifiableTcpClient(TcpClient tcpClient, string name, Guid internalID)
{
_internalID = internalID;
TcpClient = tcpClient;
Name = name;
}
public bool Equals(IdentifiableTcpClient other)
{
if (ReferenceEquals(null, other)) return false;
if (ReferenceEquals(this, other)) return true;
return _internalID.Equals(other._internalID);
}
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
public override int GetHashCode()
{
return _internalID.GetHashCode();
}
public object Clone()
{
return new IdentifiableTcpClient(TcpClient, Name, _internalID);
}
public IComponentConsumer AddComponent<T>(T component)
where T : class, IComponent
{
var type = typeof(T);
if (_components.ContainsKey(type))
{
_components[type] = component;
}
else
{
_components.Add(type, component);
}
return this;
}
public T GetComponent<T>()
where T : class, IComponent
{
if (_components.TryGetValue(typeof(T), out var component))
{
return (T)component;
}
return null;
}
}
IdentifiableTcpClientDTO
[Serializable]
public class IdentifiableTcpClientDTO
{
public string Name { get; set; }
public int HashCode { get; set; }
public IdentifiableTcpClientDTO(IdentifiableTcpClient client)
{
Name = client.Name;
HashCode = client.GetHashCode();
}
public IdentifiableTcpClientDTO()
{
}
}
There is also support for messages, commands and direct modifications on the client:
Message
public enum MessageType
{
Message,
Command
}
[Serializable]
public class Message
{
public IdentifiableTcpClientDTO SenderDTO { get; }
public string Data { get; }
public MessageType MessageType { get; }
public IEnumerable<IModification<IdentifiableTcpClient>> Modifications { get; }
public Message(IdentifiableTcpClientDTO senderDTO, string data, MessageType messageType,
IEnumerable<IModification<IdentifiableTcpClient>> modifications)
{
SenderDTO = senderDTO;
Data = data;
MessageType = messageType;
Modifications = modifications;
}
}
Where Modifications are a couple of types allowing direct modifications to the client's properties/fields, they are not really relevant for this question so I will leave them at that.
Those messages are later serialized/deserialized by MessageSerializer
MessageSerializer
public static class MessageSerializer
{
public static byte Serialize(Message message)
{
using (var memoryStream = new MemoryStream())
{
new BinaryFormatter().Serialize(memoryStream, message);
return memoryStream.ToArray();
}
}
public static Message Deserialize(byte messageData)
{
using (var memoryStream = new MemoryStream(messageData))
{
return (Message) new BinaryFormatter().Deserialize(memoryStream);
}
}
}
Moving on to the tcp server:
ClientDataEventArgs
public class ClientDataEventArgs
{
public IdentifiableTcpClient Client { get; }
public string Message { get; }
public ClientDataEventArgs(IdentifiableTcpClient client, string message)
{
Client = client;
Message = message;
}
}
TCPServerMain
public class TCPServerMain
{
public event EventHandler<IdentifiableTcpClient> ClientConnected;
public event EventHandler<IdentifiableTcpClient> ClientDisconnected;
public event EventHandler<ClientDataEventArgs> ClientDataReceived;
public TimeSpan DisconnectInterval { get; set; } = TimeSpan.FromMilliseconds(100);
public IdentifiableTcpClientDTO Identity => SharedConfiguration.ServerIdentity;
private static readonly object _padlock = new object();
private readonly Timer _disconnectTimer;
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
public TCPServerMain()
{
_disconnectTimer = new Timer(
callback: DisconnectTimerCallback,
state: null,
dueTime: (int) DisconnectInterval.TotalMilliseconds,
period: (int) DisconnectInterval.TotalMilliseconds);
var localAdd = IPAddress.Parse(ConfigurationResources.IPAdress);
var listener = new TcpListener(localAdd, int.Parse(ConfigurationResources.PortNumber));
listener.Start();
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
private void DisconnectTimerCallback(object state)
{
lock (_padlock)
{
var faultedClients = new HashSet<IdentifiableTcpClient>();
foreach (var tcpClient in Clients)
{
if (!tcpClient.TcpClient.Client.IsConnected())
{
faultedClients.Add(tcpClient);
}
}
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
}
private void ClientConnectedCallback(IAsyncResult ar)
{
lock (_padlock)
{
var listener = (TcpListener) ar.AsyncState;
try
{
var client = new IdentifiableTcpClient(listener.EndAcceptTcpClient(ar));
OnClientConnected(client);
BeginReadClientDataCallback(client);
}
catch (Exception e)
{
Console.WriteLine(e);
}
finally
{
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
}
}
private void BeginReadClientDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
try
{
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
catch (InvalidOperationException e)
{
OnDisconnectedClient(client);
Console.WriteLine(e);
}
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient)ar.AsyncState;
try
{
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return;
}
var data = Encoding.GetEncoding("Windows-1251").GetString(buffer, 0, bytesRead);
OnClientDataReceived(client, data);
}
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
finally
{
BeginReadClientDataCallback(client);
}
}
private void OnClientConnected(IdentifiableTcpClient client)
{
Clients.Add(client);
ClientConnected?.Invoke(this, client);
}
private void OnClientDataReceived(IdentifiableTcpClient client, string message)
{
ClientDataReceived?.Invoke(this, new ClientDataEventArgs(client, message));
}
private void OnDisconnectedClient(IdentifiableTcpClient client)
{
Clients.Remove(client);
ClientDisconnected?.Invoke(this, client);
}
}
TCPClientMain
public class TCPClientMain
{
public event EventHandler<string> ServerDataReceived;
private readonly IdentifiableTcpClient _currentClient;
public IdentifiableTcpClientDTO Identity => new IdentifiableTcpClientDTO(_currentClient);
public TCPClientMain(string serverIP, int portNumber)
{
_currentClient = new IdentifiableTcpClient(new TcpClient(serverIP, portNumber));
}
public TCPClientMain()
: this(ConfigurationResources.IPAdress, int.Parse(ConfigurationResources.PortNumber))
{
}
public void SendMessageToServer(string message)
{
var bytesToSend = Encoding.GetEncoding("Windows-1251").GetBytes(message);
_currentClient.TcpClient.GetStream().Write(bytesToSend, 0, bytesToSend.Length);
}
public void BeginReadDataFromServer()
{
BeginReadServerDataCallback(_currentClient);
}
private void BeginReadServerDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient) ar.AsyncState;
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return; //disconnected
}
var message = MessageSerializer.Deserialize(buffer);
ProcessMessageFromServer(client, message);
BeginReadServerDataCallback(client);
}
private void ProcessMessageFromServer(IdentifiableTcpClient client, Message message)
{
if (message.MessageType == MessageType.Message)
{
OnServerDataReceived(message.SenderDTO, message.Data);
}
else
{
foreach (var modification in message.Modifications)
{
modification.Modify(client);
}
}
}
private void OnServerDataReceived(IdentifiableTcpClientDTO sender, string message)
{
ServerDataReceived?.Invoke(sender, message);
}
}
A couple of helper classes:
SharedConfiguration
public static class SharedConfiguration
{
public static IdentifiableTcpClientDTO ServerIdentity { get; }
static SharedConfiguration()
{
ServerIdentity = new IdentifiableTcpClientDTO { HashCode = -1, Name = "SERVER" };
}
}
SocketExtensions
public static class SocketExtensions
{
public static bool IsConnected(this Socket socket)
{
try
{
return !(socket.Poll(1, SelectMode.SelectRead) && socket.Available == 0);
}
catch (SocketException) { return false; }
}
}
This is my first TCP/IP client/server implementation so it's more than likely I've made some rookie mistakes and that's what I'd like the focus to be on. Code-style and optimizations are of course welcome, especially the latter, since it's pretty important.
c# networking server tcp client
asked 12 hours ago
DenisDenis
6,37021755
$endgroup$
Hello I made a server and client API for TCP connection. It is intended to shorten the work you need to do to prepare the server and client.
It's separeted in 3 projects - TCPServer, TCPClient, Common.
I've made an extended tcpclient type IdentifiableTcpClient, allowing for various modifications, the base additions are a Name and an ID. The class implements IComponentConsumer which allows for components based system to be created, further extending the flexibility of the client. There is also a DTO type used for serialization to pass through the network IdentifiableTcpClientDTO
All components are required to inherit a marker interface IComponent
IComponent
public interface IComponent
{
}
IComponentConsumer
public interface IComponentConsumer
{
IComponentConsumer AddComponent<T>(T component) where T : class, IComponent;
T GetComponent<T>() where T : class, IComponent;
}
IdentifiableTcpClient
public class IdentifiableTcpClient : IEquatable<IdentifiableTcpClient>, IComponentConsumer, ICloneable
{
private readonly Dictionary<Type, IComponent> _components = new Dictionary<Type, IComponent>();
private readonly Guid _internalID;
public string Name { get; set; }
public TcpClient TcpClient { get; }
public IdentifiableTcpClient(TcpClient tcpClient, string name)
: this(tcpClient, name, Guid.NewGuid())
{
}
public IdentifiableTcpClient(TcpClient tcpClient)
: this(tcpClient, Environment.UserName)
{
}
//Used for cloning
private IdentifiableTcpClient(TcpClient tcpClient, string name, Guid internalID)
{
_internalID = internalID;
TcpClient = tcpClient;
Name = name;
}
public bool Equals(IdentifiableTcpClient other)
{
if (ReferenceEquals(null, other)) return false;
if (ReferenceEquals(this, other)) return true;
return _internalID.Equals(other._internalID);
}
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
public override int GetHashCode()
{
return _internalID.GetHashCode();
}
public object Clone()
{
return new IdentifiableTcpClient(TcpClient, Name, _internalID);
}
public IComponentConsumer AddComponent<T>(T component)
where T : class, IComponent
{
var type = typeof(T);
if (_components.ContainsKey(type))
{
_components[type] = component;
}
else
{
_components.Add(type, component);
}
return this;
}
public T GetComponent<T>()
where T : class, IComponent
{
if (_components.TryGetValue(typeof(T), out var component))
{
return (T)component;
}
return null;
}
}
IdentifiableTcpClientDTO
[Serializable]
public class IdentifiableTcpClientDTO
{
public string Name { get; set; }
public int HashCode { get; set; }
public IdentifiableTcpClientDTO(IdentifiableTcpClient client)
{
Name = client.Name;
HashCode = client.GetHashCode();
}
public IdentifiableTcpClientDTO()
{
}
}
There is also support for messages, commands and direct modifications on the client:
Message
public enum MessageType
{
Message,
Command
}
[Serializable]
public class Message
{
public IdentifiableTcpClientDTO SenderDTO { get; }
public string Data { get; }
public MessageType MessageType { get; }
public IEnumerable<IModification<IdentifiableTcpClient>> Modifications { get; }
public Message(IdentifiableTcpClientDTO senderDTO, string data, MessageType messageType,
IEnumerable<IModification<IdentifiableTcpClient>> modifications)
{
SenderDTO = senderDTO;
Data = data;
MessageType = messageType;
Modifications = modifications;
}
}
Where Modifications are a couple of types allowing direct modifications to the client's properties/fields, they are not really relevant for this question so I will leave them at that.
Those messages are later serialized/deserialized by MessageSerializer
MessageSerializer
public static class MessageSerializer
{
public static byte Serialize(Message message)
{
using (var memoryStream = new MemoryStream())
{
new BinaryFormatter().Serialize(memoryStream, message);
return memoryStream.ToArray();
}
}
public static Message Deserialize(byte messageData)
{
using (var memoryStream = new MemoryStream(messageData))
{
return (Message) new BinaryFormatter().Deserialize(memoryStream);
}
}
}
Moving on to the tcp server:
ClientDataEventArgs
public class ClientDataEventArgs
{
public IdentifiableTcpClient Client { get; }
public string Message { get; }
public ClientDataEventArgs(IdentifiableTcpClient client, string message)
{
Client = client;
Message = message;
}
}
TCPServerMain
public class TCPServerMain
{
public event EventHandler<IdentifiableTcpClient> ClientConnected;
public event EventHandler<IdentifiableTcpClient> ClientDisconnected;
public event EventHandler<ClientDataEventArgs> ClientDataReceived;
public TimeSpan DisconnectInterval { get; set; } = TimeSpan.FromMilliseconds(100);
public IdentifiableTcpClientDTO Identity => SharedConfiguration.ServerIdentity;
private static readonly object _padlock = new object();
private readonly Timer _disconnectTimer;
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
public TCPServerMain()
{
_disconnectTimer = new Timer(
callback: DisconnectTimerCallback,
state: null,
dueTime: (int) DisconnectInterval.TotalMilliseconds,
period: (int) DisconnectInterval.TotalMilliseconds);
var localAdd = IPAddress.Parse(ConfigurationResources.IPAdress);
var listener = new TcpListener(localAdd, int.Parse(ConfigurationResources.PortNumber));
listener.Start();
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
private void DisconnectTimerCallback(object state)
{
lock (_padlock)
{
var faultedClients = new HashSet<IdentifiableTcpClient>();
foreach (var tcpClient in Clients)
{
if (!tcpClient.TcpClient.Client.IsConnected())
{
faultedClients.Add(tcpClient);
}
}
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
}
private void ClientConnectedCallback(IAsyncResult ar)
{
lock (_padlock)
{
var listener = (TcpListener) ar.AsyncState;
try
{
var client = new IdentifiableTcpClient(listener.EndAcceptTcpClient(ar));
OnClientConnected(client);
BeginReadClientDataCallback(client);
}
catch (Exception e)
{
Console.WriteLine(e);
}
finally
{
listener.BeginAcceptTcpClient(ClientConnectedCallback, listener);
}
}
}
private void BeginReadClientDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
try
{
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
catch (InvalidOperationException e)
{
OnDisconnectedClient(client);
Console.WriteLine(e);
}
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient)ar.AsyncState;
try
{
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return;
}
var data = Encoding.GetEncoding("Windows-1251").GetString(buffer, 0, bytesRead);
OnClientDataReceived(client, data);
}
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
finally
{
BeginReadClientDataCallback(client);
}
}
private void OnClientConnected(IdentifiableTcpClient client)
{
Clients.Add(client);
ClientConnected?.Invoke(this, client);
}
private void OnClientDataReceived(IdentifiableTcpClient client, string message)
{
ClientDataReceived?.Invoke(this, new ClientDataEventArgs(client, message));
}
private void OnDisconnectedClient(IdentifiableTcpClient client)
{
Clients.Remove(client);
ClientDisconnected?.Invoke(this, client);
}
}
TCPClientMain
public class TCPClientMain
{
public event EventHandler<string> ServerDataReceived;
private readonly IdentifiableTcpClient _currentClient;
public IdentifiableTcpClientDTO Identity => new IdentifiableTcpClientDTO(_currentClient);
public TCPClientMain(string serverIP, int portNumber)
{
_currentClient = new IdentifiableTcpClient(new TcpClient(serverIP, portNumber));
}
public TCPClientMain()
: this(ConfigurationResources.IPAdress, int.Parse(ConfigurationResources.PortNumber))
{
}
public void SendMessageToServer(string message)
{
var bytesToSend = Encoding.GetEncoding("Windows-1251").GetBytes(message);
_currentClient.TcpClient.GetStream().Write(bytesToSend, 0, bytesToSend.Length);
}
public void BeginReadDataFromServer()
{
BeginReadServerDataCallback(_currentClient);
}
private void BeginReadServerDataCallback(IdentifiableTcpClient client)
{
var buffer = new byte[client.TcpClient.ReceiveBufferSize];
client.TcpClient.GetStream().BeginRead(buffer, 0, client.TcpClient.ReceiveBufferSize,
ar => ClientDataReceivedCallback(buffer, ar), client);
}
private void ClientDataReceivedCallback(byte buffer, IAsyncResult ar)
{
var client = (IdentifiableTcpClient) ar.AsyncState;
var clientStream = client.TcpClient.GetStream();
var bytesRead = clientStream.EndRead(ar);
if (bytesRead == 0)
{
return; //disconnected
}
var message = MessageSerializer.Deserialize(buffer);
ProcessMessageFromServer(client, message);
BeginReadServerDataCallback(client);
}
private void ProcessMessageFromServer(IdentifiableTcpClient client, Message message)
{
if (message.MessageType == MessageType.Message)
{
OnServerDataReceived(message.SenderDTO, message.Data);
}
else
{
foreach (var modification in message.Modifications)
{
modification.Modify(client);
}
}
}
private void OnServerDataReceived(IdentifiableTcpClientDTO sender, string message)
{
ServerDataReceived?.Invoke(sender, message);
}
}
A couple of helper classes:
SharedConfiguration
public static class SharedConfiguration
{
public static IdentifiableTcpClientDTO ServerIdentity { get; }
static SharedConfiguration()
{
ServerIdentity = new IdentifiableTcpClientDTO { HashCode = -1, Name = "SERVER" };
}
}
SocketExtensions
public static class SocketExtensions
{
public static bool IsConnected(this Socket socket)
{
try
{
return !(socket.Poll(1, SelectMode.SelectRead) && socket.Available == 0);
}
catch (SocketException) { return false; }
}
}
This is my first TCP/IP client/server implementation so it's more than likely I've made some rookie mistakes and that's what I'd like the focus to be on. Code-style and optimizations are of course welcome, especially the latter, since it's pretty important.
c# networking server tcp client
c# networking server tcp client
asked 12 hours ago
DenisDenis
6,37021755
asked 12 hours ago
DenisDenis
6,37021755
edited 11 hours ago
Denis
asked 12 hours ago
DenisDenis
6,37021755
asked 12 hours ago
DenisDenis
6,37021755
asked 12 hours ago
DenisDenis
6,37021755
6,37021755
$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
12 hours ago
$begingroup$
Not really, since I was playing around with it I left it at that, probably should parameterize it. @PieterWitvoet I've included theSharedConfigurationclass it's inside the Common project, the resources are simply resource files also located in the afore mentionted assembly. If the API is public, those should probably be parameters as well.
$endgroup$
– Denis
12 hours ago
$begingroup$
Just a few more it seems:IModificationandIdentifiableTcpClient.
$endgroup$
– Pieter Witvoet
11 hours ago
$begingroup$
@PieterWitvoet theIdentifiableTcpClientis already included,IModificationreally isn't important and would add a lot of code which is of no significance for this question. Wherever it's used in the code, could be omitted, if you want to test it out.
$endgroup$
– Denis
11 hours ago
$begingroup$
One thing I'd suggest adding to the wire format is an ApiVersion field. Having written several RPCs, and then wishing I'd added an ApiVersion, when I then want to tweak API, but maintain backwards compatibility at both ends…
$endgroup$
– CSM
7 hours ago
add a comment |
$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
12 hours ago
$begingroup$
Not really, since I was playing around with it I left it at that, probably should parameterize it. @PieterWitvoet I've included theSharedConfigurationclass it's inside the Common project, the resources are simply resource files also located in the afore mentionted assembly. If the API is public, those should probably be parameters as well.
$endgroup$
– Denis
12 hours ago
$begingroup$
Just a few more it seems:IModificationandIdentifiableTcpClient.
$endgroup$
– Pieter Witvoet
11 hours ago
$begingroup$
@PieterWitvoet theIdentifiableTcpClientis already included,IModificationreally isn't important and would add a lot of code which is of no significance for this question. Wherever it's used in the code, could be omitted, if you want to test it out.
$endgroup$
– Denis
11 hours ago
$begingroup$
One thing I'd suggest adding to the wire format is an ApiVersion field. Having written several RPCs, and then wishing I'd added an ApiVersion, when I then want to tweak API, but maintain backwards compatibility at both ends…
$endgroup$
– CSM
7 hours ago
$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
12 hours ago
$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
12 hours ago
$begingroup$
Not really, since I was playing around with it I left it at that, probably should parameterize it. @PieterWitvoet I've included the
SharedConfiguration class it's inside the Common project, the resources are simply resource files also located in the afore mentionted assembly. If the API is public, those should probably be parameters as well.$endgroup$
– Denis
12 hours ago
$begingroup$
Not really, since I was playing around with it I left it at that, probably should parameterize it. @PieterWitvoet I've included the
SharedConfiguration class it's inside the Common project, the resources are simply resource files also located in the afore mentionted assembly. If the API is public, those should probably be parameters as well.$endgroup$
– Denis
12 hours ago
$begingroup$
Just a few more it seems:
IModification and IdentifiableTcpClient.$endgroup$
– Pieter Witvoet
11 hours ago
$begingroup$
Just a few more it seems:
IModification and IdentifiableTcpClient.$endgroup$
– Pieter Witvoet
11 hours ago
$begingroup$
@PieterWitvoet the
IdentifiableTcpClient is already included, IModification really isn't important and would add a lot of code which is of no significance for this question. Wherever it's used in the code, could be omitted, if you want to test it out.$endgroup$
– Denis
11 hours ago
$begingroup$
@PieterWitvoet the
IdentifiableTcpClient is already included, IModification really isn't important and would add a lot of code which is of no significance for this question. Wherever it's used in the code, could be omitted, if you want to test it out.$endgroup$
– Denis
11 hours ago
$begingroup$
One thing I'd suggest adding to the wire format is an ApiVersion field. Having written several RPCs, and then wishing I'd added an ApiVersion, when I then want to tweak API, but maintain backwards compatibility at both ends…
$endgroup$
– CSM
7 hours ago
$begingroup$
One thing I'd suggest adding to the wire format is an ApiVersion field. Having written several RPCs, and then wishing I'd added an ApiVersion, when I then want to tweak API, but maintain backwards compatibility at both ends…
$endgroup$
– CSM
7 hours ago
add a comment |
1 Answer
1
active
oldest
votes
$begingroup$
I don't understand the point of the IComponent / IComponentConsumer stuff. Nothing else in the large section of code you've posted uses them. Are you sure they're at the right layer?
Also, with IComponentConsumer, I think the code falls into the trap of extending when it should compose. It seems to me that every class which implements the interface will have the same implementation, so you should probably replace the interface with a sealed class which implements the same API and compose that into whatever classes actually use the functionality.
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
I have a slight preference for
public override bool Equals(object obj) => Equals(client as IdentifiableTcpClient);
I am confused about Message. As I read it, the client sends plain strings to the server, and the server sends Message instances back to the client. So why does Message have a property public IdentifiableTcpClientDTO SenderDTO { get; }? The sender is a server, not a client, and it should be clear from context who sent the message so it shouldn't need to be transmitted across the network.
public class ClientDataEventArgs
It's conventional that XYZEventArgs should extend EventArgs.
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
It's normally a bad idea to make a mutable collection of your state publicly available. Are you sure you don't want to have a private mutable set and make available a readonly view of it?
lock (_padlock)
{
...
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
Invoking callbacks while holding a lock is potentially a source of pain in the future. I would consider refactoring so that you modify the set while holding the lock and invoke the events after releasing it. That way any deadlock is entirely the fault of the invoking class.
catch (Exception e)
{
Console.WriteLine(e);
}
Consider using a more sophisticated logging library. Serilog, log4net, even TraceWriter.
The Encoding instance is probably worth caching in a static field. And I would suggest that since it's not 1990 any more you should prefer UTF-8 (without BOM, since UTF-8-BOM is a monstrosity).
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
Silently swallowing exceptions is worrying. You should alleviate the worry by adding some comments to explain why these particular exceptions can be safely ignored.
The BeginFoo/EndFoo style is now legacy. In new code, I would suggest using Task (async/await) style asynchronous code, because that seems to be the preferred modern option.
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212978%2ftcp-client-and-server-api%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
I don't understand the point of the IComponent / IComponentConsumer stuff. Nothing else in the large section of code you've posted uses them. Are you sure they're at the right layer?
Also, with IComponentConsumer, I think the code falls into the trap of extending when it should compose. It seems to me that every class which implements the interface will have the same implementation, so you should probably replace the interface with a sealed class which implements the same API and compose that into whatever classes actually use the functionality.
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
I have a slight preference for
public override bool Equals(object obj) => Equals(client as IdentifiableTcpClient);
I am confused about Message. As I read it, the client sends plain strings to the server, and the server sends Message instances back to the client. So why does Message have a property public IdentifiableTcpClientDTO SenderDTO { get; }? The sender is a server, not a client, and it should be clear from context who sent the message so it shouldn't need to be transmitted across the network.
public class ClientDataEventArgs
It's conventional that XYZEventArgs should extend EventArgs.
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
It's normally a bad idea to make a mutable collection of your state publicly available. Are you sure you don't want to have a private mutable set and make available a readonly view of it?
lock (_padlock)
{
...
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
Invoking callbacks while holding a lock is potentially a source of pain in the future. I would consider refactoring so that you modify the set while holding the lock and invoke the events after releasing it. That way any deadlock is entirely the fault of the invoking class.
catch (Exception e)
{
Console.WriteLine(e);
}
Consider using a more sophisticated logging library. Serilog, log4net, even TraceWriter.
The Encoding instance is probably worth caching in a static field. And I would suggest that since it's not 1990 any more you should prefer UTF-8 (without BOM, since UTF-8-BOM is a monstrosity).
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
Silently swallowing exceptions is worrying. You should alleviate the worry by adding some comments to explain why these particular exceptions can be safely ignored.
The BeginFoo/EndFoo style is now legacy. In new code, I would suggest using Task (async/await) style asynchronous code, because that seems to be the preferred modern option.
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
$endgroup$
add a comment |
$begingroup$
I don't understand the point of the IComponent / IComponentConsumer stuff. Nothing else in the large section of code you've posted uses them. Are you sure they're at the right layer?
Also, with IComponentConsumer, I think the code falls into the trap of extending when it should compose. It seems to me that every class which implements the interface will have the same implementation, so you should probably replace the interface with a sealed class which implements the same API and compose that into whatever classes actually use the functionality.
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
I have a slight preference for
public override bool Equals(object obj) => Equals(client as IdentifiableTcpClient);
I am confused about Message. As I read it, the client sends plain strings to the server, and the server sends Message instances back to the client. So why does Message have a property public IdentifiableTcpClientDTO SenderDTO { get; }? The sender is a server, not a client, and it should be clear from context who sent the message so it shouldn't need to be transmitted across the network.
public class ClientDataEventArgs
It's conventional that XYZEventArgs should extend EventArgs.
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
It's normally a bad idea to make a mutable collection of your state publicly available. Are you sure you don't want to have a private mutable set and make available a readonly view of it?
lock (_padlock)
{
...
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
Invoking callbacks while holding a lock is potentially a source of pain in the future. I would consider refactoring so that you modify the set while holding the lock and invoke the events after releasing it. That way any deadlock is entirely the fault of the invoking class.
catch (Exception e)
{
Console.WriteLine(e);
}
Consider using a more sophisticated logging library. Serilog, log4net, even TraceWriter.
The Encoding instance is probably worth caching in a static field. And I would suggest that since it's not 1990 any more you should prefer UTF-8 (without BOM, since UTF-8-BOM is a monstrosity).
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
Silently swallowing exceptions is worrying. You should alleviate the worry by adding some comments to explain why these particular exceptions can be safely ignored.
The BeginFoo/EndFoo style is now legacy. In new code, I would suggest using Task (async/await) style asynchronous code, because that seems to be the preferred modern option.
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
$endgroup$
add a comment |
$begingroup$
I don't understand the point of the IComponent / IComponentConsumer stuff. Nothing else in the large section of code you've posted uses them. Are you sure they're at the right layer?
Also, with IComponentConsumer, I think the code falls into the trap of extending when it should compose. It seems to me that every class which implements the interface will have the same implementation, so you should probably replace the interface with a sealed class which implements the same API and compose that into whatever classes actually use the functionality.
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
I have a slight preference for
public override bool Equals(object obj) => Equals(client as IdentifiableTcpClient);
I am confused about Message. As I read it, the client sends plain strings to the server, and the server sends Message instances back to the client. So why does Message have a property public IdentifiableTcpClientDTO SenderDTO { get; }? The sender is a server, not a client, and it should be clear from context who sent the message so it shouldn't need to be transmitted across the network.
public class ClientDataEventArgs
It's conventional that XYZEventArgs should extend EventArgs.
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
It's normally a bad idea to make a mutable collection of your state publicly available. Are you sure you don't want to have a private mutable set and make available a readonly view of it?
lock (_padlock)
{
...
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
Invoking callbacks while holding a lock is potentially a source of pain in the future. I would consider refactoring so that you modify the set while holding the lock and invoke the events after releasing it. That way any deadlock is entirely the fault of the invoking class.
catch (Exception e)
{
Console.WriteLine(e);
}
Consider using a more sophisticated logging library. Serilog, log4net, even TraceWriter.
The Encoding instance is probably worth caching in a static field. And I would suggest that since it's not 1990 any more you should prefer UTF-8 (without BOM, since UTF-8-BOM is a monstrosity).
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
Silently swallowing exceptions is worrying. You should alleviate the worry by adding some comments to explain why these particular exceptions can be safely ignored.
The BeginFoo/EndFoo style is now legacy. In new code, I would suggest using Task (async/await) style asynchronous code, because that seems to be the preferred modern option.
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
$endgroup$
I don't understand the point of the IComponent / IComponentConsumer stuff. Nothing else in the large section of code you've posted uses them. Are you sure they're at the right layer?
Also, with IComponentConsumer, I think the code falls into the trap of extending when it should compose. It seems to me that every class which implements the interface will have the same implementation, so you should probably replace the interface with a sealed class which implements the same API and compose that into whatever classes actually use the functionality.
public override bool Equals(object obj)
{
return obj is IdentifiableTcpClient client && Equals(client);
}
I have a slight preference for
public override bool Equals(object obj) => Equals(client as IdentifiableTcpClient);
I am confused about Message. As I read it, the client sends plain strings to the server, and the server sends Message instances back to the client. So why does Message have a property public IdentifiableTcpClientDTO SenderDTO { get; }? The sender is a server, not a client, and it should be clear from context who sent the message so it shouldn't need to be transmitted across the network.
public class ClientDataEventArgs
It's conventional that XYZEventArgs should extend EventArgs.
public HashSet<IdentifiableTcpClient> Clients { get; } = new HashSet<IdentifiableTcpClient>();
It's normally a bad idea to make a mutable collection of your state publicly available. Are you sure you don't want to have a private mutable set and make available a readonly view of it?
lock (_padlock)
{
...
foreach (var tcpClient in faultedClients)
{
OnDisconnectedClient(tcpClient);
}
}
Invoking callbacks while holding a lock is potentially a source of pain in the future. I would consider refactoring so that you modify the set while holding the lock and invoke the events after releasing it. That way any deadlock is entirely the fault of the invoking class.
catch (Exception e)
{
Console.WriteLine(e);
}
Consider using a more sophisticated logging library. Serilog, log4net, even TraceWriter.
The Encoding instance is probably worth caching in a static field. And I would suggest that since it's not 1990 any more you should prefer UTF-8 (without BOM, since UTF-8-BOM is a monstrosity).
catch (InvalidOperationException)
{
}
catch (IOException e)
{
if (!(e.InnerException is SocketException))
{
throw;
}
}
Silently swallowing exceptions is worrying. You should alleviate the worry by adding some comments to explain why these particular exceptions can be safely ignored.
The BeginFoo/EndFoo style is now legacy. In new code, I would suggest using Task (async/await) style asynchronous code, because that seems to be the preferred modern option.
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
answered 10 hours ago
Peter TaylorPeter Taylor
16.3k2860
16.3k2860
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212978%2ftcp-client-and-server-api%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
12 hours ago
$begingroup$
Not really, since I was playing around with it I left it at that, probably should parameterize it. @PieterWitvoet I've included the
SharedConfigurationclass it's inside the Common project, the resources are simply resource files also located in the afore mentionted assembly. If the API is public, those should probably be parameters as well.$endgroup$
– Denis
12 hours ago
$begingroup$
Just a few more it seems:
IModificationandIdentifiableTcpClient.$endgroup$
– Pieter Witvoet
11 hours ago
$begingroup$
@PieterWitvoet the
IdentifiableTcpClientis already included,IModificationreally isn't important and would add a lot of code which is of no significance for this question. Wherever it's used in the code, could be omitted, if you want to test it out.$endgroup$
– Denis
11 hours ago
$begingroup$
One thing I'd suggest adding to the wire format is an ApiVersion field. Having written several RPCs, and then wishing I'd added an ApiVersion, when I then want to tweak API, but maintain backwards compatibility at both ends…
$endgroup$
– CSM
7 hours ago