Qt: How to avoid deadlock when multiple queued signals invoke same slot

Question

In following code I meet deadlock in someOperation:

class A : public QObject {
    Q_OBJECT
public:
    explicit A(QObject* parent) : QObject(parent), data(0) {}
public slots:
    void slot1() {
        someOperation();
    }
    void slot2() {
        someOperation();
    }
    void slot3() {
        someOperation();
    }
private:
    void someOperation() {
        QMutexLocker lk(&mutex);
        data++;
        QMessageBox::warning(NULL, "warning", "warning");
        data--;
        assert(data == 0);
    }
    int data;
    QMutex mutex; //protect data
};

class Worker: public QThread {
    Q_OBJECT
public:
    explicit Worker(QObject* parent) : QThread(parent) {}
protected:
    virtual void run() {
        // some complicated data processing
        emit signal1();
        // other complicated data processing
        emit signal2();
        // much complicated data processing
        emit signal3();
        qDebug() << "end run";
    }
signals:
    void signal1();
    void signal2();
    void signal3();

};

int main(int argc, char *argv[])
{
        QApplication app(argc, argv);
        A* a = new A(&app);
        Worker* w = new Worker(a);
        QObject::connect(w, SIGNAL(signal1()), a, SLOT(slot1()), Qt::QueuedConnection);
        QObject::connect(w, SIGNAL(signal2()), a, SLOT(slot2()), Qt::QueuedConnection);
        QObject::connect(w, SIGNAL(signal3()), a, SLOT(slot3()), Qt::QueuedConnection);
        w->start();

        return app.exec();
}

There is a thread that will emit three signals, all of them queued connected to an instance of class A, and all class A' slots will call to someOperation, and someOperation is protected by mutex and it will popup a message box.

Qt::QueuedConnection 2 The slot is invoked when control returns to the event loop of the receiver's thread. The slot is executed in the receiver's thread.

It seems slot2 is invoked when slot1's message box still doing modal, in main thread, but at that time slot1 has lock mutex, so deadlock.

How to change the code to avoid deadlock?

Update:(Jan.17, 2019)

What I want archive is that: slot2 not be execute before slot1 finished.

What should be kept are:

1. worker is a background thread to process data, cost long time; so, whatever, the three signals will emit from other thread.
2. worker should not blocked by emitting signals.
3. slots should execute in main thread, because they will update GUI.
4. someOperation is not reentrant.

Answer 1

That's because your function void someOperation() is not reentrant.

The static functions of QMessageBox span their own event loop, which calls QCoreApplication::processEvents() repeatedly:

1. Execution of the first invocation of someOperation() gets stuck at QMessageBox::warning(...).
2. In there, exec() calls processEvents(), 3. which sees the second signal
3. and invokes someOperation() again
4. where trying to re-lock mutex fails.

How to resolve this depends on what you want to achieve...

---

About your general approach to QThread: You're doing it wrong.
(That link gives a good start into the topic, but not a complete solution.)

You create and start a background thread. But that thread will only emit the three signals and then finish.

The slots will be called inside the main (GUI) event loop, because that's the thread affinity of your A *a.

To make the slots be executed in the background, you need to:

1. create your A instance without a parent: A *a = new A();
2. create your Worker instance with the app as parent: Worker *w = new Worker(&app); (or with nothing, at least not with a)
3. change the thread affinity of your A instance: a->moveToThread(Worker);
4. don't override Worker::run(), or if you really want to (see point 5), call the base implementation: QThread::run();
5. emit the signals from main (you can emit them from run(), but that's not necessary).

Answer 2

The requirement that "someOperation is not reentrant" is an odd one. What should happen if reentrancy is attempted? Given that someOperation can only be called from the main thread I can only see two options...

1. Block completely with mutex/barrier etc. as you have tried.
2. Block based on a recursion level counter and spin the event loop until that counter decrements to zero.

1) Will block the thread's event loop completely preventing the current message dialog from functioning correctly.

2) Will allow all message dialogs simultaneously rather then serialising them.

Rather than trying to make someOperation non-reentrant I think you need to make sure you use in a way that won't result in reentrancy.

One option might be to make use of a separate QObject derived class instance on its own QThread. Consider the following...

class signal_serialiser: public QObject {
  Q_OBJECT;
signals:
  void signal1();
  void signal2();
  void signal3();
};

If an instance of signal_serialiser is moved to its own thread it can act as a queue to buffer and forward the various signals if suitable connection types are used. In your code you currently have...

QObject::connect(w, SIGNAL(signal1()), a, SLOT(slot1()), Qt::QueuedConnection);
QObject::connect(w, SIGNAL(signal2()), a, SLOT(slot2()), Qt::QueuedConnection);
QObject::connect(w, SIGNAL(signal3()), a, SLOT(slot3()), Qt::QueuedConnection);

Change that to...

signal_serialiser signal_serialiser;
QObject::connect(w, SIGNAL(signal1()), &signal_serialiser, SIGNAL(signal1()));
QObject::connect(w, SIGNAL(signal2()), &signal_serialiser, SIGNAL(signal2()));
QObject::connect(w, SIGNAL(signal3()), &signal_serialiser, SIGNAL(signal3()));

/*
 * Note the use of Qt::BlockingQueuedConnection for the
 * signal_serialiser --> A connections.
 */
QObject::connect(&signal_serialiser, SIGNAL(signal1()), a, SLOT(slot1()), Qt::BlockingQueuedConnection);
QObject::connect(&signal_serialiser, SIGNAL(signal2()), a, SLOT(slot2()), Qt::BlockingQueuedConnection);
QObject::connect(&signal_serialiser, SIGNAL(signal3()), a, SLOT(slot3()), Qt::BlockingQueuedConnection);
QThread signal_serialiser_thread;
signal_serialiser.moveToThread(&signal_serialiser_thread);
signal_serialiser_thread.start();

I've only done basic testing but it appears to give the desired behaviour.