Stopping and deleting background QThread and its worker

Adam Badura

Qt5

Yet it does not happen. I both added qDebug (in my UDPListener run on the thread) and set a breakpoint in destructors (both UDPListener and QThread).

mcosta

Hi Adam,

if you want stop your UDPListener, you have to write it in such manner.
Please post your code.

Adam Badura

Bellow is the simplified version of the code which still contains the important aspects and shows the issues.

@#include <QtCore/QByteArray>
#include <QtCore/QDebug>
#include <QtCore/QObject>
#include <QtCore/QThread>
#include <QtCore/QtGlobal>
#include <QtNetwork/QHostAddress>
#include <QtNetwork/QNetworkInterface>
#include <QtNetwork/QUdpSocket>
#include <QtWidgets/QApplication>
#include <QtWidgets/QMainWindow>

// Required to use QHostAddress as parameter in signals.
Q_DECLARE_METATYPE( QHostAddress )

////////////////////////////////////////////////////////////////////////////////

class UDPListener
: public QObject
{
Q_OBJECT

public:
UDPListener( QObject* parent = nullptr )
: QObject( parent ),
theSocketPtr( new QUdpSocket( this ) )
{
// Required to use QHostAddress as parameter in signals.
qRegisterMetaType< QHostAddress >();

if (
  !theSocketPtr->bind(
    QHostAddress::AnyIPv4,
    21212,
    QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint
  )
)
{
  Q_ASSERT( false );
}

Q_ASSERT( theSocketPtr->state() == QAbstractSocket::BoundState );

if ( !theSocketPtr->joinMulticastGroup( QHostAddress( "224.0.0.66" ) ) )
{
  Q_ASSERT( false );
}

}

virtual ~UDPListener()
{
qDebug() << "UDPListener::~UDPListener()";
}

public slots:
void run()
{
connect(
theSocketPtr,
&QUdpSocket::readyRead,
this,
&UDPListener::processDatagrams
);

processDatagrams();

}

private slots:
void processDatagram()
{
QByteArray datagram;
QHostAddress senderAddres;
quint16 senderPort;

datagram.resize( theSocketPtr->pendingDatagramSize() );
theSocketPtr->readDatagram(
  datagram.data(),
  datagram.size(),
  &senderAddres,
  &senderPort
);

emit datagramReceived( datagram, senderAddres, senderPort );

}

void processDatagrams()
{
while ( theSocketPtr->hasPendingDatagrams() )
processDatagram();
}

signals:
void datagramReceived(
const QByteArray& aDatagram,
const QHostAddress& aSenderAddress,
quint16 aSenderPort
);

private:
QUdpSocket* theSocketPtr;

};

////////////////////////////////////////////////////////////////////////////////

class NetworkWatcher
: public QObject
{
Q_OBJECT

public:
NetworkWatcher( QObject* parent = nullptr )
: QObject( parent ),
theUDPListenerThreadPtr(),
theUDPListenerPtr()
{
}

virtual ~NetworkWatcher()
{
qDebug() << "NetworkWatcher::~NetworkWatcher()";

theUDPListenerThreadPtr->quit();

}

void runOnNewThread()
{
theUDPListenerPtr = new UDPListener();
connect(
theUDPListenerPtr,
&UDPListener::datagramReceived,
this,
&NetworkWatcher::onDatagramReceived
);

theUDPListenerThreadPtr = new QThread();
theUDPListenerThreadPtr->setObjectName( "NetworkWatcher::UDPListener" );

connect(
  theUDPListenerThreadPtr,
  &QThread::started,
  theUDPListenerPtr,
  &UDPListener::run
);
connect(
  theUDPListenerThreadPtr,
  &QThread::finished,
  theUDPListenerPtr,
  &UDPListener::deleteLater
);
connect(
  theUDPListenerThreadPtr,
  &QThread::finished,
  theUDPListenerThreadPtr,
  &QThread::deleteLater
);

theUDPListenerPtr->moveToThread( theUDPListenerThreadPtr );
theUDPListenerThreadPtr->start();

}

protected slots:
void onDatagramReceived(
const QByteArray& aDatagram,
const QHostAddress& aSenderAddress,
quint16 aSenderPort
)
{
/* ... */
}

private:
QThread* theUDPListenerThreadPtr;

UDPListener* theUDPListenerPtr;

};

////////////////////////////////////////////////////////////////////////////////

class Application
: public QApplication
{
Q_OBJECT

public:
Application( int& argc, char** argv )
: QApplication( argc, argv ),
theNetworkWatcherPtr( new NetworkWatcher( this ) )
{
theNetworkWatcherPtr->runOnNewThread();
}

virtual ~Application()
{
qDebug() << "Application::~Application()";
}

private:
NetworkWatcher* theNetworkWatcherPtr;

};

////////////////////////////////////////////////////////////////////////////////

int main( int argc, char* argv[] )
{
Application application( argc, argv );

QMainWindow mainWindow;
mainWindow.show();

return application.exec();
}@

Running the code shows that QThread (NetworkWatchar::theUDPListenerThreadPtr) and UDPListener (NetworkWatcher::theUDPListenerPtr) are never destroyed (nor their memory is deallocated).

mcosta

Simplified code?
I think it's a little bit complicated.

What means object never destroyed? Destuctor are never called?

I see that in your code you never call QThread::quit(); you should do.
I suggest to launch the Thread in MainWindow constructor or similar and call QThread::quit() on destructor or in "closeEvent"

KA51O

I would add one more connect statement and add a slot on your UDPListener to stop him working.
@
void runOnNewThread()
{
.
.
.
connect(
theUDPListenerPtr,
&UDPListener::finished,
theUDPListenerThreadPtr ,
&QThread::quit
);
.
.
.
@

In this slot stop() (or whatever) you can set a boolean flag to true, and then change the function UDPListener::processDatagrams to look like this:
@
void UDPListener::stop()
{
m_bStopWorking = true;
}

void UDPListener::processDatagrams()
{
while(theSocketPtr->hasPendingDatagrams())
{
if(!m_bStopWorking)
{
processDatagram();
}
else
{
emit finished();
}
}
}
@

The while loop might be a problem though, because as long as you're in that loop control will not return to the event queue and therefor the stop() slot will not be called. I'm not sure how long your object usually spends in that loop. If it spends a lot of time in that loop you might have to think of a way to replace that loop by a signal slot connection. Maybe by connecting your datagramReceived(...) signal to your processDatagrams() slot?

Adam Badura

[quote author="mcosta" date="1364427839"]What means object never destroyed? Destuctor are never called?[/quote]

Exactly. Destructors for UDPListener and QThread are never called.

[quote author="mcosta" date="1364427839"]I see that in your code you never call QThread::quit(); you should do.[/quote]

It does call QThread::quit. In NetworkWatcher destructor (invoked while destroying Application due to parent-child relationship). QThread::finished is emitted (checkable in debugger). And the thread does quit (seen in debugger). But connected deleteLater are not called!

I suspect it is because the events loop of main thread and the QThread are already shutdown. But I might be wrong.

[quote author="mcosta" date="1364427839"]I suggest to launch the Thread in MainWindow constructor or similar and call QThread::quit() on destructor or in "closeEvent"[/quote]

Calling QThread::quit from QMainWindow::closeEvent should help (if my above guess on the reasons of issues) with destroying and deleting QThread which is owned by the main thread. However it would not help with UDPListener since same conditions still exist for it. But it doesn't go well with the design since NetworkWatcher is bound to application, not some GUI component...

I will check this (to confirm that UDPListener will still not be destroyed) in few days since I'm on holidays now.

mcosta

Hi,

you MUST call QThread::quit in your code and conenct finished() SIGNAL to stop()

Adam Badura

[quote author="KA51O" date="1364456687"]I would add one more connect statement and add a slot on your UDPListener to stop him working.[/quote]

Interesting. But who is to call it and when? Or should I call that function instead of calling quit in the NetworkWatcher destructor? That might actually work. But I will be able to check this in few days since I'm on holidays now.

[quote author="KA51O" date="1364456687"]The while loop might be a problem though, because as long as you're in that loop control will not return to the event queue and therefor the stop() slot will not be called. I'm not sure how long your object usually spends in that loop. If it spends a lot of time in that loop you might have to think of a way to replace that loop by a signal slot connection. Maybe by connecting your datagramReceived(...) signal to your processDatagrams() slot?[/quote]

Yeah... I don't like that loop at all. "The other thread":http://qt-project.org/forums/viewthread/26147/#119546 is all about that loop and getting rid of it. But I think that in my actual case this will likely not be a real issue since the datagrams are not frequent enough. (At least in developer's environment, not sure how about client's environment.)

KA51O

[quote author="Adam Badura" date="1364460931"][quote author="KA51O" date="1364456687"]I would add one more connect statement and add a slot on your UDPListener to stop him working.[/quote]

Interesting. But who is to call it and when? Or should I call that function instead of calling quit in the NetworkWatcher destructor? That might actually work. But I will be able to check this in few days since I'm on holidays now.

[quote author="KA51O" date="1364456687"]The while loop might be a problem though, because as long as you're in that loop control will not return to the event queue and therefor the stop() slot will not be called. I'm not sure how long your object usually spends in that loop. If it spends a lot of time in that loop you might have to think of a way to replace that loop by a signal slot connection. Maybe by connecting your datagramReceived(...) signal to your processDatagrams() slot?[/quote]

Yeah... I don't like that loop at all. "The other thread":http://qt-project.org/forums/viewthread/26147/#119546 is all about that loop and getting rid of it. But I think that in my actual case this will likely not be a real issue since the datagrams are not frequent enough. (At least in developer's environment, not sure how about client's environment.)[/quote]

The stop() slot can be called from whereever you want, thats the benefit of sticking to signal slot connections when using QThread. I guess in your case the NetworkWatcher destructor is a good place, but you cannot call the slot directly. Either emit a signal that is connected to the stop slot or use the QMetaObject::invokeMethod() method.

The whole loop thing can be a problem in QThreads, because they can block the event loop. And if you rely on the event loop (as for example with your stop() slot) you should avoid busy loops. I usually try to solve the whole loop problem by using a sort of signal slot loop with boolean flags for stop, pause or whatever. This way you return to the event loop after every iteration of your "loop" and thus allow other signals from outside to be handled.

Adam Badura

Along your suggestion I change the code to following:
@#include <QtCore/QByteArray>
#include <QtCore/QDebug>
#include <QtCore/QObject>
#include <QtCore/QThread>
#include <QtCore/QtGlobal>
#include <QtNetwork/QHostAddress>
#include <QtNetwork/QNetworkInterface>
#include <QtNetwork/QUdpSocket>
#include <QtWidgets/QApplication>
#include <QtWidgets/QMainWindow>

// Required to use QHostAddress as parameter in signals.
Q_DECLARE_METATYPE( QHostAddress )

////////////////////////////////////////////////////////////////////////////////

class UDPListener
: public QObject
{
Q_OBJECT

public:
UDPListener( QObject* parent = nullptr )
: QObject( parent ),
theSocketPtr( new QUdpSocket( this ) ),
theShouldStop( false ),
theFinishedEmitted( false )
{
// Required to use QHostAddress as parameter in signals.
qRegisterMetaType< QHostAddress >();

if (
  !theSocketPtr->bind(
    QHostAddress::AnyIPv4,
    21212,
    QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint
  )
)
{
  Q_ASSERT( false );
}

Q_ASSERT( theSocketPtr->state() == QAbstractSocket::BoundState );

if ( !theSocketPtr->joinMulticastGroup( QHostAddress( "224.0.0.66" ) ) )
{
  Q_ASSERT( false );
}

}

virtual ~UDPListener()
{
qDebug() << "UDPListener::~UDPListener()";
}

public slots:
void run()
{
connect(
theSocketPtr,
&QUdpSocket::readyRead,
this,
&UDPListener::processDatagrams
);

processDatagrams();

}

void stop()
{
theShouldStop = true;
}

private slots:
void processDatagram()
{
QByteArray datagram;
QHostAddress senderAddres;
quint16 senderPort;

datagram.resize( theSocketPtr->pendingDatagramSize() );
theSocketPtr->readDatagram(
  datagram.data(),
  datagram.size(),
  &senderAddres,
  &senderPort
);

emit datagramReceived( datagram, senderAddres, senderPort );

}

void processDatagrams()
{
if ( theFinishedEmitted )
return;

while ( theSocketPtr->hasPendingDatagrams() && !theShouldStop )
  processDatagram();

if ( theShouldStop )
{
  emit finished();
  theFinishedEmitted = true;
}

}

signals:
void datagramReceived(
const QByteArray& aDatagram,
const QHostAddress& aSenderAddress,
quint16 aSenderPort
);

void finished();

private:
QUdpSocket* theSocketPtr;

bool theShouldStop;

bool theFinishedEmitted;

};

////////////////////////////////////////////////////////////////////////////////

class NetworkWatcher
: public QObject
{
Q_OBJECT

public:
NetworkWatcher( QObject* parent = nullptr )
: QObject( parent ),
theUDPListenerThreadPtr(),
theUDPListenerPtr()
{
}

virtual ~NetworkWatcher()
{
qDebug() << "NetworkWatcher::~NetworkWatcher()";

QMetaObject::invokeMethod(
  theUDPListenerPtr,
  "stop",
  Qt::QueuedConnection
);
//theUDPListenerThreadPtr->quit();

}

void runOnNewThread()
{
theUDPListenerPtr = new UDPListener();
connect(
theUDPListenerPtr,
&UDPListener::datagramReceived,
this,
&NetworkWatcher::onDatagramReceived
);

theUDPListenerThreadPtr = new QThread();
theUDPListenerThreadPtr->setObjectName( "NetworkWatcher::UDPListener" );

connect(
  theUDPListenerPtr,
  &UDPListener::finished,
  theUDPListenerThreadPtr,
  &QThread::quit
);

connect(
  theUDPListenerThreadPtr,
  &QThread::started,
  theUDPListenerPtr,
  &UDPListener::run
);
connect(
  theUDPListenerThreadPtr,
  &QThread::finished,
  theUDPListenerPtr,
  &UDPListener::deleteLater
);
connect(
  theUDPListenerThreadPtr,
  &QThread::finished,
  theUDPListenerThreadPtr,
  &QThread::deleteLater
);

theUDPListenerPtr->moveToThread( theUDPListenerThreadPtr );
theUDPListenerThreadPtr->start();

}

protected slots:
void onDatagramReceived(
const QByteArray& aDatagram,
const QHostAddress& aSenderAddress,
quint16 aSenderPort
)
{
/* ... */
}

private:
QThread* theUDPListenerThreadPtr;

UDPListener* theUDPListenerPtr;

};

////////////////////////////////////////////////////////////////////////////////

class Application
: public QApplication
{
Q_OBJECT

public:
Application( int& argc, char** argv )
: QApplication( argc, argv ),
theNetworkWatcherPtr( new NetworkWatcher( this ) )
{
theNetworkWatcherPtr->runOnNewThread();
}

virtual ~Application()
{
qDebug() << "Application::~Application()";
}

private:
NetworkWatcher* theNetworkWatcherPtr;

};

////////////////////////////////////////////////////////////////////////////////

int main( int argc, char* argv[] )
{
Application application( argc, argv );

QMainWindow mainWindow;
mainWindow.show();

return application.exec();
}@
But still UDPListener destructor is not called.

JKSH

Your code relies on this order of calling/signalling:

Application::~Application() calls NetworkWatcher::~NetworkWatcher() via parent-child relationship

NetworkWatcher::~NetworkWatcher() invokes UDPListener::stop() [queued]

UDPListener::stop() sets theShouldStop = true

UDPListener::processDatagrams() checks theShouldStop, emits UDPListener::finished()

UDPListener::finished() invokes QThread::quit() [queued]

QThread::quit() emits QThread::finished()

QThread::finished() invokes UDPListener::deleteLater() and QThread::deleteLater() [queued]

The problem is, you've already terminated the main event loop in step #1. Thus, step #5 cannot happen; theUDPListenerThreadPtr lives in the main thread, so its slots can no longer be invoked after Application::~Application() returns.

Solution: Do not use Application::~Application() to trigger cleanup. Instead, the application should only be allowed to quit AFTER all cleanup has been completed -- e.g. connect QThread::deleted() to QApplication::quit(), and then clean up everything when the main window is closed.

Also, there's no need to subclass QApplication -- just make the NetworkWatcher a local variable/object in main().