Invalid thread pointer crash in qtpromise_defer()

[top-master]

https://github.com/simonbrunel/qtpromise/blob/21faa67b5857e2999d29656698201593107b4085/src/qtpromise/qpromise_p.h#L62

Above first you check if thread is nullptr, then call it's isFinished() method.

But according to Qt docs:

"QPointer<T>, behaves like a normal C++ pointer T *, except that it is automatically set to 0 when the referenced object is destroyed"

Meaning you hold only a weak reference, and even if the thread was not null just moments ago, it can suddenly be an invalid-pointer while isFinished() did not yet return (which hopefully causes a crash, instead of causing more damage).

[glum-psyche]

QPointer should be checked for validity not as !thread but as !thread.isNull().

[pwuertz]

QPointer should be checked for validity not as !thread but as !thread.isNull().

That's just a matter of taste or style guide you choose for your code. It doesn't affect the logic of this check.

For reference, the Qt QPointer documentation uses cast-to-bool instead of isNull: https://doc.qt.io/qt-6/qpointer.html#details

[pwuertz]

even if the thread was not null just moments ago, it can suddenly be an invalid-pointer while isFinished() did not yet return

This is not true as long as you are using QObjects the way they are designed. Usually, you only manipulate a QObject from the thread of execution it belongs to (see QObject thread affinity). So, given that the QThread object to which the QPointer thread points to has thread-affinity to the currently running thread, the object cannot be deleted during the statement (!thread || thread->isFinished()), because you are not synchronously deleting it here, you are not yielding control to the current Qt event loop, and you're not calling external, unpredictable code.

So the only way a QThread object could be deleted during (!thread || thread->isFinished()) is by deleting it concurrently from another thread. This would be a serious error in concurrent design, but unrelated to the use of QPointer.

Question is, is this something that happened to you and do you have an example that triggers such an issue?

[top-master]

Mentioned line is in qtpromise_defer(...), where the call-stack should be something like:

QtPromisePrivate::qtpromise_defer
QtPromisePrivate::PromiseDataBase<T, F>::dispatch
QPromiseBase<T>::then(...) or  PromiseResolver<T>::resolve(...) or PromiseResolver<T>::reject(...)
MyClass

In many designs, not just my situation:

• There is a so called "global service" class, which may create a thread.
• Said service provides some Promises based on said thread.
• Then said service deletes thread once it's no longer required.
• And the promises live on, which can be used by other threads.
• But currently, anyone using any of said promises meets the race condition which this ticket (#54) is about.

Correct me if I am wrong:

• Seems then(...) and resolve(...) and reject(...) are meant to be thread-safe,
• but they aren't,
• because the weak-ref QPointer<T> is used, while it should be using a strong-ref like QSharedPointer<T>.

---

Also comment-1370895817 says:

"only manipulate a QObject from the thread of execution it belongs to ..."

Which is not related to this issue, and confusing, because nothing of above call-stack trace blongs to a QObject. Do you really expect us to imagine QtPromise::QPromise<T> to be a QObject???

[pwuertz]

only manipulate a QObject from the thread of execution it belongs to ...

Which is not related to this issue, and confusing, because nothing of above call-stack trace blongs to a QObject. Do you really expect us to imagine QtPromise::QPromise<T> to be a QObject???

Oh sorry, that's not what I was referring to. That comment was related to the thread-pointer from your question and the general use of QPointer and a QObject it points to, a QThread in this case. QPointer is designed for cooperative multitasking, i.e. you wouldn't use it in a situation where it could "suddenly be an invalid-pointer" in midst of an expression. QPointer has no concept of concurrency or lifetime management.

Seems then(...) and resolve(...) and reject(...) are meant to be thread-safe.. but they aren't

Those methods are thread-safe. A thread-safe method is a method that can be called from any thread. But sometimes this requires that certain conditions concerning the environment the call operates on are met. Stupid example: Say, you're calling a thread-safe method that takes a ref to some data, you delete the data, crash. That doesn't mean that the method itself is thread-unsafe, it's just not meant to be used that way. Of course you'd try to prevent improper use via API, but sometimes you can't.

Wild guess: In our case, attaching continuations to QPromise using then() may be thread-safe, but perhaps it requires you to not delete the QThread of continuation until the promise is done or cancelled.

because the weak-ref QPointer<T> is used, while it should be using a strong-ref like QSharedPointer<T>

Although similar in name, QPointer<T> and QSharedPointer<T> are vastly different in terms of use cases. QSharedPointer and QWeakPointer are basically the Qt versions of std::shared_ptr and std::weak_ptr - i.e. your basic building blocks for ref-counted, shared resources.

QPointer on the other hand is simple pointer to a QObject that is automatically cleared on QObject::destroyed. QPointer has no knowledge about how the lifetime the object it points to is managed.

So generally QPointer and QSharedPointer aren't interchangeable, or meant to be used in similar ways.

... should be using a strong-ref like QSharedPointer

Right, so why not just use a QSharedPointer instead of a QPointer to guarantee the QThread-lifetime within qtpromise_defer? I'm pretty sure it's because you're unable to form a QSharedPointer in the first place. The way a QThread handle is obtained is by calling QThread::currentThread(), a pointer with no information on who manages the lifetime of the QThread and how the lifetime is managed. The only source of lifetime-information is QObject::destroyed. Which is sufficient in a cooperative multitasking situation (-> QPointer), but insufficient in situations where QThread is concurrently destroyed - which a user is of course free to do.

So this is probably the question I was trying to ask: Is your QThread destroyed in the same event-loop that created the QThread object?

[simonbrunel]

As @pwuertz mentioned, we can't acquire a shared pointer on those threads. But even if we could, I'm not sure that's the intent of this library to keep the thread alive until promises living in it are fully resolved.

@top-master I'm wondering where exactly it crashes, can you provide the actual stack trace? Or are you able to provide a minimal project that reproduces consistently this issue, which would help to debug?

[top-master]

As @pwuertz suggested;

• I plan to change my codes,
• To consider QThread as required by promises,
• Because currently we are forced to ensure QThread is not deleted as long as it's promises remain.

But just because this library requires that, I mean:

• Normally, no matter if the unit (class or function) is thread-safe or not,
• Only the data we directly pass as parameter to the unit should be required, and even that only until the unit returns,
• Anything else that's required should be backupped or at least strong-referenced by the unit.

Note that there are exceptions to above "backup rule", for example, if a class takes a raw-pointer from us as parameter, then I would check it's docs or source-code to know how long the data is required.

However, this library did hiddenly take pointer, and crashed later.

---

@simonbrunel

"where exactly it crashes ... ?"

In my case, inside QThread's isFinished method, which I excluded from already posted call-stack trace.

"minimal project that reproduces ..."

• That's only required if we are not sure what the issue is,
• or if we want to make a unit-test to check if it's fixed,
• but in this case, I got "won't fix" as response, hence let's close this ticket.

[pwuertz]

minimal project that reproduces ..

.. only required if we are not sure what the issue is

Well, we are not sure. I did mention that at this point it is just a "wild guess". As outlined before, the deletion of a QObject in a cooperative multitasking scenario shouldn't be an issue. When a QThread is deleted within its executor of origin, the QPointer guard and qtpromise_defer should work as intended. QObject::deleteLater() might be a useful tool to ensure this.

However, if qtpromise_defer was called within the wrong context of execution (a thread different from the one 'owning' the QThread), that is indeed a problem that should be fixable within QtPromise. This means that the qtpromise_defer call needs to be posted to the event-loop responsible for the QThread, causing qtpromise_defer to be synchronous to a deletion of QThread.

Hence my question from earlier

Is your QThread destroyed in the same event-loop that created the QThread object?

Which should provide some clue if either

• QThread was deleted in the wrong thread
• or qtpromise_defer is running in the wrong thread

If it's the second type of issue, it is probably fixable within QtPromise. In case it is not fixable, we should add this as a requirement in the documentation (e.g. "don't delete the executor of a pending promise").

.. currently we are forced to ensure QThread is not deleted as long as its promises remain

Depending on what you mean by 'remaining promise' I'd argue that this is a practice that should be followed either way. Otherwise you'd be deleting the very same thread that was supposed to execute the async task you attached to it. Right now, QtPromise is designed to just skip/drop continuations for stopped event-loops, but I'd even consider this to be a warning/assert-worthy bug (see Python asyncio for reference). Think about tasks with RAII-like behavior / finally() clauses, code that is supposed to be called at all times, being partially skipped at random.

[simonbrunel]

Thanks @pwuertz for this detailed analysis! I agree on your conclusions and indeed, we need more context and ideally a test case so we understand better this crash which may be caused by op's setup (e.g. deleting the thread from the wrong one).

Right now, QtPromise is designed to just skip/drop continuations for stopped event-loops, but I'd even consider this to be a warning/assert-worthy bug

That's a good suggestion, though maybe we should only activate this warning / assert via a compilation flag.

@top-master or if we want to make a unit-test to check if it's fixed,

or if we want to make the maintainer's job easier, so they don't spend too much time trying to debug your problem that they've never encountered and which is (or at least seems) hard to reproduce.

@top-master but in this case, I got "won't fix" as response, hence let's close this ticket.

I'm not sure where you got this response or what made you think that way. However, if you know how to fix that issue, you can still open a PR. I will be happy to review either a proper fix (and test) that prevents crashing in your situation or, if it's not fixable, an update of the docs that specifies requirements for using QPromise between multiple threads.

I'm re-opening this issue so if someone else experience the same crash, at least they know it's not fixed!

[top-master]

"maybe we should only activate this warning / assert via a compilation flag."

No need for a flag and/or macro-define, instead maybe:

• Simply set a boolean variable, and warn not more than once.
• Or, set a quint64 variable to the time-stamp, and warn not more than once per minute.

---

"to make the maintainer's job easier ... hard to reproduce"

Yes, it's hard to reproduce, at least if we provide detailed explanation, but as long as you don't use a QSharedPointer, what happens is basically something like:

QThread *thread = new QThread();
delete thread;
thread->isFinished()

More detailed explanation:

// 1: User creates thread-A:
QThread *thread = new QThread();

// 2: Some Promise chaining happens on thread-A,
// which **caches** `QPointer` / weak-ref to thread-A.

// 3: Thread-B causes thread-A to be deleted.
delete thread;
thread = nullptr;

// 4: Thread-C resolves Promise, while Thread-B is in progress,
// which causes previously cached `QPointer<QThread>` to be used,
// which causes a race condition.
bool isEnded = !thread || thread->isFinished();
std::cout << "Did not crash this time, try again later, you may someday reproduce it ;-)"
    << (isEnded ? "thread ended" : "thread is running.");

---

"where you got this response ..."

The comments were very convincing, those around: "unable to form a QSharedPointer in the first place".

"if you know how to fix that issue ..."

If you can't form a strong-ref to a QThread, simply create your own custom Executer class, and users should pass a strong-ref to Executer instance whenever running on a separate thread is desired.

I mean, by default, all chained handlers/listeners should run on main-thread (if Executer is not passed).

But I guess, it's too late for such breaking change.