Non-Reentrant Class Use In Multithreaded Program

kshegunov

@kshegunov said in Non-Reentrant Class Use In Multithreaded Program:

Anything to worry about?

Don't call write operations, including non-const iterator retrieval, on an object from different threads!

Crag_Hack

I have two quick followup questions:

There's no possibility of two different non-reentrant classes sharing the same global (program-wise) resources, e. g. static variables? So say QStorageInfo uses the same globals/statics as QPair, you use QStorageInfo in one thread then QPair in another - they're not gonna fight each other and end up with a race condition and everything is safe right? (arbitrary choice of classes)
I'm not confident enough in my understanding of multithreading to feel comfortable with my code right now - so I'll just state it explicity. I am calling job.sortFileList(); in my worker thread. The job object is a custom class for backup jobs. I send it to the worker thread using signals and slots. The sortFileList() function follows as well as the comparison function. fileList is QList<std::pair<QString, QString> > fileList;

void BackupJob::sortFileList()
{
    qSort(fileList.begin(), fileList.end(), fileListCompare);
}
bool fileListCompare(const std::pair<QString, QString> &leftFileStringPair, const std::pair<QString, QString> &rightFileStringPair)
{...}

fileListCompare() is a function declared in the BackupJob.h file but not a member of the BackupJob class (global right?). Is this call to qSort safe? I'm guessing yes since the worker thread uses its own instance of the class (the class contains no non-reentrant objects, functions, or anything) and the iterators are constant (they're constant right?).
*edit - when I hover above the iterators in Qt Creator they appear as const_iterators not sure why though I didn't declare fileList as const
Thanks for the help!

kshegunov

@Crag_Hack said in Non-Reentrant Class Use In Multithreaded Program:

There's no possibility of two different non-reentrant classes sharing the same global (program-wise) resources, e. g. static variables? So say QStorageInfo uses the same globals/statics as QPair, you use QStorageInfo in one thread then QPair in another - they're not gonna fight each other and end up with a race condition and everything is safe right? (arbitrary choice of classes)

Actually this is exactly what makes the classes non-reentrant - having a shared global/static variable. Objects of those classes are safe to use from different threads only if they're explicitly thread-safe, e.g. QSemaphore/QMutex in contrast to QWidget, which is neither reentrant nor thread-safe.

Is this call to qSort safe?

As long as 2 conditions are met:

the fileListCompare doesn't use global data (i.e. static/global variables) - it's reentrant - or if it does, then all accesses to the globals is serialized between different threads (with a mutex) - it's thread-safe.
fileList isn't modified while qSort is executed. This would be true if BackupJob is reentrant, or more specifically - the thread operates on it's own instance of the fileList data member.

when I hover above the iterators in Qt Creator they appear as const_iterators not sure why though I didn't declare fileList as const

It's a deficiency of the IDE you use. qSort requires non-const iterators. As a side note, you should use std::sort instead of the legacy qSort.

Crag_Hack

Thanks kshegunov! :)

Actually this is exactly what makes the classes non-reentrant - having a shared global/static variable.
I understand that but I wonder whether two different classes ever share the same global/static variable. Then you might be in trouble if you used one in one thread and the other in another thread, even though each object is isolated to its own thread there still is an issue since they share those globals/statics.

1. the fileListCompare doesn't use global data (i.e. static/global variables) - it's reentrant - or if it does, then all accesses to the globals is serialized between different threads (with a mutex) - it's thread-safe.
It only uses the referance parameters passed to the function.

2. fileList isn't modified while qSort is executed. This would be true if BackupJob is reentrant, or more specifically - the thread operates on it's own instance of the fileList data member.
The thread operates on its own instance of a list of backupjob objects or an individual backupjob object which is passed to the thread through signals/slots. fileList is a member variable of the backupJob objects and the backupJob objects only use reentrant variables/objects. So safe right?

As a side note, you should use std::sort instead of the legacy qSort.
I wasn't even aware qSort had been obsoleted. The first google result says nothing of such matters since it's Qt 4.8. For std::sort we just pass it the same QList::begin() QList::constBegin() QList::end() QList::constEnd() right?

And how do I make those neat bars on the left for styling replied to comments?

JKSH

@Crag_Hack said in Non-Reentrant Class Use In Multithreaded Program:

Actually this is exactly what makes the classes non-reentrant - having a shared global/static variable.
I understand that but I wonder whether two different classes ever share the same global/static variable. Then you might be in trouble if you used one in one thread and the other in another thread, even though each object is isolated to its own thread there still is an issue since they share those globals/statics.

Yes, this does happen.

Such classes must be used from the GUI thread only (e.g. QWidget, QPixmap must only be used in the same thread that created a QApplication)

2. fileList isn't modified while qSort is executed. This would be true if BackupJob is reentrant, or more specifically - the thread operates on it's own instance of the fileList data member.
The thread operates on its own instance of a list of backupjob objects or an individual backupjob object which is passed to the thread through signals/slots. fileList is a member variable of the backupJob objects and the backupJob objects only use reentrant variables/objects. So safe right?

Sounds good.

As a side note, you should use std::sort instead of the legacy qSort.
I wasn't even aware qSort had been obsoleted. The first google result says nothing of such matters since it's Qt 4.8.

See http://doc.qt.io/qt-5/qtalgorithms-obsolete.html

Qt 4.8 is obsolete too. Support ended in December 2015.

For std::sort we just pass it the same QList::begin() QList::constBegin() QList::end() QList::constEnd() right?

Yes.

And how do I make those neat bars on the left for styling replied to comments?

Click the "reply"/"quote" link at the bottom-right of each post.

Paragraphs that start with > are treated as quotes.

Crag_Hack

Thanks JKSH

Yes, this does happen.

Such classes must be used from the GUI thread only (e.g. QWidget, QPixmap must only be used in the same thread that created a QApplication)

I forgot to clarify in the previous post not just two objects isolated to their own threads but two objects of two different classes... But you knew that right? Anything else to know? Are these classes the exception, perhaps only graphical classes?

the fileListCompare doesn't use global data (i.e. static/global variables) - it's reentrant - or if it does, then all accesses to the globals is serialized between different threads (with a mutex) - it's thread-safe.

It only uses the referance parameters passed to the function.

And of course this sounds good as well right...

JKSH

@Crag_Hack said in Non-Reentrant Class Use In Multithreaded Program:

I forgot to clarify in the previous post not just two objects isolated to their own threads but two objects of two different classes... But you knew that right?

Yes, I knew that :)

The widget example still stands: Suppose you create a very simple program that consists of only 1 QApplication object and 1 QWidget object. These two objects are not allowed to be in different threads.

Anything else to know? Are these classes the exception, perhaps only graphical classes?

None that I can think of. I'm quite sure it's only the graphical classes with QApplication/QGuiApplication.

the fileListCompare doesn't use global data (i.e. static/global variables) - it's reentrant - or if it does, then all accesses to the globals is serialized between different threads (with a mutex) - it's thread-safe.

It only uses the referance parameters passed to the function.

And of course this sounds good as well right...

You said: fileListCompare() only uses references parameters. I presume you were confirming that fileListCompare() doesn't use global/static variables.

As @kshegunov said: If fileListCompare() doesn't use global/static variables, then fileListCompare() is a reentrant function.

Crag_Hack

A couple more questions creeped into my domain... thankfully they're quick :) is qDebug() reentrant or should it not be used from multiple threads? All STL classes are reentrant right except for <atomic> and std::atomic guys right?

JKSH

@Crag_Hack said in Non-Reentrant Class Use In Multithreaded Program:

A couple more questions creeped into my domain... thankfully they're quick :) is qDebug() reentrant or should it not be used from multiple threads?

qDebug() itself is thread-safe. It can be called simultaneously from multiple threads, without endangering your application.

However, qDebug() sends data to stderr by default, which is unbuffered. That means: If you send a long stream of "A"s from one thread and a long stream of "B"s from another thread, you might see interleaved text when viewing stderr:

"AAAAAAAAAABBBBBBBBBBAAAAAAAAAABBBBBBBBBB"

To avoid this, call qInstallMessageHandler() to send qDebug() output to a buffered stream (like stdout or a file).

All STL classes are reentrant right except for <atomic> and std::atomic guys right?

I'd say so

Crag_Hack

Another related question I conjured up - how come some Qt classes aren't reeentrant that would be quite more useful if they were? I.E. QPair, QStorageInfo. My program makes use of drive storage info in both the main GUI thread and the worker thread (I was forced to use the win32 api in the worker thread since QStorageInfo wasn't reentrant) and I could easily foresee QPair being used in such a way as well.
Thanks ! That might finalize this thread permanently :)

kshegunov

QPair is reentrant, if the docs don't state it, then it's a "bug" in the documentation, please file it as such.

As for QStorageInfo - I don't know if it's reentrant or not, I would have to check the source to find out for sure, but usually the class not being reentrant is due to the underlying API architecture (e.g. the widget classes).

Crag_Hack

Thanks. Perhaps they are both bugs - I would think a useful class that would have any possibility of being used in multiple threads like QStorageInfo would most definitely be reentrant. Is there another factor at play here besides utility?

kshegunov

@Crag_Hack said in Non-Reentrant Class Use In Multithreaded Program:

utility

What do you mean by "utility"? Whether it's reentrant or thread-safe (or both or neither) just depends on the specific implementation. As Qt's (mostly) a platform agnostic toolkit, implementations for some platforms may be reentrant/thread-safe, while implementations for other platforms may be not, in that case the worst-case is documented - i.e. non reentrant/not thread-safe.

kshegunov

After a quick (not exhaustive) look here:
https://code.woboq.org/qt5/qtbase/src/corelib/io/qstorageinfo.h.html
https://code.woboq.org/qt5/qtbase/src/corelib/io/qstorageinfo.cpp.html
https://code.woboq.org/qt5/qtbase/src/corelib/io/qstorageinfo_p.h.html
https://code.woboq.org/qt5/qtbase/src/corelib/io/qstorageinfo_unix.cpp.html
https://code.woboq.org/qt5/qtbase/src/corelib/io/qstorageinfo_win.cpp.html

It seems to me QStorageInfo is reentrant on *nix, but is not on windows (due to ::SetErrorMode). I didn't look at macos' implementation.

Crag_Hack

Why doesn't the Qt Company make a class like QStorageInfo reentrant though? It would seem logical to do so since it's an easy situation to find yourself in needing to use the class in multiple threads.

JKSH

@Crag_Hack said in Non-Reentrant Class Use In Multithreaded Program:

Why doesn't the Qt Company make a class like QStorageInfo reentrant though? It would seem logical to do so since it's an easy situation to find yourself in needing to use the class in multiple threads.

No strong reason why.

You can post this suggestion to https://bugreports.qt.io/

kshegunov

@JKSH said in Non-Reentrant Class Use In Multithreaded Program:

No strong reason why.

I mentioned already, that ::SetErrorMode breaks reentrancy.

JKSH

@kshegunov said in Non-Reentrant Class Use In Multithreaded Program:

I mentioned already, that ::SetErrorMode breaks reentrancy.

Yes, you're right.

The current implementation is non-reentrant, but I wonder if it's possible to remove this dependency on SetErrorMode(). MSDN recommends SetThreadErrorMode() instead of SetErrorMode() -- I'm not familiar with the Windows API though, so I don't know the implications of this change.

kshegunov

@JKSH said in Non-Reentrant Class Use In Multithreaded Program:

possible to remove this dependency

Maybe, maybe not. I don't know what was the intent of the developer that implemented it. He/She may've had good reason to use this, or may not. In any case this may solve the reentrancy only partially. I haven't looked at the macOS's implementation at all, which may very well require a global state, and to be frank only glanced through the *nix and windows ones.

Perhaps it's better to start with a question to the mailing/development list why it was implemented like this, if someone remembers. But I guess a suggestion wouldn't hurt, so perhaps do them in parallel?

Crag_Hack

About to go live in the next week with one last thing to do... I am going to use the Win API for file copy instead of QFile since I can turn off write buffering. To track progress of the file copy I need to pass a callback to the copy function. See here (the lpProgressRoutine guy). The callback needs to access two worker thread object member functions. My worker thread is simply an object I pass to moveToThread:

    workerThread = new QThread(this);
    workerThread->start();
    worker = new Worker;
    worker->moveToThread(workerThread);

class Worker : public QObject
{
    Q_OBJECT
...
all worker thread code here

What's the best way to do this in a thread-safe manner? Here Kuba simply makes the callback function a static class member fuction.

static DWORD CALLBACK copyProgress(...

Is that safe for me to do as long as the static callback function only accesses the reentrant worker thread object? The current code follows. I need to access the setProgressMeters and setTimeLabels functions from the callback function - I can make jobSize and startTime member variables of the worker thread object and readSize and fileSize equivalent variables will be passed to the callback function as part of the CopyFileEx setup.

void Worker::jobCopyFiles(BackupJob &job, QTime &startTime)
{
...
    QByteArray buffer;
    for (int count = 0; !(buffer = sourceFile.read(1000000)).isEmpty() && cancel == false; count++)
    {
        int readSize = buffer.size();
        targetTempFile.write(buffer);
        setProgressMeters(readSize, fileSize, jobSize);
        setTimeLabels(startTime.elapsed(),readSize);
    }

Please let me know if you need more information. Thanks!