Sporadic synchronisation issues with QMutex/QReadWriteLock

jars121

This issue has been plaguing me for some time, and I'm at the point where I think I need some expert input.

Before I get to the code, I'll provide some context around the issue. I have a range of digital and analog inputs attached to my embedded Linux device (inputs are provided by DAQ PCI cards run by LabView if anyone's interested). In my application, each digital and analog input is managed on a separate thread; current issue aside, this is working very nicely. I'm using standard sysfs-based 'interrupts' to catch both rising and falling edges of the digital inputs. I say 'interrupts', as sysfs interrupts are basically a poll within a while(1) statement, which you'll see in my example code below.

I use two classes, digitalInput and analogInput, for reading and storing the digital and analog inputs respectively. I use a third class to manage all this data (dataManagement), and provide synchronisation between the digitalInput and analogInput classes, as well as my main class which reads and displays the capture digital and analog values. I do this by way of QReadWriteLocks in the dataManagement class, and use separate locks for the digital and analog inputs.

This approach appears to work in most cases (90% of my tests have been successful). However, there is an occasional issue whereby an event in the digitalInput class (e.g. a switch is turned on and the value goes from LOW to HIGH, causing the input's RISING EDGE detection event), is not received by main class. E.g. the edge detection event is captured successfully, the new value for that input (in this example '1') is saved to the dataManagement QVector (verified with a qDebug() print of the given input in the QVector immediately after replacing the 0 with a 1), but in the main class where I routinely check the value of each of the inputs, the value for this particular input is still 0. E.g. dataManagementPointer->digitalVector().at(givenInput) = 0, when I've verified immediately after updating the value in digitalVector that the value is in fact 1.

This leads me to conclude that there's something amiss with the QReadWriteLock. In this situation, I lock the QReadWriteLock for writing (to write the value of 1 to the dataManagement digitalVector), and in the main class I lock the dataManagement digitalVector for reading. As mentioned above, this works 90% of the time, but 10% of the time, the returned value in the main class is always 0.

With that said, here's some trimmed down example code, which hopefully aligns with my written explanation above.

dataManagement.h:

class DataManagement : public QObject
{
    static DataMangement* instance;
    Q_OBJECT

public:
    static DataManagement *getInstance();
    explicit DataManagement(QObject *parent = nullptr);

    QReadWriteLock *dataManagementAnalogLock = new 
    QReadWriteLock(QReadWriteLock::Recursive);
    QReadWriteLock *dataManagementDigitalLock = new 
    QReadWriteLock(QReadWriteLock::Recursive);

    Q_PROPERTY(QVector<QVariant> analogVector READ analogVector NOTIFY analogVectorChanged);
    QVector<QVariant> analogVector(){return this->m_analogVector;}
    Q_PROPERTY(QVector<int> digitalVector READ digitalVector NOTIFY digitalVectorChange);
    QVector<int> digitalVector(){return this->m_digitalVector;}

signals:
    void analogVectorChanged();
    void digitalVectorChanged();

public:
    QVector<QVariant> m_analogVector;
    QVector<int>       m_digitalVector;
};

digitalInput.cpp:

//Create dataManagement pointer
DataManagement *dataManagementPointer  = DataManagement::getInstance();

//Setup sysfs while(1) interrupt
//The following code executes on sysfs RISING/FALLING edge interrupt

//Lock the dataManagementDigitalLock for writing
dataManagementPointer->dataManagementDigitalLock->lockForWrite();

//Read the interrupt value (either 0 or 1)
if (std::stoi(string(buf)) > 0 {

    //Value is 1; write 1 to the correct index in the digitalVector
    dataManagementPointer->m_digitalVector.replace(selectedInput, 1);
} else {

    /Value is 0; write 0 to the correct index in the digitalVector
    dataManagementPointer->m_digitalVector.replace(selectedInput, 0);
}

//Editing of the digitalVector is complete; unlock the QReadWriteLock
dataManagementPointer->dataManagementDigitalLock->unlock();

//Print the value of the selected input for comparison with the main class
qDebug() << dataManagementPointer->digitalVector().at(selectedInput);

//Emit the digitalVectorChanged signal
emit dataManagementPointer->digitalVectorChanged();

I won't post the analogInput code, as it follows the exact same process as above, instead editing the analogVector, and locking the dataManagementAnalogLock.

the main class (cpp file):

//Create dataManagement point
DataManagement *dataManagementPoint = DataManagement::getInstance();

//Lock the dataManagementDigitalLock for reading
dataManagementPointer->dataManagementDigitalLock->lockForRead();

//Check the value of the given input
if (dataManagementPointer->digitalVector().at(selectedInput) > 0 {
    qDebug() << "Value of input " << selectedInput << " is 1.";
} else {
    qDebug() << "Value of input " << selectedInput << " is 0.";
}
//Unlock the dataManagementDigitalLock
dataManagementPointer->dataManagementDigitalLock.unlock();

Am I using the QReadWriteLock approach correctly? I previously used QMutexes instead, but from the research I've done, I figured the QReadWriteLock approach might be more suitable. My questions are as follows:

Is my understanding correct that calling .lockForWrite() will block until the current thread is able to lock for writing?
Can I improve my methodology by testing for successful lock with tryLockForWrite/Read()? If my assumption in my point above is correct, I shouldn't need to as calling .lockForWrite/Read will simply wait until the previous lock is unlocked.

Given that the qDebug() in digitalInput.cpp accurately prints the correct value 100% of the time (confirming that the sysfs-based interrupt approach is working perfectly), are there any other reasons why a similar call to dataManagementPointer->digitalVector.at(selectedInput) in the main class would produce an incorrect value? I've been over this a thousand times and all I can come up with is a flaw in my QReadWriteLock/QMutex/thread synchronisation approach.

Any input/clarification would be greatly appreciated!

ambershark

Most of it looks good to me. The only potential synchronization issue I see is that you are accessing memory for your DataManagement class from 2 threads with no mutexing.

One thread uses getInstance() and reads/writes to that variable while another thread calls getInstance(), getting that same pointer and attempts to use it possibly while it is being written (although not in the code I can see). In the code you shared it should be safe to do this, however it could have issues in other places since it isn't protected at all. You even use QVector::at which is a read only and would make it safe. This is just kind of a shot in the dark.

I went over your code line by line and the parts you shared have proper protection so I don't think it's a sync issue in these sections at least. If you haven't seen any crashes it's probably not a sync issue anyway. Write lock sync issues tend to cause crashes, at least sporadically enough to realize you probably have one.

Couple questions:

Are you sure selectedInput is the same in both threads? Maybe you are looking at one part of the vector thinking you are looking at another.
Is it possible a second write event/writelock is happening before the signal emitted captures the readlock? This would result in the vector item being cleared before the reading thread had a chance to see it.

Maybe throw some qDebugs() in to show writelocks being set and released, as well as read locks. That way you might be able to get some flow info to see if #2 is happening or not.

Beyond that nothing obvious that I see without being able to debug the code. :(

Is my understanding correct that calling .lockForWrite() will block until the current thread is able to lock for writing?

Correct.

Can I improve my methodology by testing for successful lock with tryLockForWrite/Read()? If my assumption in my point above is correct, I shouldn't need to as calling .lockForWrite/Read will simply wait until the previous lock is unlocked.

Also correct. I don't think you need to change anything, at least in the code you shared.

jars121

Brilliant, thanks @ambershark, much appreciated as always. I've been using qDebug()s as you've recommended, but haven't been able to isolate any issues thus far. As for the selectedInput, it shouldn't even be different between the threads, as the size of the QVector is fixed and set on initialisation. Having said that, I'll expand the digitalInput and main class qDebug()s to print the entire digitalVector rather than the single selectedInput to see if there's anything weird going on there.

Unfortunately I won't be able to do any of that until this time tomorrow, as while I was changing some SPI wires around I accidentally touched a 3V3 input wire against +12V, so I've killed my embedded device hah. I've got another one on the way, so I should be back up and running/testing/fixing tomorrow.

ambershark

@jars121 said in Sporadic synchronisation issues with QMutex/QReadWriteLock:

I accidentally touched a 3V3 input wire against +12V, so I've killed my embedded device hah.

Lol that sucks.

I hope some of it helps. Threading issues are such a pain to deal with. I find just adding more logging and maybe watching in a debugger is the best you can do in those situations.

From what I see your threading looks good... which is a bad thing because it means no easy fix for you. If you had crappy threading I would probably have been able to spot the issue. :)

jars121

It definitely does help! Particularly your first point regarding DataManagement access via getInstance() and the apparent lack of mutexing; I haven't given that any attention as of yet, so will certainly do so first thing tomorrow.

I'm surprised this current device has lasted as long as it has actually! I know what you mean about the code, it's easier when you know it's an absolute mess and it's just a matter of finding which disaster in your code is causing the issue hah.

jars121

My replacement device has arrived, so I'll spend today troubleshooting this issue (amongst other things).

Before I get started however, I wanted to confirm my understanding regarding the use of pointers in my application. In my code above, I use the following line to create a pointer to the DataManagement class:

DataManagement *dataManagementPointer = DataManagement::getInstance();

The corresponding getInstance() function in the DataManagement class looks like this:

DataManagement* DataManagement::instance = 0;

DataManagement *DataManagement::getInstance()
{
    if (instance == 0) {
        instance = new DataManagement;
    }
    return instance;
}

For any given function in any given class in my application that needs to use DataManagement, I've been creating a pointer as per my 'DataManagement *dataManagementPointer' code above. As such, a typical class might look like this:

someClass::someClass(QObject *parent) : QObject(parent)
{
    //Do some unrelated stuff
}

int someClass::function1()
{
    DataManagement *dataManagementPointer = DataManagement::getInstance();

    //Use the dataManagementPointer in this function
}

void someClass::function2()
{
    DataManagement *dataManagementPointer = DataManagement::getInstance();

    //Use the dataManagementPointer in this function
}

etc.

Is this the correct use of the pointer? I've played with putting a single pointer for the entire class at the top of the cpp file (for each of the classes that use DataManagement), but I receive the 'multiple definition of 'dataManagementPointer'' error, which suggests that using a global declaration isn't correct.

Given the nature of the getInstance() function, I imagine that it only creates a new instance of DataManagement the first time it's called, with subsequent calls returning the same instance. Is this true across classes or only within a class? If it's not true across classes, does this mean that if I have 5 classes using dataManagementPointer throughout their functions, there are 5 instances of DataManagement in use? If so, I would think this is going to play havoc with multithreading, and may even be the cause for the issue for which I created this post?

mrjj

Hi
The method you are doing with DataManagement::getInstance();
is called a Singleton
https://stackoverflow.com/questions/270947/can-any-one-provide-me-a-sample-of-singleton-in-c/271104#271104

There is much info on the net. Its often looked down upon a bit since its just a glorified global variable but
you are using it correctly and seem to protects its members during thread use.

Yes, it creates a single instance and all classes uses it.

You should not declare it as global var over the class as that leads to multiple definitions errors.
You can however add it as a member for the class if you want to avoid having
DataManagement *dataManagementPointer = DataManagement::getInstance();
in all member functions.

jars121

Thanks @mrjj, you've provided some much needed clarification and some very interesting reading.

What is the more standard/less looked down upon approach? I'm happy to replace all the singletons.

ambershark

@jars121 Well typically, you would pass the pointer around to the classes and functions that needed it.

Globals (and singletons) are frowned upon for many reasons, but there are some things that they are well suited for. A logger for instance pretty much requires one. There are some things that work well with singletons.

I find if I have an object that everything in my program needs access to and it will only ever have a single copy then I tend to make that a singleton. Configuration classes come to mind. I use singletons a lot there. I don't dislike them as much as most programmers but I do recognize them as glorified globals and I do recognize the issues with global variables.

jars121

I think that's actually how I ended up with all these singletons. The first class I setup was a configuration class, which stored all the values from a QSettings file. The Configuration class is available to all other classes, so a singleton makes sense. I've kept using them for subsequent classes because they seem to work, but I haven't researched or tried any alternatives.

mrjj

Hi
When possible - the alternative is to pass the object at creation

classX c2=new classX(parent, Settings );

or via function

c2->setSettings( Settings)

Also, i when i have must global data/singletons , i always put them in a class that i then
first create in main.
This way, im sure i can control the creation time as QObjects must not be created before Qpplication whichs happens in main.
if you have the data directly in global scope, they are create BEFORE main runs.

jars121

Thanks @mrjj. In your example above, what is the Settings? I've used classX classObject = new classX(); before, but I haven't passed the parent or 'Settings' (whatever that's a reference to) before.

mrjj

@jars121
Hi
Settings just being the class , other classes need access too. ( could be any name )
All QWidgets have a constructor that takes a parent widget so that's why i showed a parent.

For your own non Qt classes, they might not have any parameter to the constructor.
But if the class need other class to function, its very nice to give them as part of the constructor
so u cant get the case where there is an instance that didnt get its external class.

jars121

Thanks again @mrjj.

I'll be completely honest, I'm more confused on this topic than ever. There's plenty of documentation and examples on singletons; what term should I be searching for to better understand the correct approach (as you've demonstrated in your posts above)?

kshegunov

Dependency injection is the formal term. You shouldn't really use singletons, as already mentioned. It may be okay for Java it ain't any good for C++, unless you like pain and suffering.

jars121

Thanks @kshegunov, I never would have come up with "dependency injection" on my own!

I certainly seem to gravitate towards pain and suffering, but I'm doing my best to avoid it with this application ;)

kshegunov

@jars121 said in Sporadic synchronisation issues with QMutex/QReadWriteLock:

Thanks @kshegunov, I never would have come up with "dependency injection" on my own!

Me neither. The Java people feel compelled to make everything into a design pattern and name it apparently, I just call it - "passing arguments to the constructor".

I certainly seem to gravitate towards pain and suffering, but I'm doing my best to avoid it with this application ;)

I think most of us have done it one time or another, then we get more experienced and more cynical and prefer the less painful and suffering-prone approaches. ;)

ambershark

@jars121 said in Sporadic synchronisation issues with QMutex/QReadWriteLock:

Thanks @kshegunov, I never would have come up with "dependency injection" on my own!

I certainly seem to gravitate towards pain and suffering, but I'm doing my best to avoid it with this application ;)

I hate actual dependency injection. For C++ you just setup the constructor (or an accessor method) for your class to take the pointer you need. Actual DI tends to come with a lot of down sides imo. I used a DI implementation one time that any time you accessed an object that was passed in had a callstack of almost 40 functions just to reference a single object. It made debugging a nightmare and of course has a pretty decent performance impact with that many calls.

Here is an example to help you understand. Let's say you have a Settings object (that is not a singleton or global) and you need that in a MainWindow as well as a SomeOtherWidget. Here is how you would do that:

int main(int ac, char **av)
{
   QApplication app(ac, av);

   Settings s;
   s.loadSettings();
   
   MainWindow mw;
   mw.setSettings(&s);
   mw.show();

   return app.exec();
}

// MainWindow
class MainWindow : public QMainWindow
{
public:
   void setSettings(Settings *s) { s_ = s; }

private:
   Settings *s_;
   SomeOtherWidget *sow_;
};

// Now let's say some function in mainwindow opens a new widget that needs Settings
// ...
MainWindow::MainWindow()
{
   sow_ = new SomeOtherWidget(s_); // you can have constructor take it, or use a setSettings like I showed in MainWindow
}

// SomeOtherWidget
class SomeOtherWidget : public QWidget
{
public:
   SomeOtherWidget(Settings *s, QWidget *parent=nullptr) : s_(s), QWidget(parent) { }

private:
   Settings *s_;
};

So you can see that anywhere you need Settings you have it since you passed it around. It's all the same one allocated in main() but you didn't fall prey to the ever tempting global variable. :)

jars121

Thanks @ambershark! I believe the above makes sense, I'll try implementing that approach in place of an existing singleton and see what happens. Thanks again!

jars121

I've been playing with this for a couple of hours, and I'm struggling to apply it to my application. The above example is relatively straight forward, as it has a single class function in main that needs Settings, with the only other class being called from within that main class (MainWindow).

How do I apply this when my main class instantiates multiple classes, all of which require access to each other, and who aren't initiated with a call to a member function? I.e. the class constructor function is run only, there's no call to setSettings() as per the example above.

I.e. my main.cpp:

QApplication app(argc, argv);

class1 class1Ref;
class2 class2Ref;
class3 class3Ref;

//Setup QQmlApplicationEngine
//Set QQmlApplicationEngine rootContexts for the above classes

return app.exec();

In the above main.cpp, how would I pass class3Ref to the class1Ref constructor (for example), to make it available to the member functions of class1?