Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

Sporadic synchronisation issues with QMutex/QReadWriteLock



  • 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!


  • Moderators

    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:

    1. 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.
    2. 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.



  • 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.


  • Moderators

    @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. :)



  • 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.



  • 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?


  • Lifetime Qt Champion

    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.



  • 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.


  • Moderators

    @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.



  • 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.


  • Lifetime Qt Champion

    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.



  • 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.


  • Lifetime Qt Champion

    @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.



  • 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)?


  • Qt Champions 2017

    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.



  • 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 ;)


  • Qt Champions 2017

    @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. ;)


  • Moderators

    @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. :)



  • 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!



  • 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?


  • Moderators

    @jars121 Well first off this sounds like bad class design if you have that much reliance from one class to another. An Apple shouldn't need to know anything about a Car object. You really want to strive for your objects to be as self reliant as possible for good OOD.

    To answer the question though ... you can do 2 things:

    1. Instantiate class3 first then pass it to class1, like so:
    class3 class3Ref;
    class1 class1Ref(&class3Ref);
    
    1. You can use the setWhatever method, I use Whatever since I have no idea what these classes are, name your functions appropriately:
    class class3;
    
    class class1
    {
    public:
       class1(class3 *p=nullptr) : whatever_(p) { }
       void setWhatever(class3 *p) { whatever_ = p; }
    
    private:
       class3 *whatever_;
    };
    
    // main.cpp
    class1 c1;
    class2 c2;
    class3 c3;
    c1.setWhatever(&c3); // now your class1 knows about your class3
    

    Again this is bad design but otherwise that is how you would deal with it. You can take a pointer to those objects in the constructor as well as I showed in class1 above.



  • Thanks @ambershark, your patience and continued input really is appreciated.

    I think my class design is poor, and I really shouldn't be investigating this approach any further. The reason I've been looking at sharing all these classes, is because I have so many arrays, lists, vectors, etc. in each of the classes, which are all read from/written to in each of the classes.

    I think the approach moving forward will be to only write to a class member array from within that class, so I'll send the data from the external class to a function within the class do provide the array update.

    This would be relatively straight forward on a single threaded application, but I'm hoping that the QMutex/QReadWriteLock process I've already established will manage this approach for the multitude of threads in operation as well.

    As for reading from an external class array, I imagine simply including the class' header file should be sufficient to read?

    I'm struggling with some other components of the project at the moment, so haven't paid this any further thought, but the realisation that I've got a class design issue more than a programming one is somewhat reassuring.


  • Moderators

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

    Thanks @ambershark, your patience and continued input really is appreciated.

    No problem, happy to help. :)

    I think my class design is poor, and I really shouldn't be investigating this approach any further. The reason I've been looking at sharing all these classes, is because I have so many arrays, lists, vectors, etc. in each of the classes, which are all read from/written to in each of the classes.

    I think the approach moving forward will be to only write to a class member array from within that class, so I'll send the data from the external class to a function within the class do provide the array update.

    This is pretty much the same as sharing the classes. You don't really need to change if that's all you're going to do.

    This would be relatively straight forward on a single threaded application, but I'm hoping that the QMutex/QReadWriteLock process I've already established will manage this approach for the multitude of threads in operation as well.

    It should be fine as long as you don't ever write to something that could be being read or written in another thread at the same time.

    As for reading from an external class array, I imagine simply including the class' header file should be sufficient to read?

    It is, or you can use forward declaration like I did in the example above with class3. However having an array of classes isn't a great idea either.

    I'm struggling with some other components of the project at the moment, so haven't paid this any further thought, but the realisation that I've got a class design issue more than a programming one is somewhat reassuring.

    It is not the worst thing for sure. Getting good at OOP takes a long time imo. It's not easy. In fact I was easily 10 years into my C++ career before I stopped passing classes around everywhere, lol. You'll get there and if you find it's a problem then read some books on it and just think about each class and it's interactions with others before you code it. The side effect of bad class design is complexity though. That's probably why you're having issues with your code.



  • Rather than continue to stumble through this issue, I'm trying to take a somewhat more considered, analytical approach. Below is a dependency map I've just put together in Excel, which shows whether classX (on the vertical axis) requires access to classY (on the horizontal axis). As you can see, there is a lot of dependency between classes in my application.

    Class dependency map

    My initial approach was to encapsulate discrete functionality in separate classes. The downside of this, is the need to expose each class to each other class, as each discrete piece of functionality revolves around a common set of data.

    Should I simply consolidate these classes into a set of classes that are independent of each other?


  • Moderators

    @jars121 That's not as bad as I thought it would be. So here's some ideas on how I would handle it.

    You have a mainwindow class that needs to know about a few things like the Configuration class. You can allocated your configuration class in the mainwindow when it is created. Then you can access that in other places fairly easily. That way you've avoided a singleton/global. You can then have a place where you define your MainWindow that is shareable, sometimes I use the QApplication since it's a singleton and you can always get it with qApp. In gui widgets related to the mainwindow, you can get it with:

    QObject *p = this;
    do { p = p->parent(); } while (p->parent() != NULL);
    MainWindow *mw = qobject_cast<MainWindow *>(p);
    Q_ASSERT(mw);
    

    Then you can use your mainwindow pointer to get the classes it knows about, something like:

    auto myConfigClass = mw->configuration();
    

    Class2 could also exist in mainwindow in the same way.

    Without knowing what the rest of these classes are I can't offer too much advice.. If they have to be interdependent based on the design than so be it. It's not the end of the world. In the future just think hard about encapsulation during your object design process and you should be ok.


Log in to reply