trjhtr

I have created a class which wraps the TcpClient. It's greatly simplified everything when developing.

It takes an interface for me to run callbacks where I can control what I want to do based on the network message and/or based on disconnections and connections, this makes it less hardcoded for a particular project which has been helpful when using it in other projects.

I am new to networking, and plan to try my hand at using Sockets which is a bit lower level, but wanted to start on TcpClient since it was easier.

I want to know if I have any design flaws or non robust setup which could be problematic in future.

public abstract class SocketManager

 // call back interface [see below class for example]
 private readonly iNetworkEvents _netEvent; 

 // for sending keep alives, use seperate array 
 // this to avoid conflicts with actual messages
 protected readonly byte Empty = new byte[1];

 private readonly byte _readBuffer;
 public readonly byte WriteBuffer;

 public bool IsConnected protected set; get; 

 protected TcpClient TcpClient;
 protected int KeepAliveRateMilliseconds;

 protected Stream Stream;

 private bool lockPackets;
 protected bool secure;

 protected SocketManager(int bufferSize, iNetworkEvents callback)
 
 _readBuffer = new byte[bufferSize];
 WriteBuffer = new byte[bufferSize];
 _netEvent = callback;
 

 protected async Task<bool> Connect(string address, int port)
 
 if (IsConnected) return true;
 // have to create new every time since we have to dispose when we disconnect
 TcpClient = new TcpClient(); 
 try
 
 _netEvent.Connected(secure);
 await TcpClient.ConnectAsync(address, port);
 IsConnected = true;
 return true;
 
 catch (ObjectDisposedException e)
 
 Disconnect();
 return false;
 
 catch (SocketException)
 
 Disconnect();
 return false;
 
 

 protected async Task RunKeepAlives()
 
 while (IsConnected && lockPackets == false)
 
 Empty[0] = 0;
 await Send(Empty,1); 
 await Task.Delay(KeepAliveRateMilliseconds); 
 
 
 public async Task<bool> Send()
 
 if (!IsConnected)
 return false;
 return await Send(WriteBuffer, WriteBuffer[0] + 1); 
 
 private async Task<bool> Send(byte msg, int length)
 
 if (!IsConnected)
 
 return false;
 

 if (msg == null) return false;
 try
 
 await Stream.WriteAsync(msg, 0, length); 
 return true;
 
 catch (ObjectDisposedException e)
 
 Disconnect();
 return false;
 
 

 private async Task<int> Read(byte readBuffer)
 
 if (!IsConnected)
 
 return 0;
 
 try
 
 await Stream.ReadAsync(readBuffer, 0, 1);
 return await Stream.ReadAsync(readBuffer, 1, readBuffer[0]);
 
 catch (ObjectDisposedException e)
 
 Disconnect();
 return 0;
 
 
 protected async Task _RunReader()
 
 var totalBytes = await Read(_readBuffer);
 if (totalBytes > 0)
 
 HandleReader();
 
 

 private void HandleReader()
 
 if (Enum.IsDefined(typeof(NetworkMessage), _readBuffer[1])) // valid network message was recieved
 
 switch (_readBuffer[1])
 
 case (byte)NetworkMessage.Disconnect:
 Disconnect();
 _netEvent.Disconnect(secure); // run the callback for custom callbacks
 return;
 default:
 _netEvent.MessageRecieved((NetworkMessage)_readBuffer[1], _readBuffer, 2);
 break;
 
 
 _RunReader(); //start reading the next message
 
 public async Task Disconnect(bool graceful = false)
 
 if (IsConnected && graceful)
 
 Empty[0] = (byte)NetworkMessage.Disconnect;
 await Send(Empty,1);
 
 // we lock the keep alive packets to prevent one being sent after disposal of stream in rare cases
 lockPackets = true;
 Stream?.Dispose();
 TcpClient.Close();
 IsConnected = false;
 _netEvent.Disconnect(secure);
 


public class TcpClientSecureSocket : SocketManager

 private readonly X509CertificateCollection _collection;

 public TcpClientSecureSocket(int bufferSize, X509CertificateCollection collection, iNetworkEvents callback) : base(bufferSize,callback)
 
 secure = true;
 _collection = collection;
 
 public async Task<bool> Connect(string address, int port, int keepAliveRate)
 
 bool result = await Connect(address, port);
 if (result)
 
 var networkStream = TcpClient.GetStream();
 Stream = new SslStream(networkStream,false,ValidateCert);
 SslStream stream = (SslStream) Stream;
 await stream.AuthenticateAsClientAsync(address, _collection, SslProtocols.Tls12, false);

 _RunReader();

 if (keepAliveRate > 0)
 
 KeepAliveRateMilliseconds = keepAliveRate;
 RunKeepAlives();
 
 
 return result;
 
 private bool ValidateCert(object sender, X509Certificate x509Certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
 
 return true; //todo get CA
 


public class TCPClientSocket : SocketManager

 public TCPClientSocket(int bufferSize, iNetworkEvents callback) : base(bufferSize, callback)

 public async Task<bool> Connect(string address, int port, int keepAliveRate)
 
 bool result = await Connect(address, port);
 if (result)
 
 Stream = TcpClient.GetStream();
 _RunReader();

 if (keepAliveRate > 0)
 
 KeepAliveRateMilliseconds = keepAliveRate;
 RunKeepAlives();
 
 
 return result;
 

}

Here is usage of the class for initialisation:

var certificate = X509Certificate.CreateFromCertFile(certificateDir);
certificateCollection = new X509CertificateCollection(new certificate );

ClientNonSecure = new TCPClientSocket(255, netEvents);
ClientTcpClientSecureSocket = new TcpClientSecureSocket(255, certificateCollection, netEvents);

With an example of the netEvents looking like this:

public interface iNetworkEvents

 void MessageRecieved(NetworkMessage networkMessage, byte message, int startIndex);
 void Disconnect(bool secure);
 void Connected(bool secure);

public class NetEvents : iNetworkEvents

 public void MessageRecieved(NetworkMessage networkMessage, byte message, int startIndex)
 
 switch (networkMessage)
 
 // example message
 case NetworkMessage.LoginSuccess:

 break;
 default:
 break;
 
 

 public void Disconnect(bool secure)
 
 

 public void Connected(bool secure)
 
 

If the code is looking good I'll next move on to using the Sockets class so I can support udp as well but want to make sure I have robust code first.

Some points.

1. 
   

   It is bad practice to declare protected fields, because they can be set from overridden classes, or in the base, whereas code in either place might assume that you have ownership over the lifecycle of that field. Instead declare an abstract property with the correct semantics of the base class. For instance, Stream could be declared as:

   
   
   

   protected abstract Stream Stream get;

   
   



2. Public fields are absolutely unacceptable. For example, your WriteBuffer is entirely public - any user of the class could overwrite it, set it to null etc...




3. In general the responsibilities between the base class and derived class are not properly worked out, but it seems like TCPClientSocket should be a class which wraps rather than derives from SocketManager (or possibly even a static helper method).




4. Calls to Disconnect are not awaited.




5. RunKeepAlives - you can end up with multiple instances of this running, if you connect quickly enough after a disconnect.




6. IsConnected - It would be better not to keep this as explicit state. Flags kind of violate OOP, and are leading you to write lots of checks against it. To avoid this, you could split the class into a connection and a connection factory - Connect returns a connection, which has a Disconnect method that cancels the keep alive.




7. RunKeepAlives isn't actually run as a task. This means you don't have any handle to stop it running, ever.




8. There doesn't seem to be any reason to be using a parameter in the call to Read() since the method is an instance method and has access to the field that is used in all calls. Should either be static (not necessary), or (better) _RunReader and Read should be one method.




9. In general, the whole class is leaking it's internals over it's public interface.

Edit: Answers to your questions.

1 - the derived class does protected override Stream Stream get; and sets it in the constructor.

2) Send the data with the call, rather than modifying state that you don't own.
4) If you call Disconnect() immediately followed by Connect() the call will fail. Worse, if you change it, the call might succeed while the disconnect is still ongoing.

5) By calling the method more than once. Ever.

6) Split into a connection factory (the connect method of which returns a Connection instance) which has a disconnect method.

7) It doesn't return a task to the caller and runs forever. You need some way of stopping it (e.g. inject a cancellation token).

9) You exposed all your fields, and the callers kind of need to be aware of the state of the class when they use it. The design for this type of thing should be "what do I need the public interface to be" - perhaps even write this as an interface to start with - and then fill in the implementation.