Flexible socket framework

I'm working on an async socket server, would really appreciate any critique and some advices regarding the place where i process received messages.

My interfaces are as follows:

IAsyncClient

public delegate void ConnectedHandler(IAsyncClient a);

public delegate void ClientMessageReceivedHandler(IAsyncClient a, List msg);

public delegate void ClientMessageSubmittedHandler(IAsyncClient a, bool close = false);

public delegate void ClientReceivingStarted();

public delegate void ClientErrorHandler(string errorMessage);

public interface IAsyncClient : IDisposable
{
    bool IsConnected { get; }

    IClientChainsContainer ClientChainsContainer { get; set; }

    event ConnectedHandler Connected;
    event ClientMessageReceivedHandler MessageReceived;
    event ClientMessageSubmittedHandler MessageSubmitted;
    event ClientReceivingStarted ReceivingStarted;
    event ClientErrorHandler Error;

    void InvokeConnected(IAsyncClient a);
    void InvokeMessageReceived(IAsyncClient a, List msg);
    void InvokeMessageSubmitted(IAsyncClient a, bool close = false);
    void InvokeReceivingStarted();
    void InvokeError(string errorMessage);

    Task StartClient();
    void StartReceiving();
    void SetId(Guid clientId);
    Task Send(IProcessable message, bool close = false);
    Task SendSomeCommand();
    Task SendAlarm();
}

IAsyncSocketListener

```
public delegate void MessageReceivedHandler(Guid id, List msg);

public delegate void MessageSubmittedHandler(Guid id, bool close);

public interface IAsyncSocketListener : IDisposable
{
event MessageReceivedHandler MessageReceived;
event MessageSubmittedHandler MessageSubmitted;

IServerChainsContainer ServerChainsContainer { get; set; }

void StartListening();
bool IsConnected(Guid id);
Task Send(Guid id, IProcessable msg, bool close = false);
Task SendToAll(IProcessable msg, bool close = false);
Task SendToAllExcept(List exludedClientIds, IProcessable msg, bool close =

That's more code than I can handle, so I'll just cover some of it.

IAsyncClient exposes too much methods and is way too complex.

IClientChainsContainer ClientChainsContainer { get; set; }

It is my understanding that "client" class should handle the connection. It's job is to keep connection open and send messages to the server. Why does it expose IClientChainsContainer then? Shouldn't some other component, that is not part of the client, be responsible for processing/parsing messages?

event ClientErrorHandler Error;

I think returning Exception instead of a string would be more useful. Also if the outside code needs to be notified about the error, shouldn't you just throw the exception (or let existing exception through)? That's way more common approach.

void InvokeConnected(IAsyncClient a);
void InvokeMessageReceived(IAsyncClient a, List msg);
void InvokeMessageSubmitted(IAsyncClient a, bool close = false);
void InvokeReceivingStarted();
void InvokeError(string errorMessage);

Why are those methods public? That does not feel right. Anyone from anywhere can call any of those methods in any order. That's sounds like a disaster to me.

Task StartClient();
void StartReceiving();

So in which order should I call those to get things running? Should I call StartClient and then StartReceiving ? Should I await StartClient? You should expose a single method, say, Task ConnectAsync(), so I can call it to connect to the server and be done with it. Straightforward and simple. Everyting else should be an implementation detail. If StartReceiving reads a single message, then you should change it's signature to Task ReceiveAsync().

void SetId(Guid clientId);

Mutable ID is almost always a bad idea. I would remove this method form the interface and pass id directly to constructor. If you choose to keep this method, might as well make it into a property.

Task SendSomeCommand();
Task SendAlarm();

"SomeCommand" ? What is "some command"? You should use meaningful names. And you should not use domain-specific methods in otherwise domain-agnostic API. I would refactor those into extension methods.

That being said, here is the API that looks alright to me:

public interface IAsyncClient : IDisposable
{
    bool IsConnected { get; }

    Task ConnectAsync();
    Task DisconnectAsync();

    //close what? connection? something else? needs a better name
    Task SendAsync(byte[] message, bool close = false);
    Task ReceiveAsync();

    event ConnectedHandler Connected;
    event ClientMessageSubmittedHandler MessageSubmitted;
    event ClientReceivingStarted ReceivingStarted;
    event ClientMessageReceivedHandler MessageReceived;
    event ClientErrorHandler Error;
}

Regarding implementation:

private readonly string _pass;

Hopefully this one is encrypted. Passwords lying around in plain text is a big security risk.

private static readonly object SyncRoot = new object();

Why is it static? Do you really want multiple instances of your client to fight over the same lock? Looks weird.

Your implementation is not thread-safe. That's risky. Synchronizing a single class even with all async's and await's flying around should be way easier, than synchronizing all the other places that are going to use IAsyncClient. However it is definitely possible to design an application in such a way, that thread safety is not going to be an issue.

P.S. You might want to check out Microsoft's TPL.Dataflow library. It does what your IChainOfResponsibility does, but probably better.

StackExchange Code Review Q#162978, answer score: 2