Plugins threading model does not conform to Qt standard

[srcejon]

When building with the debug version of Qt libraries, assertions are enabled. It seems we get an assertion when several Features are closed. For example, AFC.

The assertions are of the form:

2022-07-18 17:20:10.316 (F) ASSERT failure in QCoreApplication::sendEvent: "Cannot send events to objects owned by a different thread. Current thread 0x0x21435329da0. Receiver '' (of type 'AFCWorker') was created in thread 0x0x21444945170", file kernel\qcoreapplication.cpp, line 558

This appears to be due to worker objects being deleted in the destructor of the Feature's main class:

AFC::~AFC()
....
delete m_worker;

and that the worker objects are moved to a different thread in the constructor:

AFC::AFC
...
m_worker = new AFCWorker(webAPIAdapterInterface);
m_worker->moveToThread(&m_thread);

Note that we can't just do:

m_worker->deleteLater();

As at this point, m_thread has already been stopped, so the worker's destructor never gets called.

We can't also do:

m_worker->moveToThread(QThread::currentThread());
delete m_worker;

As you can only push an object to a thread, not pull it (i.e. moveToThread needs to be executed in the content of m_thread)

The recommended approach https://wiki.qt.io/QThreads_general_usage seems to be connect &Worker::finished to &Worker::deleteLater and &QThread::finished to &QThread::deleteLater - however, that doesn't work here as SDRangel workers can be restarted.

One thing that seems to work is starting the thread then calling deleteLater()

m_thread.start();
m_worker->deleteLater();
m_thread.quit();
m_thread.wait();

As this results in the worker's destructor being called.

Any better ideas?

[f4exb]

It is a bit strange to restart the thread just for this purpose. Instead the destructor code may be rewritten this way (this is the APRS feature because I had this one in my config):

--- a/plugins/feature/aprs/aprs.cpp
+++ b/plugins/feature/aprs/aprs.cpp
@@ -83,11 +83,14 @@ APRS::~APRS()
         &APRS::networkManagerFinished
     );
     delete m_networkManager;
+
     if (m_worker->isRunning()) {
-        stop();
+        m_worker->stopWork();
     }
 
-    delete m_worker;
+    m_worker->deleteLater();
+    m_thread.quit();
+    m_thread.wait();
 }
 
 void APRS::start()

It seems there are no issues at exit time with this.

[srcejon]

It seems there are no issues at exit time with this.

The worker destructor doesn't get called though (if the thread isn't running when ~APRS is called, which can be the case if the i-gate isn't enabled in APRS - or if the stop button has been pressed in other features).

[f4exb]

OK the bad design starts with the worker being created in the class constructor but I will not change that due to side effects in the present code. So it odes not matter if the design is made only slightly worse.

[f4exb]

Or maybe when in Rome do as the Romans do... i.e. create/delete thread and worker in the start/stop cycle.

[srcejon]

None of the workers seem to do much in the constructors, so that doesn't seem problematic - just not 100% if there isn't any state that might be preserved.

[f4exb]

Some interesting stuff here: https://www.toptal.com/qt/qt-multithreading-c-plus-plus

[f4exb]

Will not do with the DATVDemodulator as this is too much of a mess.

[f4exb]

Will have to skip FileInput and SigMFFileInput because that would be too complex to change.

[f4exb]

Will not do with the ChannelAnalyzer, ATVDemod, BFMDemod, FreeDVDemod, UDPSink as they are too much of a mess.