1

I am trying to analyse a segfault that seems to occur when accessing a heap allocated object created by a sender thread and accessed by a receiver thread.

Here is a short version of the code :

#include <QCoreApplication>
#include <QDebug>
#include <QThread>
#include <QTimer>

class Data
{
public:
    Data(int data1) : m_data1(data1) {}
    int data1() {
        return m_data1;
    }
private:
    int m_data1;
};

class Sender : public QObject
{
    Q_OBJECT

public:
    Sender(int timeout) : m_timeout(timeout) {}

public slots:
    void startSendingDatas() {
        QTimer::singleShot(m_timeout, [this]() {
            emit datas(new Data(3));
        });
    }

signals:
    void datas(Data *data);

private:
    int m_timeout;
};

class Receiver : public QObject
{
    Q_OBJECT

public slots:
    void onDatas(Data *data) {
        qDebug() << "data1 = " << data->data1();
        delete data; // is it always safe ?
    }
};

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);

    Sender sender(5000);
    Receiver receiver;

    QObject::connect(&sender, SIGNAL(datas(Data*)),
                     &receiver, SLOT(onDatas(Data*)));

    QThread worker;
    worker.start();

    sender.moveToThread(&worker);

    // now we call it asynchronously
    QMetaObject::invokeMethod(&sender, "startSendingDatas");

    return a.exec();
}

#include "main.moc"

Data does not inherits from QObject so deleteLater is not an option here but is it really safe to do that ?

Thank you.

Fryz
  • 2,119
  • 2
  • 25
  • 45
  • 2
    Doesn't look very safe to me. Also, unrelated, why are you using the old string based `SIGNAL` and `SLOT` macros in `QObject::connect(&sender, SIGNAL(datas(Data*)), &receiver, SLOT(onDatas(Data*)));`? You really should be using the new type-safe variants that will fail at compile time rather than silently do nothing at run-time when you mess up - they'll even result in faster code as a bonus. – Jesper Juhl Apr 28 '20 at 17:43
  • Do you see any issues regarding Qt's signal/slot mechanism by deleting an heap allocated object through a pointer that is passed as a signal/slot argument ? – Fryz Apr 28 '20 at 18:17
  • 2
    In the code shown I can't see any obvious issue. But... if `Data` was non-POD or the sender continued to access the memory associated with the sent pointer then there are obviously going to be problems. As always with multi-threading the devil is in the detail. Also, what if multiple slots are connected to the `datas` signal? – G.M. Apr 28 '20 at 18:26
  • 1
    `datas` is connected to a unique slot here but for sure this pattern won't work if multiple slots call `delete`. – Fryz Apr 28 '20 at 19:35
  • Passing a `QSharedPointer` (with a custom deleter-function, if necessary for passing an array, as shown here: https://stackoverflow.com/a/7875821/131930 ) would be much safer than sending an unmanaged heap-allocation across the signal/slot mechanism. – Jeremy Friesner Apr 28 '20 at 22:10
  • 1
    Sending single-ownership pointers via signal-slot connections is a recipe for disaster. A signal may have zero or more listeners attached. That’s how the signals are meant to be used. A signal that leaks memory if there are no listeners, or that causes double deletion when two slots are connected - is just bad news. To pass ownership you’d want to use events since they are guaranteed to be destroyed exactly once, even if nobody accepts the event. If you insist on using signal-slot connections, use an ref counted shared object or std::shared_ptr. – Kuba hasn't forgotten Monica Apr 30 '20 at 03:31
  • Not related - but i have fixed the code sample so that the sender really emit the signal from the worker thread as my question involved signals/slots mechanism across different threads – Fryz Apr 30 '20 at 14:18

1 Answers1

1

Yes it is 'safe' to do so if you can garantee that the pointer will still be valid when you`ll access it.

In this simple example that seems the case. I see a potential issue in your code that might be the cause of your random crash :

Event driven objects may only be used in a single thread. Specifically, this applies to the timer mechanism and the network module. For example, you cannot start a timer or connect a socket in a thread that is not the object's thread.

from https://doc.qt.io/qt-5/threads-qobject.html paragraph Object reentrancy.

This is what you are doing : sender object is created on the main thread and on so the timer is started on this thread, then it is moved to the worker thread.

Another thing that I am not 100% confident about : you perform the connection before moving the object to the other thread. By default if nothing is said about the connection, when two objects are on the same thread, it is a direct connection, and when object are on different thread, it´s obviously a queued connection. I don´t know to which extend Qt is robust to changing the connection type when an object is moved, but I would rather first move the object and then connect it.

sandwood
  • 2,038
  • 20
  • 38
  • Actually `QTimer` is not used in real code. In this small example it's just a simple way to trigger a signal after the event loop is started. – Fryz Apr 28 '20 at 19:47