Implementing Dynamic Network Message Handling (Part II)

Question

This is an iterative review request of this other question I posted.

As a follow up to that previous question, I changed the pattern completely and I went to an events-based approach, which might be more relevant.

---

Pre-text and background story

To address some of the points in that question (to hopefully avoid repeats):

1. The DataMessageType enum:

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

Unfortunately, that enum is what is sent "over-the-wire" between the clients and the server to determine what packet was actually sent. It's hard to remove because I need a way to determine what packet type it was. That said, I'll get to the way I reworked it further down this message.

---

2. Using DataMessageType on the server:

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 was actually a very nice suggestion, and the basis for beginning my rewrite. Instead of using that case statement I went to an events approach, so that the main NetworkServer is not responsible for directly knowing which message type it is.

---

3. Interfaces for Net* classes:

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.

Unfortunately, these classes come from a separate project, and I can't won't change them due to the fact that the aforementioned "other" project is source I do not own. (I have it, but I don't own it.) To maintain compatibility (if that source is ever updated and I don't want to merge changes) I can't really use this suggestion. (I would love to, but it's no conceivable.)

---

4. Adapter Pattern (this is where it gets interesting):

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.

This is something I'm up for discussing. With the changes I made, the adapter pattern is looking more and more relevant.

---

On to the code!

So, in lieu of a better way to do things, I switched to an events pattern (which could easily be combined with the adapter pattern):

We'll start with the DataMessageHandler, since it was the most rewrite:

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);
                dataMessage.ReadNetworkMessage(message);
                OnLoginMessageReceived(new LoginMessageEventArgs((LoginMessage)dataMessage));
                break;
            case DataMessageType.ChatMessage:
                dataMessage = new ChatMessage(_Logger);
                dataMessage.ReadNetworkMessage(message);
                OnChatMessageReceived(new ChatMessageEventArgs((ChatMessage)dataMessage));
                break;
            default:
                _Logger.LogWarning("Unhandled DataMessageType: {0}", type);
                OnUnhandledMessageReceived(new MessageEventArgs(dataMessage));
                break;
        }

        return dataMessage;
    }

    protected void OnLoginMessageReceived(LoginMessageEventArgs e)
    {
        if (LoginMessageReceived != null)
            LoginMessageReceived(this, e);
    }

    protected void OnChatMessageReceived(ChatMessageEventArgs e)
    {
        if (ChatMessageReceived != null)
            ChatMessageReceived(this, e);
    }

    protected void OnUnhandledMessageReceived(MessageEventArgs e)
    {
        if (UnhandledMessageReceived != null)
            UnhandledMessageReceived(this, e);
    }

    public event LoginMessageReceivedEventHandler LoginMessageReceived;
    public event ChatMessageReceivedEventHandler ChatMessageReceived;
    public event UnhandledMessageReceivedEventHandler UnhandledMessageReceived;
}

Essentially, I added two events (mostly for context), and instead of just returning an IDataMessage, it also throws an event related to the message it got.

The delegates and their awfully long names:

public delegate void LoginMessageReceivedEventHandler(Object sender, LoginMessageEventArgs e);
public delegate void ChatMessageReceivedEventHandler(Object sender, ChatMessageEventArgs e);
public delegate void UnhandledMessageReceivedEventHandler(Object sender, MessageEventArgs e);

The EventArgs classes:

public class MessageEventArgs : EventArgs
{
    private IDataMessage _Message;
    public IDataMessage Message { get { return _Message; } }

    public MessageEventArgs(IDataMessage message)
    {
        _Message = message;
    }

    protected internal MessageEventArgs()
    {
        // So that child classes within this assembly don't need to worry about the `Message` parameter, which they hide anyway. (So that they can strongly type it.)
    }
}

public class LoginMessageEventArgs : MessageEventArgs
{
    private LoginMessage _Message;
    public new LoginMessage Message { get { return _Message; } }

    public LoginMessageEventArgs(LoginMessage message)
    {
        _Message = message;
    }
}

public class ChatMessageEventArgs : MessageEventArgs
{
    private ChatMessage _Message;
    public new ChatMessage Message { get { return _Message; } }

    public ChatMessageEventArgs(ChatMessage message)
    {
        _Message = message;
    }
}

So, this makes sure that all the events can have the appropriate information when handled on the server.

That last part is a bit I really want some advice on (especially that protected internal and all the new bits). I'm not sure if I'm going about things correctly here or not, and I would greatly appreciate some advice.

Next, the NetworkServer:

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

    /// <summary>
    /// Creates a new instance of the <see cref="NetworkServer"/> from the <c>NetPeerConfiguration</c> and <see cref="ILogger"/>.
    /// </summary>
    /// <param name="serverConfiguration">The intial configuration of the <see cref="NetServer"/>.</param>
    /// <param name="logger">The <see cref="ILogger"/> that the server should log to.</param>
    public NetworkServer(NetPeerConfiguration serverConfiguration, ILogger logger)
    {
        _Logger = logger;

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

        _DataMessageHandler.LoginMessageReceived += _DataMessageHandler_LoginMessageReceived;
        _DataMessageHandler.ChatMessageReceived += _DataMessageHandler_ChatMessageReceived;
    }

    /// <summary>
    /// This will run the server main loop. This should generally be waited on the other side (<c>RunServer().Wait()</c> or something).
    /// </summary>
    /// <returns>A <c>Task</c> that contains the work for the <see cref="NetworkServer"/> to do.</returns>
    /// <remarks>
    /// Why am I documenting this? It's only ever used once.
    /// 
    /// Because it's a publicly facing method, damnit!
    /// </remarks>
    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);
    }

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

    private void _DataMessageHandler_LoginMessageReceived(object sender, LoginMessageEventArgs e)
    {
        NetConnection client = e.Message.Client;
        string username = e.Message.Username;
        byte[] password = e.Message.Password;

        _Logger.LogInformation("Client ({0}) attempted login: {1}", client.RemoteEndPoint, username);
    }

    private void _DataMessageHandler_ChatMessageReceived(object sender, ChatMessageEventArgs e)
    {
        NetConnection client = e.Message.Client;
        string message = e.Message.Message;

        _Logger.LogInformation("Client ({0}) attempted chat: {1}", client.RemoteEndPoint, message);
    }
}

The new ChatMessage:

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

    private NetConnection _Client;
    public NetConnection Client { get { return _Client; } }

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

    private string _Message;
    public string Message { get { return _Message; } }

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

    public ChatMessage(ILogger logger, string message)
        : this(logger)
    {
        _Message = message;
    }

    public void WriteNetworkMessage(NetOutgoingMessage message)
    {
        message.Write((int)Type);
        message.Write(_Message);
    }

    public void ReadNetworkMessage(NetIncomingMessage message)
    {
        _Client = message.SenderConnection;
        _Message = message.ReadString();
    }
}

And lastly, the enum:

public enum DataMessageType
{
    Login,
    ChatMessage,
}

As you can see, there are still a few changes that need made for each message I add, though they are changes I am "OK" with:

1. I need to add an event and event methods (Easy).
2. I need to add event args maybe (Not always).
3. I need to subscribe to the new event (Easy).
4. I need to throw the new event (Easy).

This makes it easier to track what changes need made (makes sense, we're throwing an event for each message!).

The problem: the number of events could get out of hand. (How out of hand? I'm not sure yet!)

Any and all suggestions are welcome. I have stated my main concerns, I would appreciate if the brunt of the focus were on them.

Answer 1

I would use the adapter pattern (because I have suggested to use it), abstracting each needed class by an interface. In this way you add the ability to mock and test your code as much as you want.

Also this is a lot more code (if you extract the classes to interfaces) but if your application is business critical, it should be testable on all possible problems.

The minimum I would do would be to introduce the INetServerAdapter like so

public interface INetServerAdapter
{
    void Start();

    NetIncomingMessage ReadMessage();

    void Recycle(NetIncomingMessage message);

    event EventHandler<LoginMessageEventArgs> LoginMessageReceived;
    event EventHandler<ChatMessageEventArgs> ChatMessageReceived;

}

where the events are only 2 of them (you can add more) .

This interface should then be implemented by the NetServerAdapter class like so

public class NetServerAdapter : INetServerAdapter
{

    private readonly NetServer server;

    public NetServerAdapter(NetPeerConfiguration configuration)
    {
        server = new NetServer(configuration);
    }
    public void Start()
    {
        InternalStart();
    }

    private void InternalStart()
    {
        server.Start();
    }

    NetIncomingMessage INetServerAdapter.ReadMessage()
    {
        return server.ReadMessage();
    }

    public void Recycle(NetIncomingMessage message)
    {
        throw new NotImplementedException();
    }

    public event EventHandler<LoginMessageEventArgs> LoginMessageReceived;

    public event EventHandler<ChatMessageEventArgs> ChatMessageReceived;

}  

with the examplary LoginMessageEventargs class like so

public class LoginMessageEventArgs : EventArgs
{
    public NetConnection Client { get; private set; }
    public string Username { get; private set; }
    public byte[] Password { get; private set; }
    public LoginMessageEventArgs(NetIncomingMessage message)
    {
        Client = message.Client;
        Username = message.Username;
        Password = message.Password;
    }
}

whereas other eventargs and events should be added, if needed.

As you see I have added a private void InternalStart() method to the NetServerAdapter class. Here you can either place your while(true) loop or much better you could register a callback to the lidgren-network-gen3 server like it is shown here.

If your application is completely event/UI-driven and does not have an application loop per se, you can register a callback to fire when a message arrives. The registration must happen from the correct SynchronizationContext (ie. call RegisterReceivedCallback from the same thread you want to receive the message notification on)

peer.RegisterReceivedCallback(new SendOrPostCallback(GotMessage)); 

public static void GotMessage(object peer)
{
        var msg = peer.ReadMessage();
        // process message here
}

where you need to cast the object peer to a NetServer.

This callback instead of the while(true) loop will decrease the work for the CPU.

Now all you have to do is creating a NetServerAdapter object in your NetworkServer class and register to the events.

Answer 2

This seems like a good approach... Only got time for a quick comment but might come back later:

protected void OnLoginMessageReceived(LoginMessageEventArgs e)
{
    if (LoginMessageReceived != null)
        LoginMessageReceived(this, e);
}

The way you are raising the event isn't thread safe. This is the correct way:

protected void OnLoginMessageReceived(LoginMessageEventArgs e)
{
    var loginMessageReceived = LoginMessageReceived;
    if (loginMessageReceived != null)
        loginMessageReceived(this, e);
}

Just looked at your event args and recommend you read this blog post by Eric Lippert. It has some pros and cons of what you're doing there.

Answer 3

• I think you should use EventHandler<T> or even Action<T> as your event types. There is no point in generating a specific deleagate for every message. You should also extract your event raising logic to some static method (or extension method).
• I does not look like ILogger property should be a part of IDataMessage interface.
• I think you should change NetworkServer api. Instead of assuming, that "someone will wait for the task or something", you should make it clear on how your class should be used. I would create two void methods, like Start and Stop. Oh, and you still have this infinite while(true) loop in your task.
• You need a common interface for all your messages. Right now you have those special messages, which you handle in your NetworkServer class, those other messages which you convert to IDataMessage interface, etc. I think you should convert all messages to common interface and stick to it.
• I don't like your approach with events. But maybe its just me. I like however RubberDuck's suggestion from previous thread. The one where you turn your DataMessageHandler into proper factory and delegate processing logic to message wrapper. I think that is how you are supposed to process different messages in OOP manner.