2
\$\begingroup\$

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);
        }
    }
\$\endgroup\$
2
  • \$\begingroup\$ Is there any particular reason you mix the old APM pattern with the newer TAP (Task-based asynchronous) pattern? \$\endgroup\$
    – t3chb0t
    Jan 2 at 19:55
  • \$\begingroup\$ @t3chb0t To keep all the communication logic on a background thread and not block the UI. I was planning to use a Thread pool but this is a simpler implementation. Let me know if I am wrong \$\endgroup\$
    – Bilal Kazi
    Jan 3 at 6:37
2
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ Wow, thanks for your feedback and your implementation of an easily readable and understandable TAP pattern (which I couldn't find really on the internet). I will surely test the code out. I used while true to keep it simple since the process will be running for the lifetime of the application after launch. Also, is there any performance or stability advantage over APM pattern? \$\endgroup\$
    – Bilal Kazi
    Jan 4 at 18:46
  • \$\begingroup\$ The TAP implementation is using the APM pattern. So there is no stability advantage or performance as they both call the exact same code. The difference is the flow of the code being in the method and easier to follow vs jumping to other EndAsync methods. \$\endgroup\$ Jan 4 at 19:52
  • \$\begingroup\$ If you really wanted the whole think async you could replace the blockingcollection with the tpl dataflow or the newer channels and create a pipeline that processes as request come in. Then you wouldn't have a thread tied up the entire time on Take with the blocking collection. But that's a different implementation \$\endgroup\$ Jan 4 at 20:04
  • \$\begingroup\$ Unfortunately, I won't be able to use SendAsync & ReceiveAsync with cancellation token in my WPF Application (No support for .Net 6 yet). Any alternative way to implement the suggested method? \$\endgroup\$
    – Bilal Kazi
    Jan 8 at 11:20
  • 1
    \$\begingroup\$ You can use the same pattern as with connect that doesn't support cancellationtoken. Create a Task.Delay and Task.WhenAny. Then if the receive task isn't completed then you know the Task.Delay hit first and you want to timeout the receive or send request. \$\endgroup\$ Jan 10 at 14:55

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.