Non-Reentrant Class Use In Multithreaded Program

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.

kshegunov

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

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?

This sounds like a decent plan to me.

Crag_Hack

Will this code maintain the atomicity of the cancel variable? And should I declare cancel volatile? I'm guessing std:atomic_bool will make that unncecessary...

class Worker : public QObject
{
    bool getStop() { return cancel; }
    volatile std::atomic_bool cancel;
...
DWORD Worker::copyProgress(LARGE_INTEGER totalSize, LARGE_INTEGER totalTransferred, LARGE_INTEGER, LARGE_INTEGER, DWORD, DWORD, HANDLE, HANDLE, LPVOID data)
{
    Worker *worker = static_cast<Worker *>(data);
    if (worker->getStop())
        return PROGRESS_CANCEL;

kshegunov

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

Will this code maintain the atomicity of the cancel variable?

Yes, of course.

And should I declare cancel volatile?

No, no reason to do that. Forget volatile, especially if you're not intimately familiar with what it does. It's very special and here there is no a proper reason to use it.

I'm guessing std:atomic_bool will make that unncecessary...

That is correct. volatile means something quite different anyway, it doesn't provide atomicity at all.

Crag_Hack

Thanks! Almost time to go live!