Non-Reentrant Class Use In Multithreaded Program

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!

SGaist

What parameters can you pass to that callback ?

Crag_Hack

You can pass via the lpData parameter to the CopyFileEx function. lpDada is LPVOID which is this according to msdn:

A pointer to any type.
This type is declared in WinDef.h as follows:
typedef void *LPVOID;

SGaist

So you can pass a structure or an object that will be available in your callback after casting the pointer to a suitable class.

You can then use QMetaObject::invokeMethod to avoid trouble with respect to the event loop.

Crag_Hack

Do I make the callback function a static worker object class member? Or do I declare it as a global outside of the class? I can't do a normal worker object class member function and still pass it as a callback to the CopyFileEx function right? What's the best way?

Can you show me some code? I'm not terribly well rehearsed in these realms...
Thanks

SGaist

A function outside your class, that callback is an "implementation detail", it should go into your implementation file. Pass your object as parameter to it.
WARNING: Not tested, nor compiled

DWORD CALLBACK CopyProgressRoutine(
	LARGE_INTEGER TotalFileSize,
	LARGE_INTEGER TotalBytesTransferred,
	LARGE_INTEGER StreamSize,
	LARGE_INTEGER StreamBytesTransferred,
	DWORD dwStreamNumber,
	DWORD dwCallbackReason,
	HANDLE hSourceFile,
	HANDLE hDestinationFile,
	LPVOID lpData
	)
{
    int percentage = (double(TotalBytesTransferred.QuadPart) / double(TotalFileSize.QuadPart)) * 100;
    MyCoolClass *object = qobject_cast<MyCoolClass *>(lpData);
    if(object) {
        QMetaObject::invokeMethod(object, 
                                  "nameOfYourMethod", 
                                  Qt::QueuedConnection,
                                  Q_ARG(int, percentage));
    }
    return PROGRESS_CONTINUE;
}


void YouCoolClass::copyFile(const QString& source, const QString& destination)
{
        QByteArray srcUtf8 = QDir::toNativeSeparator(source).toUtf8();
        char *srcFilename = srcUtf8.data();
        QByteArray dstUtf8 = QDir::toNativeSeparator(destination).toUtf8();
        char *dstFilename = dstUtf8.data();
       
	bool returnVal;

	returnVal = CopyFileEx(
		srcFilename,
		dstFilename,
		(LPPROGRESS_ROUTINE)CopyProgressRoutine,
		this,
		NULL,
		COPY_FILE_NO_BUFFERING);

	if(returnVal){
		printf("%s copied to current directory.\n", filename);
	} else {
		printf("%s not copied to current directory.\n", filename);
		printf("Error %u.\n", GetLastError());
	}
}

Crag_Hack

Thanks SGaist! The thing other than static/global/worker member I wasn't sure about was how to get that callback function to call those worker thread object functions. So we have to use QMetaObject::invokeMethod? We can't call the functions directly? What's the reason for that? Is CopyFileEx blocking synchronous? If so wouldn't that prevent queued events from being processed?

SGaist

Depending on what your function does, you'll likely be modifying a GUI element which you shouldn't do outside of the GUI thread. I don't know whether this method uses threads indirectly or not. With the invokeMethod technique used, you're safe from that point of view.

Crag_Hack

The setProgressMeters and setTimeLabels functions are worker thread object functions that emit signals to the main gui thread to update it's widgets. If the callback is called by the worker thread then shouldn't it be located in the worker thread and able to call those functions directly?

SGaist

The thing is: what guarantees do you have that the OS doesn't spin a new thread to handle whatever operation you are asking it to do?

Crag_Hack

I'll share this stackoverflow post here (I am using a static callback worker thread object method instead of a global callback): I ran an experiment using CopyCallEx. I did a qDebug() << QThread::currentThreadId(); in the worker thread and in the CopyFileEx static callback function and they displayed the same thread. Also I put a qDebug() after the CopyFileEx call in the worker thread and the CopyFileEx call blocked synchronously until it finished. Does this mean I can call CopyFileEx directly from my worker thread object and access worker thread object member functions directly from the static callback function?

Crag_Hack

So with regard to your concern with the OS spinning a new thread for CopyFileEx, does my experiment show otherwise? Is it thus safe to access worker thread object methods directly from the progress callback function?
Thanks

SGaist

From the looks of it, since it's a blocking call, it should be safe. However, I'd recommend that you put a comment in your code explaining this.