How can I avoid Thread leak with reading SerialPort in C#?

Question

I have an application that requests a response from 3 virtual serial ports using serialPort.DataReceived. This can cause threads to accumulate. Each serial port runs in its own thread.

I can see the thread count go up and down, but trend upward. Over many hours number of threads my go up by 20,000+. Closing the app returns the threads.

private void Init()
{
  //...
  
    // do this for each set of scales
    serialPort.DataReceived -= SerialDataReceivedHandler;
    serialPort.DataReceived += SerialDataReceivedHandler;
    
    ThreadPool.QueueUserWorkItem(a => BeginWeighing(scales));
    
    //..
}

private void BeginWeighing(Scales scales)
{
    //....
    // Request data from serial port device
    serialPort.Write((isC320 ? C300_READ_GROSS : R300_READ_GROSS) + sgCRLF);
    
    //....

}

Here is the event handler. I don't know the exact length of the incoming response (there are several options). I wait 50ms on that thread before reading the full message (I suspect this is the issue).

private void SerialDataReceivedHandler(object sender, SerialDataReceivedEventArgs e)
{
    // Give the sensor a chance to compose all the data after the first bytes are sent.
    // This looks wrong, but I don't know any other way to reliably fetch all the data
    // from the existing response.
    System.Threading.Thread.Sleep(50);

    try
    {
        SerialPort sp = (SerialPort)sender;

        if (sp == null || !sp.IsOpen)
        {
            Debug.WriteLine("[ERROR] Serial port is null or closed");
            return;
        }

        string sensorData = sp.ReadExisting();

        Scales? scales = GetScalesFromSerialPort(sp);

        if (scales != null)
        {
            if (string.IsNullOrEmpty(sensorData))
            {
                Debug.WriteLine("[ERROR] No data in serialport scales event on race " + scales.raceNumber);
            }
            else
            {
                int dataLen = sensorData.Length;
                bool isC320 = scales.isC320;

                // Received gross weight data
                if (dataLen >= 17 && sensorData.StartsWith(isC320 ? Scales.C300_RECEIVED_GROSS : Scales.R300_RECEIVED_GROSS))
                {
                    string hexWeight = sensorData.Substring(9, 8);

                    if (int.TryParse(hexWeight, System.Globalization.NumberStyles.HexNumber, null, out int weightInt))
                    {
                        // Div by 10 for 1 decimal place on R320 scales.
                        UpdateWeight(scales.raceNumber, (double)weightInt / 10d);
                    }
                    else
                    {
                        Debug.WriteLine("Scales (" + scales.raceNumber + ") weight data: (invalid)");
                    }
                }
                else if (dataLen >= 15 && sensorData.StartsWith(isC320 ? Scales.C300_RECEIVED_ZERO : Scales.R300_RECEIVED_ZERO))
                {
                    Debug.WriteLine("[DEBUG] Read Zero from race " + scales.raceNumber + " dataLen: " + dataLen);
                }
                else
                {
                    Debug.WriteLine("[DATA] (unknown) " + sensorData + " from serialport on race " + scales.raceNumber);
                }
            }
        }
    }
    catch (System.IO.IOException ex)
    {
        SerialPort sp = (SerialPort)sender;
        string msg = "[ERROR] SerialDataReceived " + (sp != null ? sp.PortName : "[unknown port]") + " - " + ex.Message;
        Debug.WriteLine(msg);
        TDUtils.WriteLog(msg);
    }
}

SerialPort (a .NET wrapper for the Win32 API), uses threadpool threads to call the DataReceived event handler. I was expecting these threads to be returned to the pool at the end of my handler. I suspect that some are not being returned - potentially caused by my Thread.Sleep(..).

I've seen many "solutions" to this but wondering which ones actually work and fit my scenario and how can I avoid using the Sleep to gather all the data before my next data request (write) is issued?

Answer 1

The problem is that by calling Thread.Sleep(50) in your DataReceived handler, you’re blocking a thread pool thread for 50 milliseconds each time data comes in. Since the event is raised on a thread pool thread, blocking it prevents that thread from being returned immediately to the pool. Over time (especially if data arrives frequently) this causes the thread pool to spin up additional threads to handle new events, which is why you see the thread count climb by thousands.

One possible solution is to accumulate data using a buffer:

• Instead of sleeping, immediately read the available data and append it to a buffer;
• Use a timer or a state machine to detect when the message is complete;
• Once the complete message is assembled, process it and clear the buffer.

This code should work:

private StringBuilder _buffer = new StringBuilder();
private System.Timers.Timer _messageTimer;

public void Setup()
{
    serialPort.DataReceived += SerialDataReceivedHandler;
    _messageTimer = new System.Timers.Timer(50);
    _messageTimer.AutoReset = false; // one-shot timer
    _messageTimer.Elapsed += MessageTimerElapsed;
}

private void SerialDataReceivedHandler(object sender, SerialDataReceivedEventArgs e)
{
    try
    {
        SerialPort sp = (SerialPort)sender;
        if (sp == null || !sp.IsOpen)
        {
            Debug.WriteLine("[ERROR] Serial port is null or closed");
            return;
        }
        
        // Immediately read the available data and append to a buffer.
        string data = sp.ReadExisting();
        if (!string.IsNullOrEmpty(data))
        {
            lock (_buffer)
            {
                _buffer.Append(data);
            }
            // Reset the timer so it fires after 50ms of inactivity.
            _messageTimer.Stop();
            _messageTimer.Start();
        }
    }
    catch (IOException ex)
    {
        Debug.WriteLine("[ERROR] " + ex.Message);
    }
}

private void MessageTimerElapsed(object sender, System.Timers.ElapsedEventArgs e)
{
    string completeMessage;
    lock (_buffer)
    {
        completeMessage = _buffer.ToString();
        _buffer.Clear();
    }
    ProcessMessage(completeMessage);
}

private void ProcessMessage(string message)
{
    // Parse and process your complete message here.
    // For example, update the weight or log errors based on your logic.
}