Okay, looking for some constructive feedback/criticism here... 

What follows is the code of my c# UDP Socket Class.  I've written this based on a number of online examples and a few distant memories from the past, so I really have no idea how "bad" the code is/could be in a 100+ concurrent connection scenario(my dev environment is just two machines at the moment)...  It works, I know that, it is fairly stable(I can't break it when using it normally), and it behaves how I believe I want it to. 

It is a dual purpose class for handling both Servers and Clients.  It uses an asynchronous receive and synchronous send(I may switch to async send if it proves beneficial later on).  My servers use Client sockets to communicate with each other and to perform internal connection tests in a multiple server, "single shard" type environment.  I'll be devising some heavy load testing methods a little further down the line, but I'm hoping to fish out most of the major gotchas before I get there.

So, please, let me know how to fix it up if it smells terribly to you, or if not, that's even better...

Sorry for the large code dump, but THANKS for checking it out!

using System;
using System.Collections.Generic;
using System.Net;
using System.Net.Sockets;
using System.Text;

using UWAvatarServerData;

namespace UWAvatarServer
{
    public class UDPSocket : IDisposable
    {
        //Some simple string length counters and averages to get a rough idea of generated traffic.
        public int sndMax = 0;
        public MovingAverage sndAvg = new MovingAverage();
        public int csndMax = 0;
        public MovingAverage csndAvg = new MovingAverage();
        public int rcvMax = 0;
        public MovingAverage rcvAvg = new MovingAverage();

        //Constant for configuring the prevention of ICMP connection resets
        private const int SIO_UDP_CONNRESET = -1744830452;

        //UDP socket
        private Socket _socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);

        //Buffer Size Constant
        private const int bufSize = 8 * 1024;

        //Raw string data from client packets
        private Dictionary<EndPoint, Queue<string>> messageDictionary;

        //Queue for holding raw string data from server packets when in client mode.
        private Queue<string> cQ;

        //Referece to the data store used by the server(for access to the current game time clock)
        private UWDataStore dataStore;

        //Time code storage for last sent/received messages
        private double lastSentMessage = 0;
        private double lastReceivedMessage = 0;

        //Boolean to determine which mode we're in so received messages get put in the right place.
        private bool clientMode = false;

        //Max string lenght allowed by the servers.
        private int maxMessageLength = 1450;

        //IDisposable stuff
        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (disposing)
            {
                // free managed resources
                if (_socket != null)
                {
                    _socket.Dispose();
                    _socket = null;
                }
            }
        }

        //State class for async receive.
        public class State
        {
            public byte[] buffer = new byte[bufSize];
            public EndPoint epFrom = new IPEndPoint(IPAddress.Any, 0);
        }

        //Server "Mode" 
        public UDPSocket(Dictionary<EndPoint, Queue<string>> msgsDict, UWDataStore DATASTORE)
        {
            clientMode = false;
            messageDictionary = msgsDict;
            dataStore = DATASTORE;
            lastSentMessage = dataStore.UWServerSeconds;
        }

        //Client "Mode"
        public UDPSocket(Queue<string> mq, UWDataStore DATASTORE)
        {
            clientMode = true;
            cQ = mq;
            dataStore = DATASTORE;
            lastSentMessage = dataStore.UWServerSeconds;
        }

        public void CloseSocket()
        {
            _socket.Close();
        }

        //Internal connection status checking
        public int SendHowStale()
        {
            if (lastSentMessage == 0)
                lastSentMessage = dataStore.UWServerSeconds;
            return (int)(dataStore.UWServerSeconds - lastSentMessage);
        }

        //Internal connection status checking
        public int ReceiveHowStale()
        {
            if (lastReceivedMessage == 0)
                lastReceivedMessage = dataStore.UWServerSeconds;
            return (int)(dataStore.UWServerSeconds - lastReceivedMessage);
        }

        //Start/Bind a Server.
        public void Server(string address, int port)
        {
            //In case restarting uncleanly, dunno if this actually does anything..  
            _socket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.ReuseAddress, true);
            //Ensure all async packets contain endpoint info and etc.
            _socket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.PacketInformation, true);
            //Ignore ICMP port unreachable exceptions.
            _socket.IOControl((IOControlCode)SIO_UDP_CONNRESET, new byte[] { 0, 0, 0, 0 }, null);
            //Bind to port.
            if (address == "all")
            {
                _socket.Bind(new IPEndPoint(IPAddress.Any, port));
            }
            else
            {
                _socket.Bind(new IPEndPoint(IPAddress.Parse(address), port));
            }
            //Start receive callback process.
            Receive();
        }

        //Setup a Client to Server socket.
        public void Client(string address, int port)
        {
            //Dunno if these two options do anything for client sockets, but they don't seem to break anything.
            _socket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.PacketInformation, true);
            _socket.IOControl((IOControlCode)SIO_UDP_CONNRESET, new byte[] { 0, 0, 0, 0 }, null);
            _socket.Connect(IPAddress.Parse(address), port);
            //Start receive callback.
            Receive();
        }

        //ServerSend sends to any EndPoint from THIS server.
        public void ServerSend(string text, EndPoint ep)
        {
            try
            {
                byte[] data = Encoding.ASCII.GetBytes(text);

                _socket.SendTo(data, ep);
                lastSentMessage = dataStore.UWServerSeconds;
                if (text.Length > sndMax) sndMax = text.Length;
                sndAvg.ComputeAverage((float)text.Length);
                //Console.WriteLine("TO NET: " + text);
            }
            catch (Exception ex)
            {
                Console.WriteLine("ServerSend Exception: " + ex.Message);
            }
        }

        //Client Send only sends to the connected Server.
        public void cSend(string text)
        {
            try
            {
                byte[] data = Encoding.ASCII.GetBytes(text);
                _socket.Send(data);
                lastSentMessage = dataStore.UWServerSeconds;
                if (text.Length > sndMax) sndMax = text.Length;
                sndAvg.ComputeAverage((float)text.Length);
                //Console.WriteLine("TO NET: " + text);
            }
            catch (Exception ex)
            {
                Console.WriteLine("cSend Exception: " + ex.Message);
            }
        }

        //Setup Async Callback
        private void Receive()
        {
            try
            {
                State so = new State();
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, new AsyncCallback(_Receive), so);
            }
            catch (Exception)
            {
            }
        }

        //Receive Callback
        private void _Receive(IAsyncResult ar)
        {
            try
            {
                State so = (State)ar.AsyncState;

                int bytes = _socket.EndReceiveFrom(ar, ref so.epFrom);
                lastReceivedMessage = dataStore.UWServerSeconds;
                string smessage = Encoding.ASCII.GetString(so.buffer, 0, bytes);
                //Console.WriteLine("FROM NET: " + text);
                if (smessage.Length < maxMessageLength)
                {
                    if (smessage.Length > rcvMax) rcvMax = smessage.Length;
                    rcvAvg.ComputeAverage((float)smessage.Length);
                    if (clientMode)
                    {
                        cQ.Enqueue(smessage);
                    }
                    else
                    {
                        if (!messageDictionary.ContainsKey(so.epFrom))
                        {
                            messageDictionary.Add(so.epFrom, new Queue<string> { });
                        }
                        messageDictionary[so.epFrom].Enqueue(smessage);
                    }
                }
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, new AsyncCallback(_Receive), so);
            }
            catch (Exception)
            {
            }
        }
    }
}

Generally speaking I use it as such:

public static bool running = true;
static void UDPServer()
{
	using (s = new UDPSocket(MessageDictionary, UWDataStore))
	{
		s.Server("all", 37373);
		while(running)
		{
			//Servery Stuff Goes Here.
          		//Like reiteratively dequeuing the Message Dictionary Queues and processing/replying to all commands/etc...
		}
	}
}

All processing of individual messages from clients is handled with Task.Factory tasks, and a reference to the server's socket varible (s), and the client's EndPoint is sent along the ride for use in replying to clients.  There's no game logic and there are no client replies that happen directly from within the UDP server's main while loop, mostly just connection status checking and reorganizing of the Message Queues into Tasks.

Thanks again for making it this far.

Some random comments. I, too, haven't done C# networking in forever :-)

You seem to be generating garbage in each call to Receive(), so you should use an object pool for your States.

In general, a well-designed C# program generates no garbage during steady state.

I'm also not sure the Dispose() machinery is all that useful? Maybe it is. I never find finalizers to be worth the trouble, compared to just knowing when I'm done with something.

You don't seem to be calculating the idle timeout for each client -- once a client stops sending messages, you want some kind of timeout by Endpoint to be able to idle them out.

I personally don't like treating messages as string; I'd much rather use a small Buffer class that contains a byte[] array of the largest possible message, and a size (and perhaps an offset for how much has been consumed already.) Again, those buffers should be object pooled, too.

5 minutes ago, hplus0603 said:

You seem to be generating garbage in each call to Receive(), so you should use an object pool for your States.

Something like this?

https://www.infoworld.com/article/3221392/c-sharp/how-to-use-the-object-pool-design-pattern-in-c.html

And/Or this?

https://stackoverflow.com/questions/2510975/c-sharp-object-pooling-pattern-implementation

Just some quick googles to see if I know what you mean..

5 minutes ago, hplus0603 said:

I'm also not sure the Dispose() machinery is all that useful? Maybe it is. I never find finalizers to be worth the trouble, compared to just knowing when I'm done with something.

Only because I'm using the sockets within "using" blocks, which according to Microsoft, " Provides a convenient syntax that ensures the correct use of IDisposable objects."  It actually just kinda happened as I was attempting different structures to debug earlier versions and whatnot.  I find it makes things pretty simple.  Dunno if it has a cost/benefit worth considering either.

5 minutes ago, hplus0603 said:

You don't seem to be calculating the idle timeout for each client -- once a client stops sending messages, you want some kind of timeout by Endpoint to be able to idle them out.

This is handled within the server logic, all timeouts are one.  This class is meant to ONLY handle the connection setup, sending and receiving.  In as dumb a manner as it possibly can.  I wouldn't try to get away with this if I was using TCP/designing for anything near 100% delivery.

5 minutes ago, hplus0603 said:

I personally don't like treating messages as string; I'd much rather use a small Buffer class that contains a byte[] array of the largest possible message, and a size (and perhaps an offset for how much has been consumed already.) Again, those buffers should be object pooled, too.

So, I should covert my strings to byte[]'s before handing them off to the UDPSocket class?  Or, use something other than strings to begin with?  I would love to use something lighter and faster, but I've yet to find something as flexible and half as easy to code/debug/encrypt/debug when encrypted../etc.. hahaha. 

Open to any specific suggestions.

Thanks for all this!!  I really appreciate the feedback!

I went ahead and followed up on the object pooling suggestion:

//Stolen from example code..
// Generic Object Pool Class
        public class ObjectPool<T>
        {
            // ConcurrentBag used to store and retrieve objects from Pool.
            private ConcurrentBag<T> _objects;
            private Func<T> _objectGenerator;

            // Object pool contructor used to get a delegate for implementing instance initialization
            // or retrieval process
            public ObjectPool(Func<T> objectGenerator)
            {
                if (objectGenerator == null) throw new ArgumentNullException("objectGenerator");
                _objects = new ConcurrentBag<T>();
                _objectGenerator = objectGenerator;
            }

            // GetObject retrieves the object from the object pool (if already exists) or else
            // creates an instance of object and returns (if not exists)
            public T GetObject()
            {
                T item;
                if (_objects.TryTake(out item)) return item;
                return _objectGenerator();
            }

            // PutObject stores back the object back to pool.
            public void PutObject(T item)
            {
                _objects.Add(item);
            }
        }

          
        //New State Object Pool  
        ObjectPool<State> statePool = new ObjectPool<State>(() => new State());

        //State class for async receive.
        class State
        {
            public byte[] buffer = new byte[bufSize];
            public EndPoint epFrom = new IPEndPoint(IPAddress.Any, 0);
        }

And the new Receive().

//Setup Async Callback
        private void Receive()
        {
            try
            {
                State so = statePool.GetObject();
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, new AsyncCallback(_Receive), so);
            }
            catch (Exception)
            {
            }
        }

I miss anything there?

Yes, I did..  Where should I put the PutObject() call?

Well, for now I'm testing it with the PutObject() in the Catch block(s).. Since that is the only real indication I have of a client disconnect..

//Setup Async Callback
        private void Receive()
        {
            State so = statePool.GetObject();
            try
            {
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, new AsyncCallback(_Receive), so);
            }
            catch (Exception)
            {
                statePool.PutObject(so);
            }
        }

        //Receive Callback
        private void _Receive(IAsyncResult ar)
        {
            State so = (State)ar.AsyncState;
            try
            {
                int bytes = _socket.EndReceiveFrom(ar, ref so.epFrom);
                lastReceivedMessage = dataStore.UWServerSeconds;
                string smessage = Encoding.ASCII.GetString(so.buffer, 0, bytes);

                //Console.WriteLine("FROM NET: " + text);
                if (smessage.Length < maxMessageLength)
                {
                    if (smessage.Length > rcvMax) rcvMax = smessage.Length;
                    rcvAvg.ComputeAverage((float)smessage.Length);
                    if (clientMode)
                    {
                        cQ.Enqueue(smessage);
                    }
                    else
                    {
                        if (!messageDictionary.ContainsKey(so.epFrom))
                        {
                            messageDictionary.Add(so.epFrom, new Queue<string> { });
                        }
                        messageDictionary[so.epFrom].Enqueue(smessage);
                    }
                }
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, new AsyncCallback(_Receive), so);
            }
            catch (Exception)
            {
                statePool.PutObject(so);
            }
        }

That's the only place that seems to make any sense to me.. right now..

A few minutes later...

Looking good so far, memory use was up almost 10+mb above normal before I put the PutObject() calls in the catch blocks.  Now it's totally stable.  So unless somebody has a better place to put them that's where they seem happy.  It makes sense to me, the only way out of the _Receive() recycle is through an exception..

You can quickly optimize the "new AsyncCallback(_Receive)" allocation by storing the AsyncCallback instance once in a member variable.  These don't generate much garbage but they're trivial to deal with.

You can use this optimization for any delegate as long as its captured variables (if it has any) don't change each time you use it.

1 minute ago, Nypyren said:

You can quickly optimize the "new AsyncCallback(_Receive)" allocation by storing the AsyncCallback instance once in a member variable.  These don't generate much garbage but they're trivial to deal with.

I believe that's how I had it configured originally, and I went away from that because I thought it was contributing to the problem of ICMP exceptions causing the entire receive process for ALL clients to halt, not just the recently vacated one..

I fixed that by turning off the exceptions in IOControl, so I guess maybe I'll put member variable back and see how it goes... 

Though, I'm still curious if using a member variable for this actually effects how the async receive operates in this scenario.  Does it force it to use/reuse a single thread?  I'm still trying to wrap my head around why port unreachable icmp packets for specific clients were killing the entire receive process.

Just have to do more testing I guess.. 

Thanks!

10 hours ago, Septopus said:

Though, I'm still curious if using a member variable for this actually effects how the async receive operates in this scenario.  Does it force it to use/reuse a single thread?

Delegate instances can be thought of as immutable lists of (object, method) pairs.  It doesn't matter where the delegate instance lives; its behavior is only determined by what it contains and how it's invoked.

The BeginReceive method controls which thread(s) the operation uses and what happens when it wants to invoke the callback.  In .Net, there's a class called "SynchronizationContext" which is most frequently used to determine which thread asynchronous callbacks are run on.  https://docs.microsoft.com/en-us/dotnet/api/system.threading.synchronizationcontext  It's still up to the asynchronous code to decide whether to use the context or not.

I don't remember off the top of my head if BeginReceive uses the synchronization context or not.

The main thing that the SynchronizationContext allows is calling a function on a different thread.  It does this by acting as a message queue.  One thread adds delegates to a queue, and the other thread periodically reads from the queue and invokes the delegates.

However, if the queue-reading thread is busy (or waiting for something) and isn't allowed to check on the queue, it can lead to deadlocks or lower throughput than what you want.

Aha, killer info, THANKS @Nypyren!!

And I can confirm it has NO effect whatsoever on thread usage, I'm outputting the managed thread ID for the beginreceivefrom now and it's cycling through several IDs using the member variable, same as it was without it.

Here's the changes I made:

//Async Callback Member Variable
        private AsyncCallback recv = null;


//Setup Async Callback
        private void Receive()
        {
            State so = statePool.GetObject();
            try
            {
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, recv = (_Receive), so);
            }
            catch (Exception)
            {
                statePool.PutObject(so);
            }
        }

//Receive Callback
        private void _Receive(IAsyncResult ar)
        {
            State so = (State)ar.AsyncState;
            try
            {
                int bytes = _socket.EndReceiveFrom(ar, ref so.epFrom);
                lastReceivedMessage = dataStore.UWServerSeconds;
                string smessage = Encoding.ASCII.GetString(so.buffer, 0, bytes);
                //Console.WriteLine("Msg frm ThreadID: " + Thread.CurrentThread.ManagedThreadId.ToString());
                //Console.WriteLine("FROM NET: " + text);
                if (smessage.Length < maxMessageLength)
                {
                    if (smessage.Length > rcvMax) rcvMax = smessage.Length;
                    rcvAvg.ComputeAverage((float)smessage.Length);
                    if (clientMode)
                    {
                        cQ.Enqueue(smessage);
                    }
                    else
                    {
                        if (!messageDictionary.ContainsKey(so.epFrom))
                        {
                            messageDictionary.Add(so.epFrom, new Queue<string> { });
                        }
                        messageDictionary[so.epFrom].Enqueue(smessage);
                    }
                }
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, recv, so);
            }
            catch (Exception)
            {
                statePool.PutObject(so);
            }
        }

But then I realized I had more garbage creators in there.. so I folded the rest of the local variables into the state object..

//State class for async receive.
        class State
        {
            public string smessage;
            public int bytes = 0;
            public byte[] buffer = new byte[bufSize];
            public EndPoint epFrom = new IPEndPoint(IPAddress.Any, 0);
        }

//Receive Callback
        private void _Receive(IAsyncResult ar)
        {
            State so = (State)ar.AsyncState;
            try
            {
                so.bytes = _socket.EndReceiveFrom(ar, ref so.epFrom);
                lastReceivedMessage = dataStore.UWServerSeconds;
                so.smessage = Encoding.ASCII.GetString(so.buffer, 0, so.bytes);
                //Console.WriteLine("Msg frm ThreadID: " + Thread.CurrentThread.ManagedThreadId.ToString());
                //Console.WriteLine("FROM NET: " + text);
                if (so.smessage.Length < maxMessageLength)
                {
                    if (so.smessage.Length > rcvMax) rcvMax = so.smessage.Length;
                    rcvAvg.ComputeAverage((float)so.smessage.Length);
                    if (clientMode)
                    {
                        cQ.Enqueue(so.smessage);
                    }
                    else
                    {
                        if (!messageDictionary.ContainsKey(so.epFrom))
                        {
                            messageDictionary.Add(so.epFrom, new Queue<string> { });
                        }
                        messageDictionary[so.epFrom].Enqueue(so.smessage);
                    }
                }
                _socket.BeginReceiveFrom(so.buffer, 0, bufSize, SocketFlags.None, ref so.epFrom, recv, so);
            }
            catch (Exception)
            {
                statePool.PutObject(so);
            }
        }

Now it's starting to make sense.

Thanks Again!!

Another MB of ram saved, and who knows how much garbage collection!