Qt QTcpSocket: How to prevent dead lock in readyRead signal?

Question

I need some assistance in Qt on Windows 7. It seems that Qt readyRead() signal is emited by an asynchronus procedure call which causes the code to be executed concurrent but in the same thread.

In my example I have a queue which should be accessed by DoRead() and in DoTimer() which is accessed by a lock. Entire operation is running in ui (main) thread. However sometimes as DoRead() is called a dead lock occurred. The code stops execution in DoRead(). The dead lock is reproduceable if the Message Box is showed and so execution of DoTimer() is halted. However I was surprised to see that OnRead() is still called in concurrent. The only explanation for me is, that OnRead() is called by an Windows APC.

See MSDN article Asynchronus Procedure Calls:

An asynchronous procedure call (APC) is a function that executes asynchronously in the context of a particular thread. When an APC is queued to a thread, the system issues a software interrupt. The next time the thread is scheduled, it will run the APC function.

Am I true with my assumption that readyRead() could be an APC?

In either case, what could I do to prevent dead locks? I need to access the queue in DoRead() to fill the queue and in DoTimer() (and other methods of course) to read, write or delete entries from same queue. Recursive mutexes are no solution since both calls occurs in same thread.

class QMySocket : public QTcpSocket {
public:
    QMySocket() {
        ...
        connect(this, SIGNAL(readyRead()), this, SLOT(DoRead()));
        connect(_MyTimer, SIGNAL(timeout()), this, SLOT(DoTimer()));
        ...
    }
private:
    QTimer* _MyTimer;
    QQueue<int> _MyQueue;
    QMutex _Lock;

    void DoRead() {
        _Lock.lock(); // <-- Dead Lock here (same Thread ID as in DoTimer)
        _MyQueue... // Do some queue operation
        // DoSomething
        _Lock.unlock();
    }

    void DoTimer() {
        _Lock.lock();
        QQueue<int>::iterator i = _MyQueue.begin();
        while (i != _MyQueue.end()) { // Begin queue operation
            if (Condition) {
                QMessageBox::critical(...);
                i = _MyQueue.erase(i);
            } else {
                i++;
            }
        } // end queue operation
        _Lock.unlock();
    }
};

Edit 2: This had nothing to do with APC as I found out. The problem was only the extra message loop created by QMessageBox.

Instead calling QMessageBox directly, all messages will be queued and showed after any queue operation.

void DoTimer() {
    QList<QString> Messages;
    QQueue<int>::iterator i = _MyQueue.begin();
    while (i != _MyQueue.end()) { // Begin queue operation
        if (Condition) {
            Messages.append(...);
            i = _MyQueue.erase(i);
        } else {
            i++;
        }
    } // end queue operation
    QMessageBox::critical(Messages);
}

Locks are not required if there is no concurrent access to the queue (no multithreading).

Answer 1

Your only problem is the call to

QMessageBox::critical(...);

This call blocks until you press a button. But since you called it while still holding the lock, your DoRead deadlocks.

There is absolutely no reason to open a messagebox while holding that lock!

If you still want your DoTimer to respond while showing the messagebox don't use the static convenience methods like QMessagebox::critical.

Better do this

   // Somewhere in the constructor ...
   QMessageBox* msgBox = new QMessageBox( this );
   msgBox->setAttribute( QWidget::WA_DeleteOnClose );
   msgBox->setStandardButtons( QMessageBox::Ok );
   msgBox->setWindowTitle( tr("Error") );
   msgBox->setModal( true );
   //...

void DoTimer() {
    _Lock.lock();
    // DoSomething
    _MyQueue... // Iterate over queue, and do some queue operation (delete entires for exmaple)
    _Lock.unlock();
    msgBox->setText( tr("DingDong!") );
    if (!msgBox->isVisible())
        msgBox->open( this, SLOT(msgBoxClosed(QAbstractButton*)) );
}

void MyWidget::msgBoxClosed(QAbstractButton*) {
   qDebug("Byebye msgbox");
}

But still, from your code I don't see any reason to use mutexes anyway. There is no concurrency, right?