CODEWITHSUNDEEP
c++multithreadingqtqthread
John Yang
John Yang

Reputation: 577

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?

Upvotes: 1

Views: 3987

Answers (5)

V&#237;ctor Fern&#225;ndez
V&#237;ctor Fern&#225;ndez

Reputation: 1824

First of all, you should not subclass QThread directly. See: https://www.qt.io/blog/2010/06/17/youre-doing-it-wrong

Create a normal class instead (a subclass of QObject or similar) and implement your code there. Then use moveToThread() to move the subclass to that thread.

It is safe to connect signals to slots in your object using a Qt::QueuedConnection, as long as the passed parameters are not objects and you are not calling methods on those objects without any synchronization mechanism.

Anyway, what you want to do is probably easier to implement using QtConcurrent.

Upvotes: 1

Sebastian Lange
Sebastian Lange

Reputation: 4029

I probably would not run the while(!finished) loop, since this hangs your mainthread (or executing thread of the while loop). Instead check in onFinished() for running threads and emit a signal when everything is finished.

If you need to wait for all threads to be finished there is still wait()-Condition.

For modifying a variable you can use a Qt:QueuedConnection with a signal to modify a variable if it is not directly accesible.

Be careful with accessing variables from multiple threads directly. It is very often a good idea to use a QMutex when accessing the same variable from different threads.

Upvotes: 1

peppe
peppe

Reputation: 22724

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!

Upvotes: 4

JBL
JBL

Reputation: 12907

At first sight I'd say no. Indeed, you've connected your threads finished() signal without specifying the last parameter of connect (which is the ConnectionType )

As you can see from the documentation, if you don't specify this parameter, Qt::AutoConnection is the default one, and specify that the program will behave differently if the signal comes from an object in the same thread, or in a different thread.

In this case, Qt::QueuedConnection will be used for these calls, which means that when the signal will trigger a call to the slot, this call be queued in the event loop of the receiving thread. If multiple thread finish at the same time, calls to onFinished() will be queued, and invoked one by one.

EDIT: In addition, I suggest you consider a different approach for your design based on thread, and read this which says you should rather create a worker object and then move it to the thread.

Upvotes: 0

Julien Lopez
Julien Lopez

Reputation: 1021

nop, seems ok, Qt's signal/slots system is thread safe, actually, when a signal is emitted, it's put at the end of a queue, and is processed by the main event loop in that order.

Upvotes: -2

Related Questions