Thread implemented as a Singleton

Question

I have a commercial application made with C,C++/Qt on Linux platform. The app collects data from different sensors and displays them on GUI. Each of the protocol for interfacing with sensors is implemented using singleton pattern and threads from Qt QThreads class. All the protocols except one work fine. Each protocol's run function for thread has following structure:

void <ProtocolClassName>::run()
{
while(!mStop)  //check whether screen is closed or not
{

mutex.lock()
  while(!waitcondition.wait(&mutex,5))
  {
   if(mStop)
      return;
  }

  //Code for receiving and processing incoming data

 mutex.unlock();
} //end while
}

Hierarchy of GUI.

1.Login screen. 2. Screen of action.

When a user logs in from login screen, we enter the action screen where all data is displayed and all the thread's for different sensors start. They wait on mStop variable in idle time and when data arrives they jump to receiving and processing data. Incoming data for the problem protocol is 117 bytes. In the main GUI threads there are timers which when timeout, grab the running instance of protocol using

   <ProtocolName>::instance() function

Check the update variable of singleton class if its true and display the data. When the data display is done they reset the update variable in singleton class to false. The problematic protocol has the update time of 1 sec, which is also the frame rate of protocol. When I comment out the display function it runs fine. But when display is activated the application hangs consistently after 6-7 hours. I have asked this question on many forums but haven't received any worthwhile suggestions. I Hope that here I will get some help. Also, I have read a lot of literature on Singleton, multithreading, and found that people always discourage the use of singletons especially in C++. But in my application I can think of no other design for implementation.

Thanks in advance

A Hapless programmer

Answer 1

Not sure it's the cause of what you're seeing, but you have a big fat synchronization bug in your code:

void <ProtocolClassName>::run()
{
    while(!mStop)  //check whether screen is closed or not
    {

        mutex.lock()
        while(!waitcondition.wait(&mutex,5))
        {
            if(mStop)
                return; // BUG: missing mutex.unlock()
        }

        //Code for receiving and processing incoming data

        mutex.unlock();
    } //end while
}

better:

void <ProtocolClassName>::run()
{
    while(!mStop)  //check whether screen is closed or not
    {
        const QMutexLocker locker( &mutex );
        while(!waitcondition.wait(&mutex,5))
        {
            if(mStop)
                return; // OK now
        }

        //Code for receiving and processing incoming data

    } //end while
}

Answer 2

Just a drive-by analysis but this doesn't smell right.

If the application is "consistently" hanging after 6-7 hours are you sure it isn't a resource (e.g. memory) leak? Is there anything different about the implementation of the problematic protocol from the rest of them? Have you run the app through a memory checker, etc.?

Answer 3

I think singleton is not really what you are looking for. Consider this:

You have (lets say) two sensors, each with its own protocol (frame rate, for our purpose).

Now create "server" classes for each sensor instead of an explicit singleton. This way you can hide the details of how your sensors work:

class SensorServer {
protected:
   int lastValueSensed;
   QThread sensorProtocolThread;
public:
   int getSensedValue() { return lastValueSensed; }
}

class Sensor1Server {
public: 
   Sensor1Server() {
        sensorProtocolThread = new Sensor1ProtocolThread(&lastValueSensed);
        sensorProtocolThread.start();
   }
}

class Sensor1ProtocolThread : public QThread {
protected:
    int* valueToUpdate;
    const int TIMEOUT = 1000; // "framerate" of our sensor1
public:
    Sensor1ProtocolThread( int* vtu ) {
        this->valueToUpdate = vtu;
    }
    void run() {
        int valueFromSensor;
        // get value from the sensor into 'valueFromSensor'
        *valueToUpdate = valueFromSensor;
        sleep(TIMEOUT);
    }
}

This way you can do away with having to implement a singleton.

Cheers,

jrh.