UDP Server class

For my current project I need a basic UDP server. The first priority is to make it as fast, but as resource friendly as possible (it will be run on Raspberry Pi-like devices).

I would like a review about the general code 'quality' and logic. I know that I am missing some error-handling at the moment.

The Server class:

```
using System;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading;

namespace Nuernberger.FlyingDMX
{
public class Server
{
// UDPClient.Receive needs a reference as parameter
private IPEndPoint _endPoint;
public IPEndPoint Endpoint
{
get { return _endPoint; }
private set { _endPoint = value; }
}

// The server loop exists after setting this to false (+25ms max timeout time)
public bool Listening { get; private set; }

private UdpClient listener;
private Thread serverThread;
private ManualResetEvent threadBlocker = new ManualResetEvent(false);

// How long should the server try to receive a packet?
private const int SERVER_READ_TIMEOUT = 25;
// How often should the server look for new packets? (In ms)
private const int SERVER_LOOP_LIMIT = 5;

public event EventHandler OnCommandIncoming;
public event EventHandler OnServerStart;
public event EventHandler OnServerStop;

///
/// Initalizies a new instance of the FlyingDMX.Server-Class and bind's it to the given port
///
/// The port the server should be bound to
public Server(short port = 3636)
{
this.Endpoint = new IPEndPoint(IPHelper.GetBroadcastIP(), port);
this.listener = new UdpClient(port);
this.listener.EnableBroadcast = true;

this.listener.Client.ReceiveTimeout = SERVER_READ_TIMEOUT;
}

///
/// Starts listening on the given port for UDP packet's
///

-
I personally don't like sprinkling this all around the place. In generally I've found to use something like _ as prefix for private class members to be quite effective as a visual reading aid (like you've done for _endPoint). At least you should be consistent.

-
It is not obvious what unit SERVER_READ_TIMEOUT or SERVER_LOOP_LIMIT are. You should either use TimeSpan to define these or at least include the unit as a suffix (like _MS).

-
Generally OnXYZ is used for the methods raising the event rather than the event name. The event name would be without the On. In your case CommandReceived, ServerStarted, ServerStopped.

-
Raising the event: You have a race condition here. If someone unsubscribes from the event after you have checked for null but before you call the event handler you will run into a NullReferenceException. Either you use the new C# 6.0 feature in which case you would call the event handler like this:

OnCommandReceived?.Invoke(this,  new IncomingCommandEventArgs(Command.TryParse(receivedString)))

or you use the good old method of copying the reference (this is why there were usually OnXYZ methods to raise the event since it's a reasonable amount of logic which you don't want to sprinkle around everywhere):

var handler = OnCommandReceived;
if (handler != null)
{
    handler(this, new IncomingCommandEventArgs(Command.TryParse(receivedString)));
}

-
The Command.TryParse violates the general expectation of Try methods in the .NET framework. A TryParse method should return bool to indicate whether parsing has succeeded or not and return the actual result as an out parameter. Your implementation will simply throw (which is what the Try methods were meant to avoid) if the Enum.Parse fails.

-
Magic numbers like 3636 and 10060 should be avoided - introduce named constants for those like const int DEFAULT_PORT and const int ERROR_SOCKET_TIMEOUT.

-
Instead of just blindly waiting in the Stop you should create another ManualResetEvent you can wait on - it should get set when the main loop has quit.

-
Instead of dealing with threads directly you really want to have a look at the Task Parallel Library.

-
Instead of polling you should make use of the asynchronous socket API BeginReceive and EndReceive (examples can be found on MSDN).
Or in these days you probably want to be more looking at ReceiveAsync to make use of the async feature in newer .NET versions.

Those are far more effective since they won't waste CPU cycles trying to read data which isn't there. Especially when you are targeting low power devices similar to Raspberry Pis you really want to be as efficient as possible.

-
Since you have no guarantee that you will read the entire message in one go you should consider supporting partial reads.

-
UDP gives you no guarantees about packet delivery which means packets may be dropped and/or delivered out of order. Your application protocol will have to deal with this.

StackExchange Code Review Q#115692, answer score: 7