Threading issues in C++

Question

I have asked this problem on many popular forums but no concrete response. My applciation uses serial communication to interface with external systems each having its own interface protocol. The data that is received from the systems is displayed on a GUI made in Qt 4.2.1.

Structure of application is such that

1. When app begins we have a login page with a choice of four modules. This is implemented as a maindisplay class. Each of the four modules is a separate class in itself. The concerned module here is of action class which is responsible of gathering and displaying data from various systems.

2. User authentication gets him/her into the action screen. The constructor of the action screen class executes and apart from mundane initialisation it starts the individual systems threads which are implemented as singleton.

Each system protocol is implemented as a singleton thread of the form:

class SensorProtocol:public QThread {    
    static SensorProtocol* s_instance;
    SensorProtocol(){}
    SensorProtocol(const SensorProtocol&);
    operator=(const SensorProtocol&);

public:
    static SensorProtocol* getInstance();
    //miscellaneous system related data to be used for
    // data acquisition and processing
};

In implementation file *.cpp:

SensorProtocol* SensorProtocol::s_instance=0;
SensorProtocol* SensorProtocol::getInstance()
{
   //DOUBLE CHECKED LOCKING PATTERN I have used singletons
   // without this overrated pattern also but just fyi  
   if(!s_instance)
   {
       mutex.lock();
       if(!s_instance) 
           s_instance=new SensorProtocol();
       mutex.unlock();
   } 
}

Structure of run function

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

  //code to read from port when data becomes available
  // and process it and store in variables
  mutex.unlock();
}

In the action screen class I have define an InputSignalHandler using sigaction and saio. This is a function pointer which is activated as soon as data arrives on any of the serial ports.

It is a global function (we cannot change it as it is specific to Linux) which is just used to compare the file descriptors of the serial port where data has arrived and the fd's of the sensor systems, if a match is found WaitCondition.wakeOne is invoked on that thread and it comes out the wait and reads and processes the data.

In the action screen class the individual threads are started as SensorProtocol::getInstance()->start().

Each system's protocol has a frame rate at which it sends data. Based on this fact, in actions screen we set up update timers to time out at refresh rate of protocols. When these timers time out the UpdateSensorProtocol() function of operation screen is called

connect(&timer, SIGNAL(timeout), this,SLOT(UpdateSensorProtocol()));

This grabs an instance of sensor singleton as

SensorProtocol* pSingleton=SensorProtocol::getInstance();
if(pSingleton->mUpdate)
{
    //update data on action screen GUI
   pSingleton->mUpdate=false;  //NOTE : this variable is set to
                               // true in the singleton thread
                               // while one frame is processed completely
}

For all uses of singleton instance SensorProtocol::getInstance() is used. Given the above scenario, One of my protocols is hanging no matter what changes I do.

The hang occurs in the while displaying data using UpdateSensorProtocol() If I comment ShowSensorData() function in the UpdateSensorProtocol() it works fine. But otherwise it hangs and the GUI freezes. Any suggestions!

Also, Since the main thread grabs the running instance of singleton, is it really multithreading because we are essentially changing mUpdate in singleton itself albeit from action screen.

I am confused in this.

Also, Can somebody suggest an alternate design as to what I am doing now.

Thanks In Advance

Answer 1

I would start by using RAII (Resource Acquisition Is Initialization) to improve the safety of your locking code. You have code that look like this:

mutex.lock();
...logic...
mutex.unlock();

Wrap the mutex code inside a class where the mutex gets acquired in the ctor and released in the dtor. Now your code looks like this:

MyMutex mutex;
...logic...

The major improvement is that if any exceptions throw in the logic part, your mutex still gets released.

Also, don't let any exceptions leak out of your threads! Catch them even if you don't know how to handle them other than logging it somewhere.

Answer 2

Take a look at QextSerialPort:

QextSerialPort is a cross-platform serial port class. This class encapsulates a serial port on both POSIX and Windows systems.

QextSerialPort inherits from QIODevice and makes serial port communications integrate more smoothly with the rest of the Qt API.

Also, you could use a message passing scheme for communications between the I/O and GUI threads instead of shared memory. This is often much less error prone. You can use the QApplication::postEvent function to send custom QEvent messages to a QObject to be processed in the GUI thread with the QObject::customeEvent handler. It will take care of synchronization for you and alleviate your deadlock problems..

Here is a quick and dirty example:

class IODataEvent : public QEvent
{
public:
    IODataEvent() : QEvent(QEvent::User) {}

    // put all of your data here
};

class IOThread : public QThread
{
public:
    IOThread(QObject * parent) : QThread(parent) {}

    void run()
    {
    for (;;) {
            // do blocking I/O and protocol parsing
            IODataEvent *event = new IODataEvent;
            // put all of your data for the GUI into the event
            qApp->postEvent(parent(), event);
            // QApplication will take ownership of the event
        }
    }
};

class GUIObject : public QObject
{
public:
    GUIObject() : QObject(), thread(new IOThread(this)) { thread->start() }

protected:
    void customEvent(QEvent *event)
    {
        if (QEvent::User == event->type) {
            IODataEvent *data = (IODataEvent *) event;
            // get data and update GUI here
            event->accept();
        } else {
            event->ignore();
        }
        // the event loop will release the IODataEvent memory automatically
    }

private:
    IOThread *thread;
};

Also, Qt 4 supports queing signals and slots across threads.

Answer 3

your getInstance method could maybe be written like this as well to avoid having the s_instance var:

SensorProtocol& getInstance()
{
  static SensorProtocol instance;
  return instance;
}

Answer 4

The double checked locking pattern is broken in C++. This is well documented all over the internet. I don't know what your problem is but clearly you will need to resolve this in your code.

Answer 5

Problems:

Use RAII to lock/unlock your mutexes. They are currently not exception safe.

while(!mStop)
{
  mutex.lock()

  while(!WaitCondition.wait(&mutex,5))
  {
    if(mStop)
    {   
        // PROBLEM 1: You mutex is still locked here.
        // So returning here will leave the mutex locked forever.
        return;    
    }

    // PROBLEM 2: If you leave here via an exception.
    // This will not fire, and again you will the mutex locked forever.
    mutex.unlock();

    // Problem 3: You are using the WaitCondition() incorrectly.
    // You unlock the mutex here. The next thing that happens is a call
    // WaitCondition.wait() where the mutex MUST be locked
 }
 // PROBLEM 4
 // You are using the WaitCondition() incorrectly.
 // On exit the mutex is always locked. So nwo the mutex is locked.

What your code should look like:

while(!mStop)
{
  MutextLocker   lock(mutex);  // RAII lock and unlock mutex.

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

    //code to read from port when data becomes available
    // and process it and store in variables
 }

By using RAII it solves all the problems I spotted above.

On a side note.

Your double checked locking will not work correctly.
By using the static function variable suggested by 'Anders Karlsson' you solve the problem because g++ guarantees that static function variables will only be initialized once. In addition this method guaranteed that the singelton will be correctly destroyed (via destructor). Currently unless you are doing some fancy stuff via onexit() you will be leaking memory.

See here for lots of details about better implementation of singleton.
http://stackoverflow.com/questions/1008019/c-singleton-design-pattern/1008289#1008289

See here why your double checked locking does not work.
http://stackoverflow.com/questions/367633/what-are-all-the-common-undefined-behaviour-that-c-programmer-should-know-about/367690#367690

Answer 6

Thank You all for your prompt responses. Wow this is a great place to share your views. I haven't seen so many responses to this problem. Keep the discussion going guys. I have implemented some of your suggestions. Awaiting results. Also, the current design of application was done by my senior. He left and I am being made the scapegoat. But One thing I learned from working on this problem is "Think mighty hard when using the singleton" this should be the last thing in your bag of tricks. Every literature I read on singletons has split verdict. But mostly for C++ singletons I read negative reviews.

And yes. Can anyone give me a brief alternative for the sensor protocol and its interaction with GUI thread.

Answer 7

Have three sepearate threads for send, receive and display.

Raise an event whenever data is received and handle that within the display thread.

Edit in response to comment 1

I'll admit that I know nothing of qt but from what you've said it would still appear that you can create your serial port object which in turn starts up two worker threads (by use of a start method) for the input and output buffer control.

If the serial port class has a "Connect to port" method to gain use of the serial port; an "Open port" method which starts up your worker threads and opens the port; a "Close port" method to shutdown the send and receive threads and a property for setting the "On Data Received" event handler then you should be all set.

The class shouldn't need to be a singleton as you'll find that most operating systems wont allow more than one process to control a serial port at any one time, instead you'll get an exception (which you need to handle) when you try and connect if it is already in use. The worker threads ensure that the port is held under you're control.

Answer 8

First off all don't make the Systems singletons. Use some kind of Context Encapsulation for the different system.

If you ignoe this advice and still want to create "singletons" threads at least use QApplication::instance(); as the parent of the thread and put QThread::wait() in the singleton destructors otherwise your program will crash at the program exit.

if(!s_instance){
    QMutexLocker lock(&mutex);
    if(!s_instance) 
        s_instance=new SensorProtocol( QApplication::instance());
}

But this isn't going to solve your problem ...
Qt is event driven so try to exployed this very nice event-driven architecture and create a eventloop for each system thread. Then you can create "SystemProtocols" that live in another threads and you can create timers, send events between threads, ... without using low level synchronization objects.
Have a look at the blog entry from Bradley T. Hughes Treading without the headache

Code is not compiled but should give you a good idea where to start ...

class GuiComponent : public QWidget {
    //...
signals: 
    void start(int); // button triggerd signal
    void stop();     // button triggerd singal 

public slots:
    // don't forget to register DataPackage at the metacompiler
    // qRegisterMetaType<DataPackage>();
    void dataFromProtocol( DataPackage ){
        // update the gui the the new data 
    }
};

class ProtocolSystem : public QObject {
     //...
    int timerId;

signals:
    void dataReady(DataPackage);

public slots:
    void stop() {
       killTimer(timerId);  
    }

    void start( int interval ) {
       timerId = startTimer();  
    }

protected:
    void timerEvent(QTimerEvent * event) {
       //code to read from port when data becomes available
       // and process it and store in dataPackage
       emit dataReady(dataPackage);
    }
};

int main( int argc, char ** argv ) {

    QApplication app( argc, argv );
    // construct the system and glue them together
    ProtocolSystem protocolSystem;
    GuiComponent gui;
    gui.connect(&protocolSystem, SIGNAL(dataReady(DataPackage)), SLOT(dataFromProtocol(DataPackage)));
    protocolSystem.connect(&gui, SIGNAL(start(int)), SLOT(start(int)));
    protocolSystem.connect(&gui, SIGNAL(stop()), SLOT(stop()));
    // move communication to its thread
    QThread protocolThread;
    protocolSystem.moveToThread(&protocolThread);
    protocolThread.start(); 
    // repeat this for other systems ...
    // start the application
    gui.show();
    app.exec();
    // stop eventloop to before closing the application
    protocolThread.quit();
    protocolThread.wait();
    return 0;    
}

Now you have total independent systems, gui and protocols don't now each other and don't even know that the program is multithreaded. You can unit test all systems independently in a single threaded environement and just glue them together in the real application and if you need to, divided them between different threads.
That is the program architecture that I would use for this problem. Mutlithreading without a single low level synchronization element. No race conditions, no locks, ...