QThread, how to destruct it from a slot running in its event loop?

mbruel

HI,
I'm pretty new to QT and starting using it to write a multi threaded TCP proxy.
Basically, as the QTcpSocket works with the event loop, I've followed the Worker/Controller scheme. (http://doc.qt.io/qt-5/qthread.html#details)
However my case is a bit different and I'm moving the Controller inside the thread it is holding. Is that bad practice? It sounds like...
Here is my code:

InputThread::InputThread(qintptr aSocketDescriptor, InputManager & aInputMgr, QObject *parent):
    QObject(parent), iThread(), iSocketDescriptor(aSocketDescriptor),
    iInputCon(Q_NULLPTR), iInputMgr(aInputMgr)
{
    this->moveToThread(&iThread);

    iInputCon = new InputConnection(iSocketDescriptor);
    iInputCon->moveToThread(&iThread); // move the slots of iInputCon in the Thread event loop

    connect(this, &InputThread::startConnection, iInputCon, &Connection::startTcpConnection);
    connect(iInputCon, &InputConnection::closed, this, &InputThread::closeConnection);
    connect(iInputCon, &Connection::socketError, this, &InputThread::handleSocketError);

    connect(iInputCon, &InputConnection::connected, &InputConnection::doAuthentication);
    connect(iInputCon, &InputConnection::authenticated, this, &InputThread::inputAuthenticated);

    iThread.start(); // starting event loop of the thread
}

InputThread::~InputThread(){
    iThread.quit();
    iThread.wait();
    delete iInputCon;
}

void InputThread::closeConnection(){
    log("Closing InputThread");
    iInputMgr.delInputThread(this);
}

basically in the slot of the controller InputThread::closeConnection I will delete the Controller but it will be running inside the thread as I moved it in it....
I'm getting this error message on deletion:

QThread::wait: Thread tried to wait on itself
QThread: Destroyed while thread is still running
QWaitCondition::wakeAll(): mutex lock failure: Invalid argument

It was working fine when I didn't moved the controller inside its own QThread object.
But I kind of need to have also some slots from the controller that will run inside the thread, for example InputThread::inputAuthenticated that will check in a DB if the authentication is good or not, create an outputConnection and put it in relation with the input. I want all those things to be done in the thread so I'll push the objects in it and use signal/slots to interact.

My problem is how to kill the thread properly in that case? Shall I just use a pointer and don't destruct it using the Object::deleteLater slot? I'm not familiar with it and I just want to be sure it will be destroyed...

kshegunov

You should also get an error message that QObjects with parent set can't be moved to another thread, right? So this->moveToThread(&iThread); does nothing and your InputThread object is left to its old thread?

If that's not the case, your signals are then invoked from the new thread, and you have to return the execution so the thread can finish. You can't wait on a thread from that same thread, it doesn't make any sense.

mbruel

I don't use any parents for the creation of my InputThread so it works well, I can see the thread id of its slots is the one of the thread.

I know it doesn't make sense to wait the thread within the thread. I had this code before adding my InputThread inside the thread.

I've tried now something like this:

InputThread::InputThread(qintptr aSocketDescriptor, InputManager & aInputMgr, QObject *parent):
    QObject(parent), iThread(), iSocketDescriptor(aSocketDescriptor),
    iInputCon(Q_NULLPTR), iInputMgr(aInputMgr)
{
    .
    .
    connect(&iThread, &QThread::finished, this, &QObject::deleteLater);
    connect(&iThread, &QThread::finished, &iThread, &QObject::deleteLater);
}

InputThread::~InputThread(){
    delete iInputCon;
}

void InputThread::closeConnection(){
    log("Closing InputThread");
    iInputMgr.delInputThread(this);
    iThread.quit();
}

So basically not deleting the InputThread manually but getting it deleted at the end of the Thread.
However I still get this error:

QThread: Destroyed while thread is still running
QWaitCondition::wakeAll(): mutex lock failure: Invalid argument

So I guess the problem is that I call iThread.quit() within a slot running in the event queue of the iThread and this seems to be an issue... I suppose it is because when the quit return I'm still in a slot running in the thread.

I was thinking maybe to send a signal that would be connected to quit but I'm getting a compilation error when I try to this:

InputThread::InputThread(qintptr aSocketDescriptor, InputManager & aInputMgr, QObject *parent):
    QObject(parent), iThread(), iSocketDescriptor(aSocketDescriptor),
    iInputCon(Q_NULLPTR), iInputMgr(aInputMgr)
{
    .
    .
    connect(this, &InputThread::stopThreadEventLoop, &iThread, QThread::quit);
}

Here is the error:

 error: invalid use of non-static member function 'void QThread::quit()'
     connect(this, &InputThread::stopThreadEventLoop, &iThread, QThread::quit);
                                                                         ^

So is there a way to stop the event loop of my thread from a slot running inside the thread?

Otherwise how could I redesign my InputThread?
Do I have to use a wrapper object InputController that I'll push in the Thread (hold by InputThread) and have the logic in that class to make the process and communication between an InputConnectio, OutputConnection and DatabaseConnection?
It just seems a bit redundant...
Would it be another way?

kshegunov

@mbruel
Hello,
Yes, the problem is that the thread is still running when you delete the QThread object. Quit actually just posts the quit message to the event queue and then returns. The proper way to do this is to hold the QThread instances in another thread (usually the main one). Do not forget that QThread is not a real thread, but a QObject for controlling threads, so to have everything running properly you'd have to wait for the thread to finish, and only then destroy the QThread object. Why would you want to hold iThread inside the InputThread class anyway?

Kind regards.

mbruel

@kshegunov
Thanks for your quick replies.
I understood that the QThread is not the actual Thread but just an object holding it.
My design is like this:
my main class NntpProxy inherit from QTcpServer

void NntpProxy::incomingConnection(qintptr aSocketDescriptor){
    // Every new connection will run (its slots) in a new thread
    InputThread *thread = iInputMgr->newInputThread(aSocketDescriptor);

    emit thread->startConnection(); // starting the connection inside the new thread
}

So basically, my InputManager class hold a QList of InputThread.
I was hoping that InputThread could hold the QThread handle in order to be able to stop its event loop when I'm done with my InputConnection (the QTcpSocket being closed)

If I let the QThread in NntpProxy, I suppose I'll have to hold there a QList of QThread that would be the same size than my QList<InputThread> in InputManager.
Moreover, I'm not sure how I could know in NntpProxy which is the QThread corresponding to the InputThread that needs to be waited and closed.
Do you see the problem? I'm not sure I'm really clear...
And I find that holding 2 QList is a waste anyway...

Just to be clear, I'm not deleting my InputThread anymore, just using a connect to do it:

connect(&iThread, &QThread::finished, this, &QObject::deleteLater);

So I would find really convenient to be able to emit a signal to InputThread that would be queued and stop the event queue of the thread. The issue is that the emission is done from a slot running within the thread... I turning in round... And I don't understand why I'm not able to connect a signal to QThread::quit (see the error message in my previous response)

Do you see any solution? The only thing I can think of in order to hold only 1 QList of the threads would be to still hold the QThread in InputThread but not moving it into the thread it is holding so it would be able to wait it and close it.
Instead use a "redundant" object InputController that will be moved into the the thread and hold all the interesting objects: InputConnection, OutputConnection, DatabaseConnection and also the logic to keep track of the states, create them and make them communicate. All these Connection object would also be moved into the QThread...

What do you think about this solution? Any better idea?

mbruel

Alright I've found a "tricky" solution that seems to work. Here it is, please let me know what you think about it and if I should rather redesign the architecture to not moveToThread the InputThread that holds the instance of the QThread.

void NntpProxy::incomingConnection(qintptr aSocketDescriptor){
    InputThread *thread = iInputMgr->newInputThread(aSocketDescriptor, this);
    emit thread->startConnection(); // starting the connection inside the new thread
}

void NntpProxy::stopThread(InputThread *aInputThread){
    aInputThread->stopThread();
    delete aInputThread;
}

InputThread::InputThread(qintptr aSocketDescriptor, InputManager & aInputMgr,
                         NntpProxy *aProxy, QObject *parent):
    QObject(parent), iThread(), iSocketDescriptor(aSocketDescriptor),
    iInputCon(Q_NULLPTR), iInputMgr(aInputMgr), iProxy(aProxy)
{
    this->moveToThread(&iThread);

    iInputCon = new InputConnection(iSocketDescriptor);
    iInputCon->moveToThread(&iThread); // move the slots of iInputCon in the Thread event loop

    connect(this, &InputThread::startConnection, iInputCon, &Connection::startTcpConnection);
    connect(iInputCon, &InputConnection::closed, this, &InputThread::closeConnection);
    connect(iInputCon, &Connection::socketError, this, &InputThread::handleSocketError);

//    connect(iInputCon, &InputConnection::connected, &InputConnection::doAuthentication);
    connect(iInputCon, &InputConnection::authenticated, this, &InputThread::inputAuthenticated);

    // signal to stop the event queue of the thread
    connect(this, &InputThread::stopThreadEventLoop, iProxy, &NntpProxy::stopThread, Qt::QueuedConnection);

    iThread.start(); // starting event loop of the thread
}

void InputThread::stopThread(){
    log("InputThread::stopThread");
    iThread.wait(1000);
    iThread.quit();
}

InputThread::~InputThread(){
    delete iInputCon;
}

void InputThread::closeConnection(){
    iInputMgr.delInputThread(this);
    emit stopThreadEventLoop(this);
}

Basically on a QTcpSocket close, I send a QueuedConnection signal to my NntpProxy that is in the main thread and from there call InputThread::stopThread that is a public method not a slot and will try to wait for the thread to finish before closing it. \o/

Here are my logs:

[2015/12/11 22:25:40] [Thread 0xb4efc940] [NntpProxy] New incoming connection: 6
[2015/12/11 22:25:40] [Thread 0xb4efc940] InputThread[6] Constructor
[2015/12/11 22:25:40] [Thread 0xb4efc940] Connection[6] Constructor ssl:1, serverSocket: 1
[2015/12/11 22:25:40] [Thread 0xb4efc940] InputConnection[6] Constructor
[2015/12/11 22:25:40] [Thread 0xb4efc940] [InputManager] Connection Created: id=6, iInputs.size()=1
[2015/12/11 22:25:40] [Thread 0xb4d33b40] InputConnection[6] Starting connection...
[2015/12/11 22:25:40] [Thread 0xb4d33b40] InputConnection[6] SSL socket
[2015/12/11 22:25:40] [Thread 0xb4d33b40] InputConnection[6] Encryption handshake done
[2015/12/11 22:25:40] [Thread 0xb4d33b40] InputConnection[6] Client Connected
[2015/12/11 22:25:40] [Thread 0xb4d33b40] InputConnection[6] doAuthentication
[2015/12/11 22:25:40] [Thread 0xb4d33b40] InputConnection[6] Encrypted Socket
[2015/12/11 22:25:47] [Thread 0xb4d33b40] InputThread[6] TCP authentication done, user: bob

[2015/12/11 22:25:54] [Thread 0xb4d33b40] InputConnection[6] Data In: quit

[2015/12/11 22:25:54] [Thread 0xb4d33b40] InputConnection[6] Disconnected
[2015/12/11 22:25:54] [Thread 0xb4d33b40] InputThread[6] Closing InputThread
[2015/12/11 22:25:54] [Thread 0xb4d33b40] [InputManager] Connection Deleted: id=6, res: 1, iInputs.size()=0
[2015/12/11 22:25:54] [Thread 0xb4efc940] [NntpProxy] Closing thread: 6
[2015/12/11 22:25:54] [Thread 0xb4efc940] InputThread[6] InputThread::stopThread
[2015/12/11 22:25:55] [Thread 0xb4efc940] InputThread[6] Destructor
[2015/12/11 22:25:55] [Thread 0xb4efc940] InputConnection[6] Destructor
[2015/12/11 22:25:55] [Thread 0xb4efc940] Connection[6] Destructor (deleting iSocket)

kshegunov

@mbruel
Hello,
You could use that if it works, but consider the following example:

NntpProxy::incomingConnection(qintptr socketDescriptor)
{
    QThread * thread = new QThread();
    thread->start();
    // This object would be where your socket is created and its signals handled
    SocketHandler * handler = new SocketHandler();
    handler->moveToThread(thread);

    // When the handler is destroyed, signal the thread to quit
    QObject::connect(handler, SIGNAL(aboutToQuit()), thread, SLOT(quit()));
    // When the thread has finished, schedule it deleted
    QObject::connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));

    // This can be done with a signal-slot connection but at this point it's not needed
    // setupSocket() is a slot and will be invoked in the handler's thread
    QMetaObject::invokeMethod(handler, "setupSocket", Q_ARG(qintptr, socketDescriptor));
}

class SocketHandler : public QObject
{
    Q_OBJECT
public:
    // Some slots and functions and the like
signals:
    void aboutToQuit();
private slots:
    void setupSocket(qintptr socketDescriptor);
private:
    QTcpSocket * socket;
}

void SocketHandler::setupSocket(qintptr socketDescriptor)
{
    // This is called in the worker thread
    // Here you setup your socket and connect the signals from it to the current object
    socket = new QTcpSocket();
    socket->setSocketDescriptor(socketDescriptor);
    // ... more socket initialization if needed

    // When the socket disconnects, schedule the handler object to be deleted
    QObject::connect(socket, SIGNAL(disconnected()), this, SLOT(deleteLater()));
    // ... connect other signals from the socket to be handled in this object as you see fit 
}

SocketHandler::~SocketHandler()
{
     delete socket; // Free your memory

     emit aboutToQuit();  // Just signal anyone that's interested that the handler is being destroyed
}

I think this is a bit simpler and should help you set up your infrastructure. As a note, bear in mind that creating and destroying threads might not be much light-weight (especially on *nix platforms). Usually when one wants to have many operations threaded thread pools are used, to allow recycling the threads, instead of destroying them. Additionally, if you don't have a lengthy operation, the benefit of threading your sockets is marginal.

I hope that helps.
Kind regards.

mbruel

@kshegunov
Well thanks a lot! That is indeed much better! I'm really not quite used to the signal/slots mechanism, I didn't think we could just connect in this way the destruction of the Handler to the quit of the QThread. So nice! No need indeed to keep a list of the threads and wonder how to match them again as they will just be, just need to send a signal...
Connecting the QThread::finished to QObject::deleteLater is also really handy.
Just to have a full log and track memory leaks, I've defined MyThread class that inherit from QThread and only trace construction and deletion with the socketDescriptor.

About the Thread usage, I know it is costly to create them but that it is not an issue in my use case. The connections are supposed to last a long time (between few minutes to few hours) and be used a lot: each one transiting data at a rate superior to 10Mbps (potentially with double SSL ecryption/decryption on the fly)
So I think using a thread per connection is quite relevant.

Thanks again, and have a great weekend!

kshegunov

@mbruel said:
No problem.

I didn't think we could just connect in this way the destruction of the Handler to the quit of the QThread

Actually it's quite used in the Qt framework as QObject instances are often destroyed through signal-slot connections. Look up the QObject::destroyed signal if you're interested.

So I think using a thread per connection is quite relevant.

I'm not arguing that, after all I have no idea what your program is supposed to do, just making an observation.

Kind regards.

mbruel

I'm having a new problem when I move a second QTcpSocket inside the thread.
Everything seems to work fine but the thread is crashing (segmentation fault) after deletion (after receiving the destroyed signal). It seems to be an desallocation issue... I couldn't manage to figure out the problem yesterday and today...
I'm going to write a easier example of my architecture and open a new post.
Cheers