12

I have a scenario where an anonymous QObject starts an asynchronous operation by emitting a signal. The receiving slot stores the QObject-pointer and sets a property of this object later. The object could be gone meanwhile.

So, is there a safe way to check if this pointer still valid?

P.S.: I'm aware of QObject::destroyed signal, which I could connect to the object supposed to call the setProperty of that pointer. But I wonder, if it works easier.

Ilya
  • 4,583
  • 4
  • 26
  • 51
Valentin H
  • 7,240
  • 12
  • 61
  • 111

2 Answers2

15

This is a great question, but it is the wrong question.

Is there a way to check if the pointer is valid? Yes. QPointer is designed specifically to do that.

But the answer to this question is useless if the object lives in another thread! You only know whether it's valid at a single point in time - the answer is not valid immediately afterwards.

Absent other mechanisms, it is useless to hold a QPointer to an object in a different thread - it won't help you. Why? Look at this scenario:

             Thread A                    Thread B
1. QPointer returns a non-zero pointer
2.                                     deletes the object
3. Use the now-dangling pointer

I'm aware of QObject::destroyed signal, which I could connect to the object supposed to call the setProperty of that pointer. But I wonder, if it works easier.

The destroyed signals are useless when sent using queued connections - whether within a thread, or across thread boundaries. They are meant to be used within one thread, using direct connections.

By the time the target thread's event loop picks up the slot call, the originating object is long gone. Worse - this is always the case in a single-threaded application. The reason for the problem is the same as with the QPointer: the destroyed signal indicates that the object is no longer valid, but it doesn't mean that it was valid before you received the signal unless you're using a direct connection (and are in the same thread) or you're using a blocking queued connection.

Using the blocking queued connection, the requesting object's thread will block until the async thread finishes reacting to object's deletion. While this certainly "works", it forces the two threads to synchronize on a resource with sparse availability - the front spot in the async thread's event loop. Yes, this is literally what you compete for - a single spot in a queue that can be arbitrarily long. While this might be OK for debugging, it has no place in production code unless it's OK to block either thread to synchronize.

You are trying to work very hard around the fact that you're passing a QObject pointer between threads, and the object's lifetime, from the point of view of the receiving thread, is uncontrolled. That's your problem. You'd solve everything by not passing a raw object pointer. Instead, you could pass a shared smart pointer, or using signal-slot connections: those vanish whenever either end of the connection is destructed. That's what you'd want.

In fact, Qt's own design patterns hint at this. QNetworkReply is a QObject not only because it is a QIODevice, but because it must be to support direct indications of finished requests across thread boundaries. In light of a multitude of requests being processed, connecting to QNetworkAccessManager::finished(QNetworkReply*) can be a premature pessimization. Your object gets notified of a possibly very large number of replies, but it really is only interested in one or very few of them. Thus there must be a way to notify the requester directly that its one and only request is done - and that's what QNetworkReply::finished is for.

So, a simple way to proceed is to make the Request be a QObject with a done signal. When you ready the request, connect the requesting object to that signal. You can also connect a functor, but make sure that the functor executes in the requesting object's context:

// CORRECT
connect(request, &Request::done, requester, [...](...){...});
// WRONG
connect(request, &Request::done, [...](...){...});

The below demonstrates how it could be put together. The requests' lifetimes are managed through the use of a shared (reference-counting) smart pointer. This makes life rather easy. We check that no requests exist at the time main returns.

#include <QtCore>

class Request;
typedef QSharedPointer<Request> RequestPtr;
class Request : public QObject {
   Q_OBJECT
public:
   static QAtomicInt m_count;
   Request() { m_count.ref(); }
   ~Request() { m_count.deref(); }
   int taxIncrease;
   Q_SIGNAL void done(RequestPtr);
};
Q_DECLARE_METATYPE(RequestPtr)
QAtomicInt Request::m_count(0);

class Requester : public QObject {
   Q_OBJECT
   Q_PROPERTY (int catTax READ catTax WRITE setCatTax NOTIFY catTaxChanged)
   int m_catTax;
public:
   Requester(QObject * parent = 0) : QObject(parent), m_catTax(0) {}
   Q_SLOT int catTax() const { return m_catTax; }
   Q_SLOT void setCatTax(int t) {
      if (t != m_catTax) {
         m_catTax = t;
         emit catTaxChanged(t);
      }
   }
   Q_SIGNAL void catTaxChanged(int);
   Q_SIGNAL void hasRequest(RequestPtr);
   void sendNewRequest() {
      RequestPtr req(new Request);
      req->taxIncrease = 5;
      connect(req.data(), &Request::done, this, [this, req]{
         setCatTax(catTax() + req->taxIncrease);
         qDebug() << objectName() << "has cat tax" << catTax();
         QCoreApplication::quit();
      });
      emit hasRequest(req);
   }
};

class Processor : public QObject {
   Q_OBJECT
public:
   Q_SLOT void process(RequestPtr req) {
      QThread::msleep(50); // Pretend to do some work.
      req->taxIncrease --; // Figure we don't need so many cats after all...
      emit req->done(req);
      emit done(req);
   }
   Q_SIGNAL void done(RequestPtr);
};

struct Thread : public QThread { ~Thread() { quit(); wait(); } };

int main(int argc, char ** argv) {
   struct C { ~C() { Q_ASSERT(Request::m_count == 0); } } check;
   QCoreApplication app(argc, argv);
   qRegisterMetaType<RequestPtr>();
   Processor processor;
   Thread thread;
   processor.moveToThread(&thread);
   thread.start();

   Requester requester1;
   requester1.setObjectName("requester1");
   QObject::connect(&requester1, &Requester::hasRequest, &processor, &Processor::process);
   requester1.sendNewRequest();
   {
      Requester requester2;
      requester2.setObjectName("requester2");
      QObject::connect(&requester2, &Requester::hasRequest, &processor, &Processor::process);
      requester2.sendNewRequest();
   } // requester2 is destructed here
   return app.exec();
}

#include "main.moc"
Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • Thanks for pointing to QPointer with so much endurance and pressure. I was finally forced to take a look at it :-). Somehow I seem to have been missing a very essential class in Qt. This does actually what I need - listens for destroyed and sets the pointer to 0. No need for a global QObject* list. Clean Qt-way. Multi-threading is not an issue in my case, as all this is done in GUI-Thread. Thanks! If ilya deletes his answer, I'll select yours. – Valentin H Aug 27 '15 at 13:19
  • Oh, I was not aware, that selected answer could be changed. Ok, yours is the winner :-) – Valentin H Aug 27 '15 at 13:20
  • @ValentinHeinitz Make sure that both the `QPointer` and the object it points to are only used from the same thread. That's how it's meant to be used, and it will work perfectly then. As soon as the object and the pointer are used in different threads, you have undefined behavior and you must seek other solutions. – Kuba hasn't forgotten Monica Aug 27 '15 at 14:16
7

It is impossible to check is that pointer still valid. So, the only safe way here is to inform receiving part about deleting of that QObject (and in multithreading case: before accessing to object you need to check and block it to be sure, that the object will not be deleted in another thread right after check). The reason of it is simple:

  • Theoretically it is possible that after deleting of initial object, system will put another object in that memory (so pointer will look like valid).
  • Or it is possible that object will be deleted, but it's memory will not be overwritten by something else, so it still will look like valid (but it fact it will be invalid).
  • So, there are no any way to detect is that pointer valid, if you have only pointer. You need something more.

Also it is not safe to just send a signal about deleting of object in multithreading case (or to use QObject::destroyed as you suggested). Why? Because it is possible, that things happens in this order:

  • QObject send a message "a am going to be deleted",
  • QObject deleted,
  • your receiving code uses that pointer (and this is wrong and dangerous),
  • your receiving code receives message "a am going to be deleted" (too late).

So, in case of only one thread you need QPointer. Else you need something like QSharedPointer or QWeakPointer (both of them are thread-safe) - see answer of Kuba Ober.

Ilya
  • 4,583
  • 4
  • 26
  • 51
  • The first point is good indeed! Concerning the last 2, I mean a safe way provided by Qt, not plain C++. I think there should be a list of all objects internally stored in Qt. So the check in this list should be easy ans inexpensive. SharedPointer would help in call-back like pattern, but I want to use signals/slots only. – Valentin H Aug 26 '15 at 05:49
  • I think, the point 1 answers the question. It can't reasonably work. Even it a pointer points to a valid QObject, you never know, it it is THE Object you mean. Thank you! – Valentin H Aug 26 '15 at 06:23
  • This is very misleading. `QPointer` has never been meant to use across threads! It does not provide the API necessary to be used as such. **AARGH**. – Kuba hasn't forgotten Monica Aug 26 '15 at 18:24
  • "The only safe way here is to inform receiving part about deleting of that QObject." This is wrong! *Prior* to deleting, you must inform the receiver about it, **block**, the receiver must remove its references to the object, you then **resume** and actually delete the object. Signal-slot connections are handled essentially in that fashion, but the synchronization mechanisms is optimized to minimize the potential for contention, it is lock-free in most cases. There is no other way about it. – Kuba hasn't forgotten Monica Aug 27 '15 at 12:46
  • "might be not safe to just send a signal about deleting of object" This is wrong. **It is never safe to do so across thread boundaries** unless you prematurely pessimize and use a blocking queued connection, with the signal sent **prior to** the deletion (that's how `destroyed` is emitted), or otherwise implement the same synchronization mechanism. – Kuba hasn't forgotten Monica Aug 27 '15 at 12:49
  • Given the deficiencies, I'm really worried that this answer exists at all. Either you have to make sure it is accurate, and that you know enough to write it so, or you should delete it. When talking about undefined behavior (since that's what we talk about), you have to use very precise language. – Kuba hasn't forgotten Monica Aug 27 '15 at 12:52
  • 1
    @KubaOber In the given scenario, the caller is aware of possible deletion of the object. My assumption was, there is a global list of QObject pointers, and could be an API to call `contains` of this list. The Point 1, even it it very theoretical, answers, why such an api is not possible or sensible. – Valentin H Aug 27 '15 at 13:08
  • @KubaOber, thank you for very clear answer! My anwer was incomplete, but I am not sure that I should delete it, because it explains in short, why it is impossible to check correctness of pointer. As far as I understand, it was useful (now we know, that this question was about only one thread). Thank you again for great anwer and comments! – Ilya Aug 27 '15 at 13:54