9
\$\begingroup\$

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<IncomingCommandEventArgs> OnCommandIncoming;
        public event EventHandler<ServerStartStopEventArgs> OnServerStart;
        public event EventHandler<ServerStartStopEventArgs> OnServerStop;

        /// <summary>
        /// Initalizies a new instance of the FlyingDMX.Server-Class and bind's it to the given port
        /// </summary>
        /// <param name="port"> The port the server should be bound to</param>
        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;
        }

        /// <summary>
        /// Starts listening on the given port for UDP packet's
        /// </summary>
        /// <param name="blockThread"> Blocks the method until the listening loop exists</param>
        public void Start(bool blockThread = false)
        {
            this.serverThread = new Thread(() =>
            {
                this.Listening = true;

                if (this.OnServerStart != null)
                    this.OnServerStart(this, new ServerStartStopEventArgs(this.Endpoint));

                while (this.Listening)
                {
                    // listener.Available counts packet's we have to process. Wait if none are ready.
                    if(this.listener.Available > 0)
                    {
                        try
                        {
                            Byte[] data = this.listener.Receive(ref this._endPoint);
                            var receivedString = Encoding.ASCII.GetString(data);

                            if (this.OnCommandIncoming != null)
                                this.OnCommandIncoming(this, new IncomingCommandEventArgs(Command.TryParse(receivedString)));
                        }
                        catch(SocketException ex)
                        {
                            if (ex.ErrorCode != 10060)
                            {
                                // Handle the error. 10060 is a timeout error, which is expected.
                            }
                        }

                        Thread.Sleep(SERVER_LOOP_LIMIT);
                    }
                }
            });

            this.serverThread.Start();

            if (blockThread)
                threadBlocker.WaitOne();
        }

        /// <summary>
        /// Stops the server and unblocks the thread if needed.
        /// </summary>
        public void Stop()
        {
            this.Listening = false;
            this.listener.Close();

            // Let the server loop exit gracefully
            Thread.Sleep(SERVER_READ_TIMEOUT + SERVER_LOOP_LIMIT + 10);
            this.serverThread = null;

            // Release the thread block on the Start method
            this.threadBlocker.Set();

            if (this.OnServerStop != null)
                this.OnServerStop(this, new ServerStartStopEventArgs(this.Endpoint));
        }
    }
}

In case it's relevant, the TryParse method of the Command class:

public static Command TryParse(string text)
{
    return new Command((CommandType)Enum.Parse(typeof(CommandType), text.Split(':')[0]), text.Split(':')[1].Split(';'));
}

The form of the incoming commands is:

CommandType:Arg1;Arg2;Arg3;...

\$\endgroup\$
7
\$\begingroup\$
  1. 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.

  2. 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).

  3. 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.

  4. 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)));
    }
    
  5. 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.

  6. 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.

  7. 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.

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

  9. 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.

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

  11. 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.