Search code examples
c++multithreadingqtqthread

QThread: safe way of modifying a variable from different thread?


I am new to QThread and multithreading, so I am not sure if I am doing it correctly. The program has not crashed so far, but I want to check if I am doing it correctly. I have some code as follows (MyThreadClass is inherited from QThread):

std::vector<MyThreadClass* > workThreads;
for(int i=0;i<Solutions.size();i++)
{
    workThreads.push_back(new MyThreadClass(Solutions[i]));
}
for(int i=0;i<workThreads.size();i++)
{
    connect(workThreads[i], SIGNAL(finished()), this, SLOT(onFinished()));
    workThreads[i]->start();
}

bool finished = false;
while(!finished)
{
    if(m_finishedThread==workThreads.size())
        finished=true;

    this->msleep(10);
}

The onFinished function is given as follows:

void MyClass::onFinished()
{
    ++m_finishedThread;
}

So you can see that the while loop is waiting for all the threads to finish and updated the m_finishedThread variables. Is this a safe way of doing it? If all threads finish their job and trying to "connect" to onFinished() function, will it cause a problem?


Solution

  • That's safe. The trick is that the connection you will establish between your QThread subclasses and this will be a queued connection. That's because the thread that emits the signal finished (*) is different from the thread this (the receiver) lives in.

    A queued connection is implemented via event posting. A special event is posted to the event queue of the thread this lives in; the handling of that event is calling your slot. So, your slot will only be accessed

    1. sequentially
    2. from one thread only: the one this lives in

    and that's obviously thread safe.

    By the way, is that your real code? You could do exactly the same with

    for (int i = 0; i < numThreads; ++i) 
        threads[i]->wait();
    

    (*) I'm talking about the thread which emits the signal, not the affinity of the object that emits the signal. In particular, chances are that your QThread subclass objects are living in the very same thread than this!

    Where's the catch? The thread which emits finished is not the one those objects (and this) are living in -- is the thread managed by those objects.


    Addendum (2): this is the code that implements signal emission. As you can see, the check is against the thread currently running vs. the thread the receiver lives in. The thread the sender lives in is not taken into account, so you don't need to do stuff like thread->moveToThread(thread) to make a queued connection.

    How's it possible that the thread currently running isn't the same thread the sender lives in? Because that's exactly what QThread does: the QThread object lives in one thread, the thread it manages (and which emits finished()!) is another thread:

    // runs in the MANAGED thread; lives in any thread
    void QThread::run_in_another_thread() {
        run(); // user-supplied implementation
        emit finished();
    }
    

    Addendun to the addendum: but then, are you relying on the undocumented knowledge that finished gets emitted from another thread? Isn't that relying on the implementation?

    The answer is no. There's only one other choice: finished() gets emitted as part of a reaping/cleanup handling, coming from the event loop the QThread object is leaving. That is, the same thread this is living in.

    It can't come from anywhere else -- if we're running user code, we aren't running anything else (remember that one thread can't be running two different things).

    That implies:

    1. That you have a running event loop in the thread this lives in. You need this in any case.
    2. That the invokation will be "direct" (i.e. direct call), because the thread emitting the signal is the same one this lives in. So, we're just calling a function within the same thread; by definition, that's thread safe.

    So, this changes absolutely nothing about the way we handle this. It requires an active event loop in this, and that's it.


    Addendum: I should've read the code better. This doesn't change what I said above, but:

    bool finished = false;
    while(!finished)
    {
        if(m_finishedThread==workThreads.size())
            finished=true;
    
        this->msleep(10);
    }
    

    This loop here never returns to the event loop. This means your slot will never be invoked because the metacall events will never be processed. Please use a QTimer or sneak in some calls to QCoreApplication::processEvents instead of this kind of loops!