Non-Reentrant Class Use In Multithreaded Program

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.

Crag_Hack

I'm implementing the callback now but am having a simple issue.

DWORD Worker::copyProgress(LARGE_INTEGER totalSize, LARGE_INTEGER totalTransferred, LARGE_INTEGER streamSize, LARGE_INTEGER streamTransferred, DWORD streamNo, DWORD callbackReason, HANDLE src, HANDLE dst, LPVOID data)
{
    Worker *thisWorker = qobject_cast<Worker *>(data);

compiler complains:

C2665: 'qobject_cast': none of the 2 overloads could convert all the argument types

could be 'T *qobject_cast<Worker*>(const QObject *)'

or       'T *qobject_cast<Worker*>(QObject *)'

while trying to match the argument list '(LPVOID)'

What's up with this guy? Google didn't show much info...

Crag_Hack

Found what I was looking for...
https://stackoverflow.com/questions/39592202/converting-between-qobject-and-void

Crag_Hack

Another obstacle to overcome - the CopyFileEx function only accepts LPBOOL, which is
typedef BOOL far *LPBOOL; and typedef int BOOL;,
so I can't use my volatile std::atomic_bool cancel I was using before. What's the best way to solve this problem? Do I have to use a normal non-atomic LPBOOL to pass to the CopyFileEx function? If so do I need to use a mutex for the cancel bool that allows me to cancel the file copy logic when requested? If so where do I put the mutexes? Thanks. Here's the current code.

class Worker : public QObject
{
public:
    void stop() { cancel = true; }
    volatile std::atomic_bool cancel;
...
void Replicator::stopWork()
{
    if (isBusy)
        worker->stop();
}

SGaist

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

CopyFileEx

It's not the same variable, don't mix stoping your Worker with Stoping CopyFileEx.

Crag_Hack

But I need to be able to stop the worker thread from the gui thread, not just when the program is scanning files but also when it is copying files, using the same cancel condition as created by clicking the cancel button in the gui thread. All the logic is handled in the worker thread both scanning, volume shadow copy service, file copying and everything. All of these operations need to be cancelled if desired. The worker thread remains alive it just gets reused for each backup that is performed. I did just find a workaround as per your suggestion got my creative juices flowing... I can check the std::atomic_bool cancel variable in the CopyFIleEx progress callback function and if it is true then return PROGRESS_CANCEL. Can you think of another way to do this though? Just curious.