Resilient & Stable TCP Server Polling

Question

I am looking for feedback to perfect my code developed for WPF in terms of speed, stability and resiliency. My code is supposed to handle synchronous status polling as well as asynchronous Commands to numerous TCP servers (more than 20). I am using a BlockingCollection to funnel these Polls & Commands and using the APM Pattern for Socket Communication. Please review.

ManualResetEvent connect = new ManualResetEvent(false);
ManualResetEvent send = new ManualResetEvent(false);
ManualResetEvent receive = new ManualResetEvent(false);
BlockingCollection<string[]> cmd_Queue = new BlockingCollection<string[]>();

...

Task.Run(() => TCP_Comms());
...

void TCP_Comms()
    {
        while (true)
        {
            while (wifi_state)
            {
                string[] frame = cmd_Queue.Take(); // string[] { IP, Command }
                Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
                try
                {
                    client.BeginConnect(frame[0], port, new AsyncCallback(Connect_Callback), client);
                    if (!connect.WaitOne(timeout_Millis)) //1000ms
                    {
                        Debug("Connection timeout");
                    }
                    else
                    {
                        data = Encoding.ASCII.GetBytes(frame[1]);
                        client.BeginSend(data, 0, data.Length, SocketFlags.None, new AsyncCallback(Send_Callback), client);
                        if (!send.WaitOne(timeout_Millis))
                        {
                            Debug("Send timeout");
                        }
                        else
                        {
                            var response = new byte[client.ReceiveBufferSize];
                            client.BeginReceive(response, 0, response.Length, SocketFlags.None, new AsyncCallback(Receive_Callback), client);
                            if (!receive.WaitOne(timeout_Millis))
                            {
                                Debug("Receive timeout");
                            }
                            else
                            {
                                string response_data = Encoding.ASCII.GetString(response, 0, response.Length);
                                Debug("Response Received: " + response_data);
                            }
                        }                          
                    }                 
                }
                catch (SocketException ex)
                {
                    Debug("Comms Error: " + ex.Message);
                }                  
                connect.Reset();
                send.Reset();
                receive.Reset();                   
                client.Close();
            }
        }
    }

    void Connect_Callback(IAsyncResult ar)
    {
        try
        {               
            client.EndConnect(ar.AsyncState as Socket);             
            connect.Set();
        }
        catch (Exception ex)
        {
            Debug("Connect Error: " + ex.Message);
        }
    }

    void Send_Callback(IAsyncResult ar)
    {
        try
        {
            client.EndSend(ar.AsyncState as Socket);
            Debug("Send Success");
            send.Set();

        }
        catch (Exception ex)
        {
            Debug("Send Error: " + ex.Message);
        }
    }

    void Receive_Callback(IAsyncResult ar)
    {          
        try
        {
            client.EndReceive(ar.AsyncState as Socket);                
            Debug("Receive Success");
            receive.Set();
            
        }
        catch (Exception ex)
        {
            Debug("Recieve Error: " + ex.Message);
        }
    }

Answer 1

The APM Pattern is old school. I would suggest using the TAP extensions that are built in now. Can still use Task.Run as it takes a Func Task as one of the method overloads.

I would also change the timeout to be a TimeSpan they are easier to read and change instead of having to think in milliseconds.

private TimeSpan timeout = TimeSpan.FromSeconds(1);

while true is always a code smell for me. I don't know how this is running but if converting it to run as a service the services have a shutdown event. What I would recommend is changing the method signature of TCP_Comms to be

async Task TCP_Comms(CancellationToken cancellationToken)

and instead of while true have the while check the cancellationtoken

while (!cancellationToken.IsCancellationRequested)

If right now you don't need a way to break out of the loop when calling Task.Run can just pass in a default CancellationToken

Task.Run(() => TCP_Comms(CancellationToken.None));

This will allow later an option to pass in a cancellation token that does get cancelled when the app is shutting down by replacing CancellationToken.None with a token that gets cancelled when the app is shutting down.

There is a lot of code in the loop. I would recommend breaking them out into smaller methods or local functions, depending on the version of c# you are using.

For example on connecting/sending/receiving can break out each code into it's own set of code like the following. These are a local functions so has access the the variables of cancellationtoken and client socket. If want a methods would need to pass them along.

async Task<bool> Connect(string host, int port)
{
    // using Task.Delay to have a timeout on the Connection
    var completedTask = await Task.WhenAny(
        client.ConnectAsync(host, port),
        Task.Delay(timeout, cancellationToken));

    // await here to throw any exception this task might have been completed with
    try
    {
        await completedTask;
    }
    catch (Exception ex)
    {
        Debug("Connect Error: " + ex.Message);
        return false;
    }

    // timeout hit first
    if (!client.Connected)
    {
        Debug("Connection timeout");
        client.Close();
        return false;
    }

    return true;
}

async Task<bool> Send(byte[] command)
{
    // Create a CancellationTokenSource to timeout the sendasync
    using var tokenSource = new CancellationTokenSource(timeout);
    try
    {
        await client.SendAsync(command, SocketFlags.None, tokenSource.Token);
    }
    catch (TaskCanceledException)
    {
        Debug("Send timeout");
        return false;
    }
    catch (Exception ex)
    {
        Debug("Send Error: " + ex.Message);
        return false;
    }

    return true;
}

async Task<string> Receive(int bufferSize)
{
    var buffer = new byte[bufferSize];
    // Create a CancellationTokenSource to timeout the receiveasync
    using var tokenSource = new CancellationTokenSource(timeout);
    try
    {
        await client.ReceiveAsync(buffer, SocketFlags.None, tokenSource.Token);
        var response = Encoding.ASCII.GetString(buffer, 0, buffer.Length);
        Debug("Response Received: " + response);
        return response;
    }
    catch (TaskCanceledException)
    {
        Debug("Receive timeout");
        return null;
    }
    catch (Exception ex)
    {
        Debug("Receive Error: " + ex.Message);
        return null;
    }
}

Now the code for each event that happens in the code is it's own set of code and logging.

Now makes the main inner loop look similar to

while (wifi_state)
{
    string[] frame = cmd_Queue.Take(); // string[] { IP, Command }
    Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    try
    {
        if (await Connect(frame[0], port))
        {
            var data = Encoding.ASCII.GetBytes(frame[1]);
            if (await Send(data))
            {
                var response = await Receive(client.ReceiveBufferSize);
            }
        }
    }
    catch (SocketException ex)
    {
        Debug("Comms Error: " + ex.Message);
    }

    client.Close();

Warning I didn't run/test this code as I don't have all your code or a server setup to send and receive messages This is just used as an example of how using the TAP and restructuring the code into smaller functions/methods will help make it read easier and will be more up-to-date and I personally feel easier to maintain by anyone coming afterwards.