Multithreaded client server socket

I have created this library mostly for a learning experience with sockets and threading.

For this review, focus on socket/threading. Let me know if I properly implemented both. If you want to comment on other part go ahead I'm open to any comment.

I have read this for the threading code.

I'm using lock() and ManualResetEvent to handle the threading part.

I am not using the SocketAsyncEventArgs pattern but I did read this to figure out how to play with socket.

On that page you can read this:

• On the first receive op, receive less bytes than the length of the prefix.

• After having the received part of the prefix on a previous receive op or ops, then receive another part of the prefix, but not all of it.

• After having received part of the prefix on a previous receive op or ops, then receive the rest of the prefix, but nothing more.

• After having received part of the prefix on a previous receive op or ops, we then receive the rest of it, plus part of the message.

• After having received part of the prefix on a previous receive op or ops, we then receive the rest of it, plus all of the message.

• Receive exactly the number of bytes that are in the prefix, but nothing more.

• After having received exactly the number of bytes that are in the prefix on a previous receive op or ops, we then receive part of the message.

• After having received exactly the number of bytes that are in the prefix on a previous receive op or ops, we then receive all of the message.

• Receive the number of bytes for the prefix plus part of the message, but not all of the message.

• After having received the prefix and part of the message on a previous receive op or ops, we then receive another part of the message, but not all of it.

• After having received the prefix and part of the message on a previous receive op or ops, we then receive all the rest of the message.

• Receive the number of bytes for the prefix plus all of the message on the first receive op.

I think I

Observations as I'm reading down the code:

Server

-
Class doesn't implement an interface. If I'm using your library, at least one of my classes is going to have a dependency on the Server class. If I use an IoC container, I need to pick one that can work with concrete types (that's usually not a problem though). If I want to write unit tests for that class of mine however, I'm stuck. I need to write a wrapper around your Server class, and inject that instead of using the Server directly: by not implementing an interface, you're creating work for your client code.

-
Class is sealed. That's usually not a good idea. General design guidelines say:


DO NOT seal classes without having a good reason to do so.

If I wanted to take your library and, I don't know, extend the Server class with a decorator that logs everything coming in and everything going out... well I'm out of luck, I need to think of another way. And a sealed class can't be mocked, so the combination of not implementing an interface and sealing the type makes for either tightly-coupled client code, or client code that needs to work harder than it should to decouple the components.

-
None of the private fields are readonly. Fields that can be initialized statically, or in the constructor, and that aren't assigned a new reference, should be made readonly to better convey the idea that their reference shouldn't be reassigned throughout the lifetime of the class instance.

-
clients would probably be better off as a ConcurrentDictionary.

-
Event delegates aren't standard. Client code will expect event delegates to be compatible with System.EventHandler. The convention is to declare events using an EventHandler delegate for events without arguments, and to use the generic EventHandler for events with arguments - and encapsulate the arguments in a class derived from System.EventArgs. That way if your arguments ever need to change, you're not changing the delegate's signature and you're not breaking any existing client code.

-
Naming convention for event-raising procedures isn't standard either. RaiseConnected would be OnConnected, and RaiseMessageReceived would be OnMessageReceived, for example.

-
Naming convention for event-handling procedures isn't consistent. Take this snippet:

client.Connected += state_Connected;
    client.SocketError += client_SocketError;
    client.MessageReceived += state_MessageReceived;
    client.Disconnected += state_Disconnected;
    client.InitServer(socket);

Typically when the event source is called client, event handlers would be named client_NameOfEvent.

-
KindMessage reads funny. I'd reverse the words to get MessageKind. And it would make KindMessage kindOfSend read like MessageKind messageKind.

-
msg has no reason not to be spelled out: message gets picked up just as well by IntelliSense/auto-complete.

-
There's a possible NullReferenceException right here - GetClient can return null, so you need a local variable and a null-check here:

GetClient(ClientId).SendBytes(clients.Keys.ToArrayOfByte(),KindMessage.ListClientId);`

-
msg would be more descriptive as messageBytes in the Send method.

-
This boilerplate repeats a lot:

var handler = Disconnected;
if (handler != null)
{
    handler(ClientId);
}

You could make a general-purpose event-raising method... if your event delegates were EventHandler delegates, and if your arguments were encapsulated in an EventArgs class:

private void RaiseEventInternal(EventHandler raised, EventArgs args)
{
    var handler = raised;
    if (handler != null)
    {
        handler(this, args);
    }
}

Alternatively, you can declare your events with a no-op delegate:

public event ConnectedHandler Connected = delegate { };

That way the handler will never be null on any thread, and you can safely do away with the thread-local copy and null-check.

I strongly recommend you standardize your events, and if you're going to keep the class sealed, at least make it implement some IServer interface:

```
public interface IServer
{
///
/// An event that is raised when a client successfully connects.
///
EventHandler Connected;
///
/// An event that is raised when a client successfully disconnects.
///
EventHandler Disconnected;
///
/// An event that is raised when a client successfully receives a message.
///
EventHandler MessageReceived;
///
/// An event that is raised when a client successfully sends a message.
///
EventHandler MessageSent;
///
/// An event that is raised when a client throws an exception.
///
EventHandler SocketError;

///
/// Starts the server.
///
void Start(string address, int port);
///
/// Stops the server.
///
void Stop();

///
/// Sends the specified message to all connected clients.
///
voi

StackExchange Code Review Q#100434, answer score: 12