local static QMutex

[mentalmushroom]

Hello, could somebody answer me if it is safe to use static QMutex on the stack of the function that is called by several threads (see the same code below). Static variables are created only once, but I'm afraid, perhaps, another thread may try to use it when it is not yet created.

// safe or not? 
void testFunc()
{
 static QMutex mutex;
 mutex.lock();
 // access shared resource
 mutex.unlock();
}

// safe or not? 
void testFunc()
{
 static QMutex mutex;
 mutex.lock();
 // access shared resource
 mutex.unlock();
}

To copy to clipboard, switch view to plain text mode 

[wysota]

if it is safe to use static QMutex on the stack

It is not safe because such things don't exist. Static variables are not allocated on the stack. Don't mix "stack" and "local" variables.

By the way, mutexes should be used to protect data, not code. Yours protects code.

[mentalmushroom]

Indeed static variables are not allocated on the stack, otherwise they would be lost when function returns. Sorry, my question was incorrect, so I'll ask it different: is it safe to use local static QMutex instance?
Also could you, please, explain what did you mean saying

By the way, mutexes should be used to protect data, not code. Yours protects code.

Yes, mutex protects the data, but we protect it from the code, don't we? Here is more complete sample. Do you find it wrong?

QMutex mutex;
bool sharedBoolVariable;
 
void setValueOfSharedResource(bool newValue)
{
 // protect the shared data
 mutex.lock();
 sharedBoolVariable = newValue;
 mutex.unlock();
}

QMutex mutex;
bool sharedBoolVariable;

void setValueOfSharedResource(bool newValue)
{
 // protect the shared data
 mutex.lock();
 sharedBoolVariable = newValue;
 mutex.unlock();
}

To copy to clipboard, switch view to plain text mode 

[wysota]

is it safe to use local static QMutex instance?

I don't know, probably not, however it doesn't make much sense to do so anyway.

Also could you, please, explain what did you mean saying Yes, mutex protects the data, but we protect it from the code, don't we? Here is more complete sample. Do you find it wrong?

You are protecting code (the function) and not the data. If the data is shared, there are at least two places where you access it from thus having a local mutex (static or not) doesn't make sense. Your last snippet is better (however we only see one function using it) and is not prone to the problem you stated (since it is initialized before the program is launched).

[mentalmushroom]

You are protecting code (the function) and not the data. If the data is shared, there are at least two places where you access it from thus having a local mutex (static or not) doesn't make sense. Your last snippet is better (however we only see one function using it) and is not prone to the problem you stated (since it is initialized before the program is launched).

One function may be called by several different threads, therefore the mutex is used: it protects sharedBoolVariable from being written to at the same time. In the second sample I didn't use local mutex (it was the different question), although, it is often convenient to use static local QMutex in such a case. I've noticed this approach in the code of another more experienced programmer, but it appeared suspicious to me, so I wanted to clear it up.

[wysota]

One function may be called by several different threads

Exactly Thus you are protecting the function and not the variable. Somehow you don't care that one actually has to READ the variable somewhere.

it is often convenient to use static local QMutex in such a case. I've noticed this approach in the code of another more experienced programmer, but it appeared suspicious to me, so I wanted to clear it up.

In the first code you posted the use is incorrect. It's a stupid shortcut of a lazy programmer who didn't want to do it properly. It protects access to this method and to this method only.

[mentalmushroom]

Exactly Thus you are protecting the function and not the variable. Somehow you don't care that one actually has to READ the variable somewhere.

Ok, I know that, and I am not going to fall into a dispute about it (just wanted to be sure I understand you right). However, sometimes (rarely) it is not needed to read anything: that was just a theoretic sample, it could be protection for writing a log when you don't like text from different functions to be mixed or adding/removing thread from the list when you simply don't use that list anywhere else or some kind of a wrapper for not reenterant function.

Thank you for paying me attention, Master of Zen

[wysota]

However, sometimes (rarely) it is not needed to read anything

What's the point of setting something if nobody cares about it?

it could be protection for writing a log when you don't like text from different functions to be mixed

You need to setup the log first somewhere.

or adding/removing thread from the list

If you have a single function that both adds and removes from the list then sure however this is not very convinient. Still you are protecting code and not data and that's breaking a primary rule of using critical sections.

some kind of a wrapper for not reenterant function.

Again, protecting code instead of data that makes the function not reentrant. Functions are not inherently reentrant. It's the use of data inside them that makes them non reentrant. And by the way, reentrancy is about accessing the same function twice from the same thread so mutexes can't fix reentrancy problems.

Here's an example of a non reentrant function:

char *toUpper(char *txt) {
  static char buf[256];
  char *ptr = txt;
  char *bufPtr = buf;
  while(ptr) {
    *(bufPtr++) = toUpper(*(ptr++));
  }
  *bufPtr = 0;
  return buf;
}

char *toUpper(char *txt) {
  static char buf[256];
  char *ptr = txt;
  char *bufPtr = buf;
  while(ptr) {
    *(bufPtr++) = toUpper(*(ptr++));
  }
  *bufPtr = 0;
  return buf;
}

To copy to clipboard, switch view to plain text mode 

This call is not allowed with it:

char *result = toUpper(toUpper("xyz"));

char *result = toUpper(toUpper("xyz"));

To copy to clipboard, switch view to plain text mode 

[mentalmushroom]

I decided to create a sample application where the local static QMutex is used. Mutex is used in the print function. If I don't use it, text is printed in a wrong order. It seems to work fine, except it, perhaps, violates some rule you mentioned. I didn't notice any bugs or any other problems so far, do you think this code is incorrect? Honestly, I don't know the rule you are talking about. Where can I read it?

#include <QtCore/QCoreApplication>
#include <QThread>
#include <QMetaType>
#include <QMutex>
#include <iostream>
 
void print(const QString &message)
{
	using namespace std;
 
	static QMutex mutex;	
	mutex.lock();		//	if we run it without mutex, lines may be mixed 
	cout << QThread::currentThreadId() << " " << qPrintable(message) << endl;
	mutex.unlock();
}
 
class Printer: public QObject
{
	Q_OBJECT
 
public slots:
	void startPrinting()
	{	
		for (int i = 0; i < 1000; ++i)
		{
			//	perform some processing...
			//	...
 
			//	print something on the screen/to the log/whatever
			print(QString("i = %1").arg(i));
		}
 
		emit finished();
	}
 
signals:
	void finished();
};
 
//Q_DECLARE_METATYPE(Printer*)
 
int main(int argc, char *argv[])
{
	QCoreApplication a(argc, argv);
 
	QThread *t1 = new QThread;
	Printer *printer1 = new Printer;
	printer1->moveToThread(t1);
 
	QThread *t2 = new QThread;	
	Printer *printer2 = new Printer;
	printer2->moveToThread(t2);
 
	QObject::connect(t1, SIGNAL(started()), printer1, SLOT(startPrinting()));
	QObject::connect(printer1, SIGNAL(finished()), t1, SLOT(quit()));
	QObject::connect(t1, SIGNAL(finished()), printer1, SLOT(deleteLater()));
	QObject::connect(t1, SIGNAL(finished()), t1, SLOT(deleteLater()));
 
	QObject::connect(t2, SIGNAL(started()), printer2, SLOT(startPrinting()));
	QObject::connect(printer2, SIGNAL(finished()), t2, SLOT(quit()));
	QObject::connect(t2, SIGNAL(finished()), t2, SLOT(deleteLater()));
	QObject::connect(t2, SIGNAL(finished()), printer2, SLOT(deleteLater()));
 
	t1->start();
	t2->start();
 
	return a.exec();
}
 
#include "main.moc"

#include <QtCore/QCoreApplication>
#include <QThread>
#include <QMetaType>
#include <QMutex>
#include <iostream>

void print(const QString &message)
{
	using namespace std;

	static QMutex mutex;	
	mutex.lock();		//	if we run it without mutex, lines may be mixed 
	cout << QThread::currentThreadId() << " " << qPrintable(message) << endl;
	mutex.unlock();
}

class Printer: public QObject
{
	Q_OBJECT

public slots:
	void startPrinting()
	{	
		for (int i = 0; i < 1000; ++i)
		{
			//	perform some processing...
			//	...

			//	print something on the screen/to the log/whatever
			print(QString("i = %1").arg(i));
		}

		emit finished();
	}

signals:
	void finished();
};

//Q_DECLARE_METATYPE(Printer*)

int main(int argc, char *argv[])
{
	QCoreApplication a(argc, argv);

	QThread *t1 = new QThread;
	Printer *printer1 = new Printer;
	printer1->moveToThread(t1);

	QThread *t2 = new QThread;	
	Printer *printer2 = new Printer;
	printer2->moveToThread(t2);

	QObject::connect(t1, SIGNAL(started()), printer1, SLOT(startPrinting()));
	QObject::connect(printer1, SIGNAL(finished()), t1, SLOT(quit()));
	QObject::connect(t1, SIGNAL(finished()), printer1, SLOT(deleteLater()));
	QObject::connect(t1, SIGNAL(finished()), t1, SLOT(deleteLater()));
	
	QObject::connect(t2, SIGNAL(started()), printer2, SLOT(startPrinting()));
	QObject::connect(printer2, SIGNAL(finished()), t2, SLOT(quit()));
	QObject::connect(t2, SIGNAL(finished()), t2, SLOT(deleteLater()));
	QObject::connect(t2, SIGNAL(finished()), printer2, SLOT(deleteLater()));

	t1->start();
	t2->start();

	return a.exec();
}

#include "main.moc"

To copy to clipboard, switch view to plain text mode 

[wysota]

This will seem to work fine too however is completely incorrect and some day when you least expect it will explode in your face:

QString x = "abc";
printf("%s\n", x.toLocal8Bit().constData());

QString x = "abc";
printf("%s\n", x.toLocal8Bit().constData());

To copy to clipboard, switch view to plain text mode 

Sorry, it's no argument for me that something "works".

By the way, this will also "works" most of the time:

int x, y; // uninitialized
int z = x/y;

int x, y; // uninitialized
int z = x/y;

To copy to clipboard, switch view to plain text mode 

[mentalmushroom]

This will seem to work fine too however is completely incorrect and some day when you least expect it will explode in your face

Absolutely agree, I've already experienced such situations. But here I don't see any reason for that, except local static QMutex that I am not sure work correct.

QString x = "abc";
printf("%s\n", x.toLocal8Bit().constData());

What is wrong with this code (just curious)?

[wysota]

But here I don't see any reason for that, except local static QMutex that I am not sure work correct.

You are breaking rules. When you do that, it's "boo" and not "yay". Regardless if something works or not. If you (or rather someone else) ever need to refactor your code, it is very likely you will miss the mutex declaration inside the print function.

What is wrong with this code (just curious)?

Well... maybe this is not that much incorrect as you will get away with it with modern compilers but in general QByteArray created by toLocal8Bit() is a temporary variable that might get deleted before you use the data pointer exposed by constData(). However this is certainly incorrect:

const char * dat = x.toLocal8Bit().constData();
printf("%s\n", dat);

const char * dat = x.toLocal8Bit().constData();
printf("%s\n", dat);

To copy to clipboard, switch view to plain text mode 

Nevertheless many people use it and claim it to work. It will work most of the time because there is a good chance nothing overwrites the stack based byte array object right after it was deleted (however if your process context switches out between those two lines and registers are dumped to stack, it will be overwritten) and a slightly less chance that the private component allocated on the heap will not be overwritten too.