Signals and not propagated outside thread

Mark81

I wrote a simple "copier" class, that runs in a different thread:

int main(int argc, char *argv[])
{
    QGuiApplication app(argc, argv);

    Copier copier;
    auto threadCopier = new QThread;
    copier.moveToThread(threadCopier);
    QObject::connect(threadCopier, &QThread::started, &copier, &Copier::run);
    QObject::connect(threadCopier, &QThread::finished, &copier, &Copier::deleteLater);
    threadCopier->start();

    Engine engine(&worker, &copier); // this is my main class
    QObject::connect(&engine, &Engine::quit, &app, &QGuiApplication::quit);
    return app.exec();
}

here the cpp code of the Copier class:

#include "copier.h"
#include <QTimer>
#include <QDir>

Copier::Copier(QObject *parent) : QObject(parent)
{
    connect(this, &Copier::finished, this, &Copier::copyFile);
}

void Copier::copy(QFileInfo source)
{
    _queue.enqueue(source);
    if (_queue.size() == 1) QTimer::singleShot(0, this, &Copier::copyFile);
}

void Copier::copyFile()
{
    qint64 bufferSize = 4194304;
    QByteArray buffer;
    qint64 byteWritten = 0;
    qint64 byteTotal;

    emit progressChanged(0.0);

    if (_queue.isEmpty()) return;
    QFileInfo src = _queue.dequeue();
    QString srcFilePath = src.absoluteFilePath();
    QString dstFilePath = _dstPath + src.fileName();
    QFile srcFile(srcFilePath);
    QFile dstFile(dstFilePath);

    byteTotal = src.size();
    QFileInfo dst(dstFilePath);
    QDir dir = dst.dir();
    if (!dst.isDir() && !dir.exists()) dir.mkpath(dir.path());

    if (srcFile.open(QIODevice::ReadOnly) && dstFile.open(QIODevice::WriteOnly))
    {
        qint64 internalBufferSize = srcFile.size() < bufferSize ? srcFile.size() : bufferSize;

        while (1)
        {
            buffer = srcFile.read(internalBufferSize);
            if (buffer.isEmpty()) break;

            dstFile.write(buffer);
            byteWritten += internalBufferSize;
            emit progressChanged((qreal) byteWritten / byteTotal);
        }
        srcFile.close();
        dstFile.close();
    }

    emit finished();
}

void Copier::run() { }

and its usage in Engine class:

    connect(_copier, &Copier::progressChanged, this, &Engine::copier_progressChanged);
    connect(_copier, &Copier::finished, this, &Engine::copier_finished);

    // ...

    QString dstPath = label.left(label.indexOf("*")).trimmed() + DEST_PATH + "/";
    _copier->setDstPath(dstPath);

    QDirIterator it(SOURCE_PATH);
    while (it.hasNext())
    {
        it.next();
        QFileInfo info = it.fileInfo();
        if (info.isFile())
        {
            _copier->copy(info);
            _dvr.fileTotal++;
        }
    }

here the slots function:

void Engine::copier_progressChanged(qreal value)
{
    _dvr.countFile = value; countFileChanged();
}

void Engine::copier_finished()
{
    _dvr.fileCopied++;
    _dvr.countTotal = (qreal) _dvr.fileCopied / _dvr.fileTotal; emit countTotalChanged();
}

When it runs, it appends several files to the queue and start copying. The progressChanged signals are received and the I can show the progress of the copy. The problem is the finished signal: in the copier thread it is catch and the next file is copied, but in the engine thread all signals are received at the end.

I mean:

append files to queue and start copying first one
when the first has finished the finished signal is fired
this is catch in copier and the slot is called (copy_file) so the next file begins to be copied
BUT the signal is NOT catch in the engine thread!
each file is copied like above
when no more files have to be copied, the engine thread receives all the "finished" signals at once

Where is my mistake?

sierdzio

@Mark81 said in Signals and not propagated outside thread:

threadCopier->start();
Engine engine(&worker, &copier); // this is my main class

You start the thread before the engine has a chance to run the connect calls. This way some of the initial emits of finished() might be missed by the engine.

Mark81

@sierdzio Actually the copy begins when the user press a PushButton, there are no fires of the finished() signal in the constructor. I forgot to mention in the question - I apologize.

sierdzio

@Mark81 said in Signals and not propagated outside thread:

if (info.isFile())
{
_copier->copy(info);

You are not executing this method in a thread at all! This is a direct call to copy() and it will be executed in your main thread.

You have to either trigger the copy by a signal-slot connection, or use QObject::invokeMethod() to fire your method in the thread.

And that explains why the signals are seemingly fired too late - the event loop does not have enough time to process the signals because everything runs on main thread.

I may be wrong here, of course, I just had a quick look at the code.

Mark81

Oh, my fault! I thought that because _copier is running in another thread (from main.cpp) any call to its method will run in that thread!

So, I'm changing the code. I'm not sure about the syntax for registering metatypes to be used with QMetaObject::invokeMethod(). Hence I tried the signal-slot connection.

I moved the copy function in the public slot section of Copier.h. Then I declared a signal copier_copy(QFileInfo source) in Engine.h and added this code in the cpp:

qRegisterMetaType<QFileInfo>("QFileInfo");
connect(this, &Engine::copier_copy, _copier, &Copier::copy);
// ...

emit copier_copy(info);

but unfortunately the behavior is exactly the same!

EDIT:

here the invokeMethod() version:

qRegisterMetaType<QFileInfo>("QFileInfo");
// ..

QGenericArgument argument = Q_ARG(QFileInfo, info);
QMetaObject::invokeMethod(_copier, "copy", argument);

but nothing has changed.

Mark81

"Solved" in this way - even I don't fully understand why:

in copier.cpp

Copier::Copier(QObject *parent) : QObject(parent)
{
    //connect(this, &Copier::finished, this, &Copier::copyFile);
    connect(this, &Copier::finished, this, &Copier::copier_finished);
}

void Copier::copier_finished()
{
    QTimer::singleShot(0, this, &Copier::copyFile);
}

dheerendra

I saw your issue. In the first case 'finished' did not reach engine, becz you never allowed the event loop to start in main thread. I felt your code did not move beyond following line.
"Engine engine(&worker, &copier);"
You can check this by placing debug statement.

I felt that you wanted to make the threaded copy of files.

There two threads(main-thread and copy-thread)
Main-thread as engine
Copy-thread has copier
Your idea was, engine should ask the copier to copy the file
Copier should copy the contents of the file. This copy operation should happen in copy-thread.
Copy-thread should emit the signal saying that copy is 'finished'
This finished signal should be caught by main-thread(engine).
Engine should update progress bar. Now Engine should tell the copier again that start copying new file.
Same task repeat till it copies all the files.

Hope my understanding is correct.

I felt maintaining separate Queue inside the Copier is unnecessary. Signal-Slots across the thread work using Queue.

If this is the case, here is sample code for you.

#include "Copier.h"

Copier::Copier(QObject *parent) : QObject(parent){}
void Copier::run(){}
void Copier::copyFile(int fi)
{
   qDebug() << Q_FUNC_INFO <<  " File Name =" << fi << " Thread ID=" <<   QThread::currentThreadId() << endl;
   QThread::sleep(5);// Simulation code. Here copy file code comes. 
   return copyComplete();
}

Engine::Engine(QObject *parent) : QObject(parent){}

void Engine::copyComplete()
{
   qDebug() << Q_FUNC_INFO << " Copy Complete" <<endl;
   emit startNextFile(x++);
}

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

    qDebug() << Q_FUNC_INFO << " Thread ID=" << QThread::currentThreadId() << endl;

    QThread thr;
    Engine eng;
    Copier cp;
    cp.moveToThread(&thr);
    QObject::connect(&thr,SIGNAL(started()),&cp,SLOT(run()));
    QObject::connect(&cp,SIGNAL(copyComplete()),&eng,SLOT(copyComplete()));
    QObject::connect(&eng,SIGNAL(startNextFile(int)),&cp,SLOT(copyFile(int)));
    thr.start();
    eng.startNextFile(0);

    return a.exec();
}

Hope this helps.

Mark81

It's all correct but point 8. Actually, in my idea the finished() signal is caught by both copier and engine. Copier will start automatically the next queued file (if any), and engine will update the progress bar only.

In this way I can append new files to be copied everywhere in the program without worrying about the current state. For this reason I kept the queue in the copier class. Anyway your suggestion should work in a simplified case.

sierdzio

@dheerendra great post, thanks for chiming in!

@Mark81 said in Signals and not propagated outside thread:

QGenericArgument argument = Q_ARG(QFileInfo, info);

Never use QGenericArgument, it is an internal symbol of Qt. You should just use Q_ARG directly in invokeMethod, however ugly it might look ;-)

JKSH

@sierdzio said in Signals and not propagated outside thread:

You should just use Q_ARG directly in invokeMethod, however ugly it might look ;-)

Alternatively, you can use lambdas with QMetaObject::invokeMethod() since Qt 5.10: http://doc.qt.io/qt-5/qmetaobject.html#invokeMethod-4 (No more Q_ARGs!)

dheerendra

Additional information. You can keep sending the file names to copier. They will be automatically queued and slot will be executed. It is not like filenames need to be sent once the copy is complete. Engine can keep sending them.