Socket server in c#

Question

I'm writing a socket server for a project I am working on. I'm used to working in .NET FW, but I'm attempting to write this using .NET standard/core. Particularly, I am concerned about managing configuration, test-ability, and DI. Guidance on how to achieve this would be greatly appreciated.

I included the classes that I thought were important. I have the following concerns.

1. In .NET framework, configuration is usually accessed statically and not injected like it is done here. With many configuration values, how should I manage configuration in .NET standard? For this reason, I thought I could create factory classes like PacketFactory.cs that are built using configuration one time on startup. Is this a good or bad strategy?
2. I've already written a few unit tests for classes, and I'm noticing that I end up with a lot of interfaces and factories (see point #1) just for testing - I assume this is a good thing, but getting concerned with if it will end up as overkill.
3. Connection.cs constructor has a lot of parameters, but I don't think that it has too much responsibility; I can even think of more parameters to add later.
4. I'm starting to see Program.cs (or whatever entry point) is going to have a lot of code just to build up a bunch of objects using config values that ultimately get injected later. Is this OK or expected?

There are of course a lot of other things that need to be improved upon, but as this is my first time writing something really thinking about DI and using IoC, I have doubts about the approaches I'm taking.

Program.cs : exists as a separate project that calls into the networking library.

class Program
{
    private static IConfigurationRoot _configuration;

    static void Main(string[] args)
    {
        _configuration = new ConfigurationBuilder()
            .SetBasePath(Directory.GetCurrentDirectory())
            .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
            .Build();

        var services = new ServiceCollection();
        Program.RegisterNetwork(services);
        Program.RegisterLogging(services);

        var provider = services.BuildServiceProvider();
        var service = provider.GetService<INetworkService>();

        service.Start();

        while(true)
        {
            Thread.Sleep(1000);
        }
    }

    private static void RegisterNetwork(IServiceCollection services)
    {
        var settings = new NetworkSettings();
        var section = _configuration.GetSection("Project").GetSection("NetworkSettings");
        section.Bind(settings);

        // TODO: I would really like to figure out how to avoid double binding.
        services.Configure<NetworkSettings>(options => section.Bind(options));

        var packetWriter = new PacketFactory(settings.MinPacketSize);
        services.AddSingleton(packetWriter.GetType(), packetWriter);

        // Setup the cryptographic scheme.
        var index = settings.CryptorIndex;
        var selection = settings.CryptorKeySelectionType;
        var cryptor = new Cryptor(index, selection);

        var handler = new BarrackPacketHandler();
        var connection = new ConnectionFactory(Log.Logger, handler, packetWriter, cryptor, settings.MaxPacketSize, settings.MinPacketSize);

        services.AddSingleton<IPacketHandler>(handler);
        services.AddSingleton<ConnectionFactory>(connection);
        services.AddSingleton<INetworkService, NetworkService>();
    }

    private static void RegisterLogging(IServiceCollection services)
    {
        Log.Logger = new LoggerConfiguration()
            .ReadFrom.Configuration(_configuration)
            .CreateLogger();

        services.AddSingleton(Log.Logger);
    }
}

NetworkService.cs

public class NetworkService : INetworkService
{
    private ConnectionFactory _connectionFactory;
    private NetworkSettings _settings;
    private ILogger _logger;

    /// <summary>
    /// Socket responsible for accepting incoming connection requests.
    /// </summary>
    private Socket _listener;
    private List<Connection> _connections = new List<Connection>();

    public NetworkService(IOptions<NetworkSettings> settings, ILogger logger, ConnectionFactory connectionFactory)
    {
        _settings = settings.Value;
        _logger = logger;
        _connectionFactory = connectionFactory;
        _listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    }

    public void Start()
    {
        IPAddress ip;
        IPAddress.TryParse(_settings.IPAddress, out ip);
        var endpoint = new IPEndPoint(ip, _settings.Port);

        _listener.Bind(endpoint);
        _listener.Listen(_settings.Backlog);

        _listener.BeginAccept(this.AcceptCallback, null);
    }

    private void AcceptCallback(IAsyncResult result)
    {
        try
        {
            var client = _listener.EndAccept(result);
            var connection = _connectionFactory.Create(client);
            _connections.Add(connection);
        }
        finally
        {
            // In any circumstance, the server needs to be able to accept new connections without faulting.
            _listener.BeginAccept(this.AcceptCallback, null);
        }
    }
}

Connection.cs

/// <summary>
/// Represents the state of a remote connection to a single client.
/// </summary>
public class Connection
{
    private static ILogger _logger;

    /// <summary>
    /// Specifies the source class containing packet handling functions.
    /// </summary>
    private static IPacketHandler _handler;

    /// <summary>
    /// Used to create packets for reading and writing.
    /// </summary>
    private static PacketFactory _packetFactory;

    /// <summary>
    /// Socket to a remote client.
    /// </summary>
    private Socket _client;

    /// <summary>
    /// Cryptographic implementation to secure packets.
    /// </summary>
    private Cryptor _cryptor;

    /// <summary>
    /// The receive buffer where bytes are written to.
    /// </summary>
    private byte[] _buffer;

    public int BufferSize { get; private set; }
    public int MinPacketSize { get; private set; }

    public Connection(ILogger logger, IPacketHandler handler, PacketFactory packetFactory, Socket client, Cryptor cryptor, int bufferSize, int minPacketSize)
    {
        _logger = logger;
        _handler = handler;
        _packetFactory = packetFactory;
        _client = client;
        _cryptor = cryptor;
        _buffer = new byte[bufferSize];

        this.BufferSize = bufferSize;
        this.MinPacketSize = minPacketSize;

        _client.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, this.ReceiveCallback, null);
    }

    /// <summary>
    /// Callback that is invoked upon receiving packets.
    /// </summary>
    /// <param name="result"></param>
    private void ReceiveCallback(IAsyncResult result)
    {
        try
        {
            var bytesRead = _client.EndReceive(result);

            // If there aren't any bytes received, then transmission is finished.
            if (bytesRead <= 0)
            {
                return;
            }

            var cursor = 0;
            while (cursor < bytesRead)
            {
                // Filter out any packets with invalid lengths.
                // The smallest packet size + the 16-bit header defining the length.
                var remaining = bytesRead - cursor;
                if (remaining < this.MinPacketSize + sizeof(short))
                {
                    // TODO: Log
                    break;
                }

                // The length of the packet is defined as a short at the beginning.
                // Valid lengths should be greater than '0' and complete packets (no fragments).
                var length = BitConverter.ToInt16(_buffer, 0);
                if (length > remaining || length <= 0)
                {
                    break;
                }

                var packet = new byte[length];

                // This will be the offset into the buffer to read following packets.
                cursor += sizeof(short);
                Buffer.BlockCopy(_buffer, cursor, packet, 0, length);
                cursor += length;

                _cryptor.Decrypt(packet, 0, length);
                var reader = _packetFactory.CreateReader(packet);
                _handler.Handle(this, reader);
            }

            // Clearing the buffer may not be necessary, but it is done for safety.
            Array.Clear(_buffer, 0, _buffer.Length);
            _client.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, this.ReceiveCallback, null);
        }
        catch (Exception e)
        {
            _logger.Error(e.Message, e);
        }   
    }
}

PacketHandler.cs

/// <summary>
/// Defines methods for handling packets with various opcodes.
/// Implement this class by extending it with methods having the [PacketHandlerAttribute] attribute.
/// </summary>
public abstract class PacketHandler : IPacketHandler
{
    /// <summary>
    /// Contains a mapping of opcodes to handler functions.
    /// </summary>
    private static Dictionary<PrimaryOp, PacketHandlerMethod> _handlerMap;

    /// <summary>
    /// Defines the format for a packet handler function.
    /// </summary>
    /// <param name="conn"></param>
    /// <param name="reader"></param>
    private delegate void PacketHandlerMethod(Connection conn, PacketReader reader);

    /// <summary>
    /// Constructs a handler by registering methods for each packet type.
    /// </summary>
    public PacketHandler()
    {
        _handlerMap = new Dictionary<PrimaryOp, PacketHandlerMethod>();

        var methods = this.GetType().GetMethods();
        foreach (var method in methods)
        {
            var attributes = method.GetCustomAttributes(typeof(PacketHandlerAttribute), false);
            foreach (PacketHandlerAttribute attribute in attributes)
            {
                var opcode = attribute.Opcode;
                if (_handlerMap.ContainsKey(opcode))
                {
                    throw new Exception($"Unable to register handler for {opcode.ToString()} because it has already been registered!");
                }

                var del = (PacketHandlerMethod)Delegate.CreateDelegate(typeof(PacketHandlerMethod), this, method);

                _handlerMap.Add(opcode, del);
            }
        }
    }

    /// <summary>
    /// Looks up a handler for the specified opcode and runs it.
    /// </summary>
    /// <param name="reader"></param>
    public void Handle(Connection conn, PacketReader reader)
    {
        var opcode = reader.Opcode;
        if (!_handlerMap.ContainsKey(opcode))
        {
            // TODO: Log it?
            return;
        }

        var handler = _handlerMap[opcode];
        if (handler == null)
        {
            // TODO: log it?
            return;
        }

        handler(conn, reader);
    }
}

PacketFactory.cs

public class PacketFactory
{
    public readonly int MinPacketSize;
    public PacketFactory(int minPacketSize)
    {
        this.MinPacketSize = minPacketSize;
    }

    public PacketWriter CreateWriter(PrimaryOp opcode)
    {
        return new PacketWriter(opcode, this.MinPacketSize);
    }

    public PacketReader CreateReader(byte[] buffer)
    {
        return new PacketReader(buffer, this.MinPacketSize);
    }
}

Answer 1

configuration is usually accessed statically

I have a different impression. Configuration is accessed in a way that is convenient in terms of maintainability and testing. And static references do not help in those areas. It is common to register settings in DI containers and either inject them directly or using some sort of wrapper (probably what you have with IOptions interface). There is nothing wrong with that.

constructor has a lot of parameters, but I don't think that it has too much responsibility

IMHO, it has everything to do with responsibility. Your class manages too many entities. I can think of two patterns that can be useful here:

• A decorator pattern. Take a look at how streams are organized. How you can wrap a buffering stream around a file stream to get a file stream with a large buffer. Don't you think that, for example, encryption can be added to existing connection by wrapping around it? It will require a change in signature, but maybe it is a good thing?
• A pipeline pattern. Why should connection know every step which needs to be taken to process a message. First decrypt it, then create reader, then pass a handler, then continue processing inside a handler... That sounds like a pipeline to me. Wouldn't it be nicer if your connection was only responsible for reading an array of bytes? Once you have the binary packet you could just pass it to some IMessagePipeLine.Process(byte[]) method. Or maybe just fire some event Action<byte[]> PacketReceived and let the outer code handle the data? P.S. If you are interested in this approach check out TPL.Dataflow library, it has some neat classes for building an async pipeline, and I believe it is available under .Net Core.

a lot of code just to build up a bunch of objects using config values

Is there a reason why you can't inject the settings directly in your component? Something along those lines:

//register settings inside container
services.AddSingleton<CryptoSettings>(here_goes_factory_method_that_loads_settings);
//register crypto service wich has a dependency on settings class
services.AddSingleton<ICryptor, Cryptor>();

class Cryptor : ICryptor
{
     public Cryptor(CryptoSettings settings) {...}
     ...
}

The point of DI containers is that you let container resolve dependencies for you. You on the other hand do most of the heavy lifting manually.

That being said, complex application can require hundreds or even thousands of lines just to register all the components. That can be tedious, but it is a price that you have to pay for DI container. However it is worth mentioning, that containers provide some powerful features when it comes to registration. For example, most containers allow you to automatically register all services that implement an interface / located in certain namespace / marked with an attribute. Some containers has build-in support for configuration files and can inject configuration values into registered components directly from xml file, as long as the file structure follows specified format.

A few other things that I noticed:

1. What is this infinite loop doing inside your program class? Apart from periodically stealing processor core from other threads? Why not run your listener inside the main thread synchronously?

2. There is no clean up on Program close. Connections are never terminated and DI container is never disposed.

3. _handler.Handle(this, reader); - this should rise red flags. Circular dependencies are almost always an indication of some poor design choices. So whenever you have to pass a this reference to a method, try to figure out why it is necessary. Maybe instead what you actually need is another service, that both classes ("handler" and "connection") can depend on and share.