Dynamic network messaging

Question

I'm building a client and server for a game but wanted a generic messaging system in a shared library that let me focus on application logic and was largely separate form the underlying networking I/O details.

I'm mostly looking for suggestions or possible improvements. Performance on the server is a concern with all the casting. I'm stuck on .Net 3.5 for the client (Unity3D) but if there's decent performance improvements to be found for the server I'd consider splitting the library. I'm also using Protobuf-net for serializing messages being sent across the network. My network protocol uses two 4 byte prefixes, one for the message byte length and the 2nd is an int32 that maps to a dictionary of types so I can deserialize messages to their exact type upon arrival. The typemap is built at startup reflecting through all the message subtypes in the same library.

I'll not post the client/server code because I see those as separate concerns (Network I/O, handshaking, serialization etc can be done a bunch of different ways). Lastly, I've cut out most the threading code to make it easier to read.

My approach breaks down messaging into three simple Message, Request and Response types with the following rules:

1. All network messages will derive from the Message class.
2. Any message that derives from Request will expect a Response in reply.
3. Request and Response are both sub-classes of Message.

Message.cs

public partial class Message
{
    public virtual bool IsResponse { get { return false; } }
    public int id { get; }
    public string description { get; }

    static int idCounter = 0;

    public Message()
    {
        id = idCounter++;
    }

    public Message(string description)
    {
        id = idCounter++;
        this.description = description;
    }
}

Response.cs

public class Response : Message
{
    public MessageStatus status { get; }
    public bool Success { get { return status == MessageStatus.Ok; } }
    public override bool IsResponse { get { return true; } }

    public Response(int id, string description)
    {
        this.id = id;
        this.status = MessageStatus.Ok;
        this.description = description;
    }

    public Response(int id, MessageStatus status, string description)
    {
        this.id = id;
        this.status = status;
        this.description = description;
    }
}

Request is not shown, but it's basically Message with an additional type field so the application knows what kind of Response type is expected. Game/Application logic can register to listen for the arrival of specific message subclasses. All the plumbing is taken care of via two singeltons; a MessageRouter and a ResponseRouter (couldn't think of better names). As the server or client receives data and the protocol handler deserializes it into a Message instance it then passes it off to the MessageRouter via the Dispatch method:

MessageRouter.cs

class MessageRouter
{
    private static MessageRouter instance;
    protected Dictionary<Type, object> handlers;

    public static MessageRouter Instance
    {
        get
        {
            if (instance == null) { instance = new MessageRouter(); }
            return instance;
        }
    }

    protected MessageRouter()
    {
        handlers = new Dictionary<Type, object>();
    }

    public bool AddHandler<T>(Action<T> handler) where T : Message
    {
        if (handler == null) { return false; }
        var messageType = typeof(T);
        object outHandler;
        if (handlers.TryGetValue(messageType, out outHandler))
        {
            var typedHandler = outHandler as Action<T>;
            handlers[messageType] = typedHandler + handler;
            return true;
        }
        handlers.Add(messageType, handler);
        return true;
    }

    public bool RemoveHandler<T>(Action<T> handler) where T : Message
    {
        if (handler == null) { return false; }
        var messageType = typeof(T);
        return handlers.Remove(messageType);
    }

    public bool Dispatch<T>(T message) where T : Message
    {

        if (message == null) { return false; }

        if (message.IsResponse)
        {
            if (ResponseRouter.Instance.Dispatch(message)) { return true; }
        }

        var messageType = message.GetType();
        object handler;
        if (handlers.TryGetValue(messageType, out handler))
        {
            if (handler == null) { return false; }
            (handler as Action<T>)(message);
        }
        return false;
    }
}

As you may have noticed, if the message has an IsResponse property of true it is passed off to the ResponseRouter instead as I assume no code should be registering to listen for response types via the MessageRouter (I should probably check for that and log a warning). The ResponseRouter uses a similar pattern but they're more one-shot events, as soon as they're triggered the handler is removed from the response router. Response handlers are passed into the client/server's send method:

    public delegate void ResponseCallback<T>(T response) where T : Message;

    void Send<TMessage>(TMessage message)
        where TMessage : Message;
    { ... }

    void Send<TRequest, TResponse>(TRequest request, ResponseCallback<TResponse> responseCallback,)
        where TRequest : Request
        where TResponse : Response;
    {
        ...
        ResponseRouter.Instance.RegisterCallback(request, responseCallback);
        ...
    }

ResponseRouter.cs

public class ResponseRouter
{
    private static ResponseRouter instance;
    private Dictionary<int, object> responseMap;

    public static ResponseRouter Instance
    {
        get
        {
            if (instance == null) { instance = new ResponseRouter(); }
            return instance;
        }
    }

    protected ResponseRouter()
    {
        responseMap = new Dictionary<int, object>();
    }

    public bool RegisterCallback<TRequest,TResponse>(TRequest request, ResponseCallback<TResponse> callback) 
        where TRequest : Request 
        where TResponse : Response
    {

        if (responseMap.ContainsKey(request.id)) { return false; }
        responseMap.Add(request.id, callback);
        return true;
    }

    public bool Dispatch<T>(T response) where T : Message
    {
        if (response == null) { return false; }
        object callback = null;
        if (responseMap.TryGetValue(response.id, out callback))
        {
            responseMap.Remove(response.id);
        }
        if (callback == null) { return false; }
        (callback as ResponseCallback<T>)(response);
        return true;
    }
}

To support a new message type I simply have to subclass Message or Response and write the application code to hook into handling them. The only messaging special case is authentication. Establishing or restoring client sessions (disconnect recover) is all logic that can exist far away from my core server/client code.

Answer 1

Constructor chaining

Calling one constructor from another constructor is called constructor chaining and helps to dry (don't repeat yourself) your code.

For instance the Message class would benefit like so

public partial class Message
{
    public virtual bool IsResponse { get { return false; } }
    public int id { get; }
    public string description { get; }

    static int idCounter = 0;

    public Message()
    {
        id = idCounter++;
    }

    public Message(string description)
        :this()
    {
        this.description = description;
    }
}  

and the Response class like so

public class Response : Message
{
    public MessageStatus status { get; }
    public bool Success { get { return status == MessageStatus.Ok; } }
    public override bool IsResponse { get { return true; } }

    public Response(int id, string description)
        :this(id, MessageStatus.Ok, description)
    {
    }

    public Response(int id, MessageStatus status, string description)
    {
        this.id = id;
        this.status = status;
        this.description = description;
    }
}  

---

MessageRouter

By using the null-coalescing operator ?? you can clean the Instance property like so

public static MessageRouter Instance
{
    get
    {
        return instance ?? (instance = new MessageRouter());
    }
}  

---

Using a stateful Singleton prevents the class and its caller's to be tested. Assume you have one test to check that a handler is added successfully and a test to remove a handler. The first one is easy, but for the second one you will to first add a handler and then remove it so you are basically testing 2 different/independent parts of your code but tests should only test 1 part of your code.

So you could say, fine I will just remove the handler which you have added with the first test. This is also bad, because one test should not depend on another test. But you say, hey I can live with that problem here. Then I would need to answer, if you change the order of the calling tests the tests won't assert the same way. If at first the remove handler test is executed it will return false.

So a much better way would be to use an Interface which is the injected to the constructor of the caller's and stored in a private field.

---

Taking a look at this

public bool AddHandler<T>(Action<T> handler) where T : Message
{
    if (handler == null) { return false; }
    var messageType = typeof(T);
    object outHandler;
    if (handlers.TryGetValue(messageType, out outHandler))
    {
        var typedHandler = outHandler as Action<T>;
        handlers[messageType] = typedHandler + handler;
        return true;
    }
    handlers.Add(messageType, handler);
    return true;
}

I see a few points to mention. First if a passed in argument is null I would expect the method to throw an ArgumentNullException instead of just returning false. Second you really should add some vertical space to group related code together / distinct unrelated code like so

public bool AddHandler<T>(Action<T> handler) where T : Message
{
    if (handler == null) { throw new ArgumentNullException("handler"); }
    // if you use C# 6.0 you can use the nameof operator like so
    // if (handler == null) { throw new ArgumentNullException(nameof(handler)); }
    var messageType = typeof(T);

    object outHandler;
    if (handlers.TryGetValue(messageType, out outHandler))
    {
        var typedHandler = outHandler as Action<T>;
        handlers[messageType] = typedHandler + handler;

        return true;
    }

    handlers.Add(messageType, handler);

    return true;
}

This does not cost you any more memory or decreases execution speed, but make it easier to grasp the logic at first glance which makes it a lot easier to maintain.

---

public bool RemoveHandler<T>(Action<T> handler) where T : Message
{
    if (handler == null) { return false; }
    var messageType = typeof(T);
    return handlers.Remove(messageType);
}

here the messageType variable is only used to remove the entry of the dictionary which just can be omitted like so

public bool RemoveHandler<T>(Action<T> handler) where T : Message
{
    if (handler == null) { return false; }

    return handlers.Remove(typeof(T));
}

Some of the mentioned points aply to the ResponseRouter too.

Answer 2

Naming Conventions:

public int id { get; }

Public properties are typically named with CamelCase. Particularly, you should keep a single convention:

public virtual bool IsResponse { get { return false; } }

---

Nested ifs:

if (message.IsResponse)
{
    if (ResponseRouter.Instance.Dispatch(message)) { return true; }
}

You can combine those particular ifs into one statement because you don't have an else.

---

Otherwise, I don't have any comments.