Sporadic synchronisation issues with QMutex/QReadWriteLock

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?

ambershark

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

Instantiate class3 first then pass it to class1, like so:

class3 class3Ref;
class1 class1Ref(&class3Ref);

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.

jars121

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.

ambershark

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