Implementing Dynamic Network Message Handling

Question

I'm trying to handle network messages in a dynamic, and effective way, though I feel I've overthought it again. As of now, I have a very structured approach, but it feels as if it's a lot of overhead for such a simple process.

The other issue, is that if I start making more types of Login messages, then I will have many small classes floating around. Is this an appropriate design? Should I allow micro-classes here?

This code stems from the code mentioned over on this other question. Though, I am working on a different portion of it now. That said, most (if not all) of the suggestions from that question have been implemented in some manner.

I have rewritten the entire NetworkServer as follows:

public class NetworkServer
{
    private readonly NetServer _MainNetServer;
    private readonly HashSet<NetConnection> _ConnectedClients = new HashSet<NetConnection>();
    private readonly ILogger _Logger;
    private readonly DataMessageHandler _DataMessageHandler;

    public NetworkServer(NetPeerConfiguration serverConfiguration, ILogger logger)
    {
        _Logger = logger;

        _Logger.LogInformation("Initializing server...");
        _MainNetServer = new NetServer(serverConfiguration);
        _DataMessageHandler = new DataMessageHandler(_Logger);
    }

    public Task RunServer()
    {
        return Task.Run(() =>
        {
            _MainNetServer.Start();
            _Logger.LogImportant("Server starting on socket {0}...", _MainNetServer.Socket.LocalEndPoint);
            DateTime started = DateTime.UtcNow;

            //_Logger.LogImportant(_Logger.FormatMessage("The server was started on {0} at {1}.", started.ToString("dd-MM-yyyy"), started.ToString("HH:mm:ss.fffffff")));
            _Logger.LogImportant("The server was started at: {0}", started.ToString("O"));

            while (true)
            {
                NetIncomingMessage msg;
                while ((msg = _MainNetServer.ReadMessage()) != null)
                {
                    switch (msg.MessageType)
                    {
                        case NetIncomingMessageType.VerboseDebugMessage:
                        case NetIncomingMessageType.DebugMessage:
                        case NetIncomingMessageType.WarningMessage:
                        case NetIncomingMessageType.ErrorMessage:
                            ProcessErrorMessage(msg);
                            break;
                        case NetIncomingMessageType.StatusChanged:
                            ProcessStatusChangeMessage(msg.SenderConnection);
                            break;
                        case NetIncomingMessageType.Data:
                            ProcessDataMessage(msg);
                            break;
                        default:
                            _Logger.LogWarning("Unhandled type: {0}", msg.MessageType);
                            break;
                    }
                    _MainNetServer.Recycle(msg);
                }

                System.Threading.Thread.Sleep(1);
            }
        });
    }

    private void ProcessDataMessage(NetIncomingMessage message)
    {
        _Logger.LogInformation("Data recieved from: {0}, payload size: {1} bytes", message.SenderConnection.RemoteEndPoint, message.LengthBytes);

        IDataMessage messageResult = _DataMessageHandler.HandleMessage(message);

        switch (messageResult.Type)
        {
            case DataMessageType.Login:
                // Do login-ey things
                string username = ((LoginMessage)messageResult).Username;
                byte[] password = ((LoginMessage)messageResult).Password;
                break;
        }
    }

    private void ProcessErrorMessage(NetIncomingMessage message)
    {
        _Logger.LogError("Network error: ", message.ReadString());
    }

    private void AddClient(NetConnection client)
    {
        if (_ConnectedClients.Add(client))
            _Logger.LogInformation("New client discovered: {0}", client.RemoteEndPoint);
        else
            _Logger.LogError("Duplicate client discovered: {0}", client.RemoteEndPoint);
    }

    private void RemoveClient(NetConnection client)
    {
        if (_ConnectedClients.Contains(client))
            _ConnectedClients.Remove(client);

        _Logger.LogInformation("Client disconnected: {0}", client.RemoteEndPoint);
    }

    private void ProcessStatusChangeMessage(NetConnection connection)
    {
        switch (connection.Status)
        {
            case NetConnectionStatus.Disconnected:
                RemoveClient(connection);
                break;
            case NetConnectionStatus.Connected:
                AddClient(connection);
                break;
            default:
                _Logger.LogWarning("Unhandled StatusChanged: {0} now {1}", connection.RemoteEndPoint, connection.Status);
                break;
        }
    }
}

Then I added an interface called IDataMessage, which represents a message that contains some data:

public interface IDataMessage
{
    ILogger Logger { get; }
    DataMessageType Type { get; }

    void WriteNetworkMessage(NetOutgoingMessage message);
    void ReadNetworkMessage(NetIncomingMessage message);
}

I have a LoginMessage which represents a username/password combination:

public class LoginMessage : IDataMessage
{
    private readonly ILogger _Logger;
    public ILogger Logger { get { return _Logger; } }

    public string Username { get; private set; }
    public byte[] Password { get; private set; }

    public DataMessageType Type { get { return DataMessageType.Login; } }

    public LoginMessage(ILogger logger, string username, byte[] password)
        : this(logger)
    {
        Username = username;
        Password = password;
    }

    public LoginMessage(ILogger logger)
    {
        _Logger = logger;
    }

    public void WriteNetworkMessage(NetOutgoingMessage message)
    {
        message.Write((int)Type);
        message.Write(Username);
        message.Write((byte)Password.Length);
        message.Write(Password);
    }

    public void ReadNetworkMessage(NetIncomingMessage message)
    {
        Username = message.ReadString();
        byte passLength = message.ReadByte();
        Password = message.ReadBytes(passLength);
    }
}

The DataMessageType enum:

public enum DataMessageType
{
    Login,
}

And lastly, the DataMessageHandler class which builds the appropriate IDataMessage:

public class DataMessageHandler
{
    private readonly ILogger _Logger;
    public ILogger Logger { get { return _Logger; } }

    public DataMessageHandler(ILogger logger)
    {
        _Logger = logger;
    }

    public IDataMessage HandleMessage(NetIncomingMessage message)
    {
        DataMessageType type = (DataMessageType)message.ReadInt32();
        IDataMessage dataMessage = null;

        switch (type)
        {
            case DataMessageType.Login:
                dataMessage = new LoginMessage(_Logger, message);
                break;
            default:
                _Logger.LogWarning("Unhandled DataMessageType: {0}", type);
                break;
        }

        return dataMessage;
    }
}

Then, on the client, it's written as so:

NetOutgoingMessage sendMsg = client.CreateMessage();

using (SHA256 sha = SHA256.Create()) // SHA256 is by default a 32-byte hash.
{
    LoginMessage loginMessage = new LoginMessage(mainLogger, "EBrown", sha.ComputeHash(System.Text.Encoding.UTF8.GetBytes("Password")));
    loginMessage.WriteNetworkMessage(sendMsg);
}

client.SendMessage(sendMsg, NetDeliveryMethod.ReliableOrdered);

Now, for every message I add, I have to do several things:

1. Add to the DataMessageType enumeration.
2. Build a class that can inherit and implement IDataMessage.
3. Add the appropriate code in DataMessageHandler to build and return the correct IDataMessage.
4. Update private void ProcessDataMessage(NetIncomingMessage message) for the new type.

As implied by the site title, I am looking for any and all suggestions.

Answer 1

Building on top of @RubberDuck's answer

You should use the kind of interface @RubberDuck suggested if you have messages which should be processed using the implementing class itself which doesn't need to return a result (void Process()) and if it should return something you can stick to it too, as long as it is for every implementing class makes sense.

That being said, let us check what we can do for the specific tasks like your LoginMessage.

Ideally one class should have only one responsibility and only one reason to change.

Sometimes the circumstances makes it necessary that you violate this principle, but you can make the violation less evil.

If we check the NetworkServer class for what could cause itself to change, we can identify

• NetServer
• NetConnection
• DataMessageHandler
• NetIncomingMessage

because you use these as classes but not as interfaces. If you change the name of the class, or its implementation by removing or adding methods etc, you will also need to change the NetworkServer class.

I would like to suggest using interfaces here. Instead of injecting the NetPeerConfiguration into the constructor, you should just inject an INetServer interface.

Using interfaces makes your code mockable and therefor easier to test.

The INetServer could use these methods and properties

INetIncomingMessage ReadMessage()
void Recycle(INetIncomingMessage);
void Start();
EndPoint LocalEndPoint { get; }  

and your NetworkServer class wouldn't depend on the concrete NetServer class anymore.

If we think this further, it could also be a good idea if a class which should implement the IDataMessageHandler interface would live in the class which implements the INetServer interface. Doing this and using topshelf you could turn this in a service (which can be run as a console application too) skipping the while(true) loop and instead using events which then would be processed by the NetworkServer class.

In this way the NetworkServer would only need to change if the underlying INetServer implementation would change.

If you have figured out what type of messages should be processed you should add an event for each of them to the INetServer interface keeping one event with one event which passes an IDataMessage as event argument for future extensions.

Why should you do this ? An interface declaration shouldn't change because it would otherwise break all implementing classes. Using an "abstract" event leaves you the possibility to process future changes too but now in the NetworkServer class.

Let us assume you will have the need to have a IDatabaseDataMessage and this should be processed by the NetworkServer this interfaces should implement the IDataMessage interface and could therefor be passed through the event and handled like so

private void INetServer_DataMessageReceived(object sender, DataMessageEventArgs e)
{
    IDataMessage msg = e.Message;
    IDatabaseDataMessage dbMsg = msg as IDatabaseDataMessage;
    if (dbMsg != null) 
    {
        ProcessDatabaseMessage(dbMsg);
        return;
    }
    // process other future extensions here

}

Basically this all boils down to this

• leave yourself some time to think over your goals of the server
• think about what type of messages needs to be processed and think about it carefully
• for each of these messages add an event to your INetServer and to the implementing class
• add one generally event to the interface

EDIT

Based on the comments and the chatting the problem is like that, that the NetServer and the related classes are out of your control.

This screams for the adapter pattern which you should place between your NetworkServer class and the NetServer class. The purpose here is not to help your application to understand the 3rd party server, but to hide deep down the code of the maybe ugly convertation of the NetIncomingMessage.

Answer 2

I really don't like this enum and "type" member.

public enum DataMessageType
{
    Login,
}


public interface IDataMessage
{
    ILogger Logger { get; }
    DataMessageType Type { get; }

    void WriteNetworkMessage(NetOutgoingMessage message);
    void ReadNetworkMessage(NetIncomingMessage message);
}

The idea behind polymorphism is that so long as the type adheres to an interface, the client code just doesn't care what the concrete type really is. As you've noticed, this implementation requires you adjust an enum, and at least two other classes every time you add a new child type of IDataMessage. This isn't how it's supposed to work.

Right now, the only thing HandleMessage does is turn a NetIncomingMessage into an IDataMessage. This is good. It's a single responsibility, but the name isn't the greatest. It's a factory class that adapts an outside interface into an internal one. Consider a rename for it, but over all it's good. You need a class that is responsible for determining which type of IDataMessage to create based on the state of NetIncomingMessage. It makes sense to have to update this class anytime you create a new implementation of IDataMessage. Don't worry about that.

This here is where things get not so good.

    private void ProcessDataMessage(NetIncomingMessage message)
    {
        _Logger.LogInformation("Data recieved from: {0}, payload size: {1} bytes", message.SenderConnection.RemoteEndPoint, message.LengthBytes);

        IDataMessage messageResult = _DataMessageHandler.HandleMessage(message);

        switch (messageResult.Type)
        {
            case DataMessageType.Login:
                // Do login-ey things
                string username = ((LoginMessage)messageResult).Username;
                byte[] password = ((LoginMessage)messageResult).Password;
                break;
        }
    }

This is where it just shouldn't matter what type of IDataMessage we have. The code should just work so long as the message returned from the handler adheres to the interface.

We can fix this by letting each IDataMessage define it's own behavior. First, let's adjust the interface a little bit.

public interface IDataMessage
{
    ILogger Logger { get; }

    void WriteNetworkMessage(NetOutgoingMessage message);
    void ReadNetworkMessage(NetIncomingMessage message);

    void Process(); // void may not be appropriate, I don't have enough context to say.
}

Note that I removed the "type" enum. If we ever really just have to know exactly what concrete type we're working with, we can call typeof() instead. Now we move the interesting code that used to be in ProcessDataMessage to LoginMessage.

public class LoginMessage : IDataMessage
{
    //...

    void Process()
    {
        // processing code from ProcessDataMessage
    }

    //...
}

Then we adjust the original method accordingly, ending up with much simpler code here.

    private void ProcessDataMessage(NetIncomingMessage message)
    {
        _Logger.LogInformation("Data recieved from: {0}, payload size: {1} bytes", message.SenderConnection.RemoteEndPoint, message.LengthBytes);

        IDataMessage messageResult = _DataMessageHandler.HandleMessage(message);

        messageResult.Process();
    }

See how it doesn't matter what type we get back from the factory? All of the differentiating logic is stored and implemented in the specific concrete implementations.