TCP client and server API The Next CEO of Stack OverflowTCP async socket server client communicationObjects sending TCP ClientSimple TCP Client Server application with receive side onlyBasic TCP server client applicationTCP Chat (Server/Client)Simple TCP client-server solutionCommon client server implementationSimple Go TCP server and clientServer-client data transferEvent Based TCP Library

My ex-girlfriend uses my Apple ID to login to her iPad, do I have to give her my Apple ID password to reset it?

How did Beeri the Hittite come up with naming his daughter Yehudit?

Won the lottery - how do I keep the money?

Yu-Gi-Oh cards in Python 3

How to Implement Deterministic Encryption Safely in .NET

Why is the US ranked as #45 in Press Freedom ratings, despite its extremely permissive free speech laws?

Does the Idaho Potato Commission associate potato skins with healthy eating?

What is the difference between "hamstring tendon" and "common hamstring tendon"?

Is it okay to majorly distort historical facts while writing a fiction story?

What day is it again?

Reshaping json / reparing json inside shell script (remove trailing comma)

Calculate the Mean mean of two numbers

From jafe to El-Guest

Can I board the first leg of the flight without having final country's visa?

What happened in Rome, when the western empire "fell"?

Computationally populating tables with probability data

Does higher Oxidation/ reduction potential translate to higher energy storage in battery?

Is it professional to write unrelated content in an almost-empty email?

What steps are necessary to read a Modern SSD in Medieval Europe?

Is it ok to trim down a tube patch?

free fall ellipse or parabola?

Does Germany produce more waste than the US?

Small nick on power cord from an electric alarm clock, and copper wiring exposed but intact

Is it convenient to ask the journal's editor for two additional days to complete a review?



TCP client and server API



The Next CEO of Stack OverflowTCP async socket server client communicationObjects sending TCP ClientSimple TCP Client Server application with receive side onlyBasic TCP server client applicationTCP Chat (Server/Client)Simple TCP client-server solutionCommon client server implementationSimple Go TCP server and clientServer-client data transferEvent Based TCP Library










6












$begingroup$


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 separated into 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.










share|improve this question











$endgroup$











  • $begingroup$
    Do you have some requirement to use "Windows-1251" as the encoding?
    $endgroup$
    – VisualMelon
    Feb 6 at 14:19










  • $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
    Feb 6 at 14:21











  • $begingroup$
    Just a few more it seems: IModification and IdentifiableTcpClient.
    $endgroup$
    – Pieter Witvoet
    Feb 6 at 14:47






  • 1




    $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
    Feb 6 at 19:18






  • 1




    $begingroup$
    The Begin/End async IO style is obsolete. Use Task and await.
    $endgroup$
    – usr
    Feb 7 at 13:28
















6












$begingroup$


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 separated into 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.










share|improve this question











$endgroup$











  • $begingroup$
    Do you have some requirement to use "Windows-1251" as the encoding?
    $endgroup$
    – VisualMelon
    Feb 6 at 14:19










  • $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
    Feb 6 at 14:21











  • $begingroup$
    Just a few more it seems: IModification and IdentifiableTcpClient.
    $endgroup$
    – Pieter Witvoet
    Feb 6 at 14:47






  • 1




    $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
    Feb 6 at 19:18






  • 1




    $begingroup$
    The Begin/End async IO style is obsolete. Use Task and await.
    $endgroup$
    – usr
    Feb 7 at 13:28














6












6








6


1



$begingroup$


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 separated into 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.










share|improve this question











$endgroup$




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 separated into 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






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited 7 mins ago









Jamal

30.5k11121227




30.5k11121227










asked Feb 6 at 13:50









DenisDenis

6,39721856




6,39721856











  • $begingroup$
    Do you have some requirement to use "Windows-1251" as the encoding?
    $endgroup$
    – VisualMelon
    Feb 6 at 14:19










  • $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
    Feb 6 at 14:21











  • $begingroup$
    Just a few more it seems: IModification and IdentifiableTcpClient.
    $endgroup$
    – Pieter Witvoet
    Feb 6 at 14:47






  • 1




    $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
    Feb 6 at 19:18






  • 1




    $begingroup$
    The Begin/End async IO style is obsolete. Use Task and await.
    $endgroup$
    – usr
    Feb 7 at 13:28

















  • $begingroup$
    Do you have some requirement to use "Windows-1251" as the encoding?
    $endgroup$
    – VisualMelon
    Feb 6 at 14:19










  • $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
    Feb 6 at 14:21











  • $begingroup$
    Just a few more it seems: IModification and IdentifiableTcpClient.
    $endgroup$
    – Pieter Witvoet
    Feb 6 at 14:47






  • 1




    $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
    Feb 6 at 19:18






  • 1




    $begingroup$
    The Begin/End async IO style is obsolete. Use Task and await.
    $endgroup$
    – usr
    Feb 7 at 13:28
















$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
Feb 6 at 14:19




$begingroup$
Do you have some requirement to use "Windows-1251" as the encoding?
$endgroup$
– VisualMelon
Feb 6 at 14:19












$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
Feb 6 at 14:21





$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
Feb 6 at 14:21













$begingroup$
Just a few more it seems: IModification and IdentifiableTcpClient.
$endgroup$
– Pieter Witvoet
Feb 6 at 14:47




$begingroup$
Just a few more it seems: IModification and IdentifiableTcpClient.
$endgroup$
– Pieter Witvoet
Feb 6 at 14:47




1




1




$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
Feb 6 at 19:18




$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
Feb 6 at 19:18




1




1




$begingroup$
The Begin/End async IO style is obsolete. Use Task and await.
$endgroup$
– usr
Feb 7 at 13:28





$begingroup$
The Begin/End async IO style is obsolete. Use Task and await.
$endgroup$
– usr
Feb 7 at 13:28











1 Answer
1






active

oldest

votes


















7












$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.






share|improve this answer









$endgroup$













    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
    );



    );













    draft saved

    draft discarded


















    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









    7












    $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.






    share|improve this answer









    $endgroup$

















      7












      $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.






      share|improve this answer









      $endgroup$















        7












        7








        7





        $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.






        share|improve this answer









        $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.







        share|improve this answer












        share|improve this answer



        share|improve this answer










        answered Feb 6 at 15:29









        Peter TaylorPeter Taylor

        18.2k2963




        18.2k2963



























            draft saved

            draft discarded
















































            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.




            draft saved


            draft discarded














            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





















































            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







            Popular posts from this blog

            कुँवर स्रोत दिक्चालन सूची"कुँवर""राणा कुँवरके वंशावली"

            Why is a white electrical wire connected to 2 black wires?How to wire a light fixture with 3 white wires in box?How should I wire a ceiling fan when there's only three wires in the box?Two white, two black, two ground, and red wire in ceiling box connected to switchWhy is there a white wire connected to multiple black wires in my light box?How to wire a light with two white wires and one black wireReplace light switch connected to a power outlet with dimmer - two black wires to one black and redHow to wire a light with multiple black/white/green wires from the ceiling?Ceiling box has 2 black and white wires but fan/ light only has 1 of eachWhy neutral wire connected to load wire?Switch with 2 black, 2 white, 2 ground and 1 red wire connected to ceiling light and a receptacle?

            चैत्य भूमि चित्र दीर्घा सन्दर्भ बाहरी कडियाँ दिक्चालन सूची"Chaitya Bhoomi""Chaitya Bhoomi: Statue of Equality in India""Dadar Chaitya Bhoomi: Statue of Equality in India""Ambedkar memorial: Centre okays transfer of Indu Mill land"चैत्यभमि