1
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Singe your class holds an IDisposable-implementing type (TcpClient), it needs to also implement IDisposable. \$\endgroup\$ – Jesse C. Slicer Jan 8 '18 at 1:59
  • \$\begingroup\$ I don't understand? Why does the wrapper need it? I don't dispose these wrappers when i dispose TcpClient, they just create a new TcpClient on Connect. \$\endgroup\$ – WDUK Jan 8 '18 at 2:01
  • \$\begingroup\$ I don't know how to state it any other way: if your class creates and holds an IDisposable object, it should implement the IDisposable interface itself to ensure proper, deterministic disposal of those types. This has been framework design guidelines since 2003. stackoverflow.com/a/898867/3312 might help. \$\endgroup\$ – Jesse C. Slicer Jan 8 '18 at 15:14
  • \$\begingroup\$ Does implementing the interface even do anything though, since i already dispose ? Or is it just some syntax sugar ? \$\endgroup\$ – WDUK Jan 8 '18 at 22:17
  • \$\begingroup\$ If you can 100% guarantee that the .Dispose() calls in the Disconnect() method are always reached for every call to Connect(), then you don't need to do that. Otherwise, implement the interface to allow your callers to dispose the object. \$\endgroup\$ – Jesse C. Slicer Jan 8 '18 at 22:23
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for editing in some answers, i need to do some thinking on that connection factory idea. :) \$\endgroup\$ – WDUK Jan 10 '18 at 6:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.