QFutureInterfaceBase: rework setContinuation() overload set

Add new overloads to make sure that we can pass continuation data
even for the continuations with context. The new overloads
intentionally do not expose QFutureInterfaceBasePrivate pointer,
because they would be used in the inline code (see QTBUG-117514).
The overload with a context provides a continuation future as a
QVariant, because that is the easiest way to keep the
QFutureInterface<T> object alive without exposing the template
type T to a non-templated exported base class. We cannot pass the
QFutureInterfaceBase because of the reference count logic, which
contains two different counters: for base class (void case), and
for the interface that holds an actual type T.

This patch is a pre-requisite for a follow-up patch that would
introduce the ability to cancel the entire continuation
chain from the first QFuture object of the chain, even if
that QFuture is already finished.

Note that the pre-existing QFutureInterface::cancel() code
relies on the fact that only then-continuations provide
continuation data. The onFailed() and onCanceled() handlers
never provide it, even when they were supposed to be executed
in the same thread.
To keep the old behavior on the cancel() method, introduce
a new ContinuationType enum and make sure that the type is
properly set when providing a continuation data.

The new overloads also allow to get rid of the template
QtPrivate::watchContinuation() helper method. The exported
Impl should stay for BC reasons. This patch implements it
in terms of the new setContinuation() overload.

During the development of this patch it was important to
verify that the behavior of all public and/or exported
methods stays unchanged. It was done by running the tst_qfuture
binary that was originally compiled with the old QtCore version
against the updated QtCore library.

Task-number: QTBUG-130662
Change-Id: I2bd5821846561d18c198205f3c57d3f0dacbb94b
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

src/corelib/thread/qfuture_impl.h

@@ -16,6 +16,7 @@
 #include <QtCore/qthreadpool.h>
 #include <QtCore/qexception.h>
 #include <QtCore/qpromise.h>
+#include <QtCore/qvariant.h>
 
 #include <memory>
 
@@ -491,8 +492,13 @@ struct ContinuationWrapper
     ContinuationWrapper(ContinuationWrapper &&other) = default;
     ContinuationWrapper &operator=(ContinuationWrapper &&) = default;
 
+    template <typename F = Function,
+              std::enable_if_t<std::is_invocable_v<F, const QFutureInterfaceBase &>, bool> = true>
     void operator()(const QFutureInterfaceBase &parentData) { function(parentData); }
 
+    template <typename F = Function, std::enable_if_t<std::is_invocable_v<F>, bool> = true>
+    void operator()() { function(); }
+
 private:
     Function function;
 };
@@ -544,7 +550,8 @@ void CompactContinuation<Function, ResultType, ParentResultType>::create(F &&fun
             continuationJob = nullptr;
         }
     };
-    f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d);
+    f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d,
+                         QFutureInterfaceBase::ContinuationType::Then);
 }
 
 template<typename Function, typename ResultType, typename ParentResultType>
@@ -572,16 +579,8 @@ void CompactContinuation<Function, ResultType, ParentResultType>::create(F &&fun
             continuationJob = nullptr;
         }
     };
-    f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d);
+    f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d,
-}
+                         QFutureInterfaceBase::ContinuationType::Then);
-
-template <typename Continuation>
-void watchContinuation(const QObject *context, Continuation &&c, QFutureInterfaceBase &fi)
-{
-    using Prototype = typename QtPrivate::Callable<Continuation>::Function;
-    watchContinuationImpl(context,
-                          QtPrivate::makeCallableObject<Prototype>(std::forward<Continuation>(c)),
-                          fi);
 }
 
 template<typename Function, typename ResultType, typename ParentResultType>
@@ -604,7 +603,9 @@ void CompactContinuation<Function, ResultType, ParentResultType>::create(F &&fun
         continuationJob.execute();
     };
 
-    QtPrivate::watchContinuation(context, std::move(continuation), f->d);
+    f->d.setContinuation(context, ContinuationWrapper(std::move(continuation)),
+                         QVariant::fromValue(fi),
+                         QFutureInterfaceBase::ContinuationType::Then);
 }
 
 template<typename Function, typename ResultType, typename ParentResultType>
@@ -678,7 +679,8 @@ void FailureHandler<Function, ResultType>::create(F &&function, QFuture<ResultTy
         failureHandler.run();
     };
 
-    future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation)));
+    future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation)), fi.d,
+                              QFutureInterfaceBase::ContinuationType::OnFailed);
 }
 
 template<class Function, class ResultType>
@@ -696,7 +698,9 @@ void FailureHandler<Function, ResultType>::create(F &&function, QFuture<ResultTy
         failureHandler.run();
     };
 
-    QtPrivate::watchContinuation(context, std::move(failureContinuation), future->d);
+    future->d.setContinuation(context, ContinuationWrapper(std::move(failureContinuation)),
+                              QVariant::fromValue(fi),
+                              QFutureInterfaceBase::ContinuationType::OnFailed);
 }
 
 template<class Function, class ResultType>
@@ -776,7 +780,8 @@ public:
             auto parentFuture = QFutureInterface<ResultType>(parentData).future();
             run(std::forward<F>(handler), parentFuture, std::move(promise));
         };
-        future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation)));
+        future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation)), fi.d,
+                                  QFutureInterfaceBase::ContinuationType::OnCanceled);
     }
 
     template<class F = Function>
@@ -790,7 +795,9 @@ public:
             run(std::forward<F>(handler), parentFuture, std::move(promise));
         };
 
-        QtPrivate::watchContinuation(context, std::move(canceledContinuation), future->d);
+        future->d.setContinuation(context, ContinuationWrapper(std::move(canceledContinuation)),
+                                  QVariant::fromValue(fi),
+                                  QFutureInterfaceBase::ContinuationType::OnCanceled);
     }
 
     template<class F = Function>

src/corelib/thread/qfutureinterface.cpp

@@ -63,46 +63,17 @@ void QtPrivate::watchContinuationImpl(const QObject *context, QSlotObjectBase *s
 
     auto slot = SlotObjUniquePtr(slotObj);
 
-    auto *watcher = new QObjectContinuationWrapper;
+    // That is now a double-inderection, because the setContinuation() overload
-    watcher->moveToThread(context->thread());
+    // also uses QSlotObjectBase approach. But that's a solution for backwards
-
+    // compatibility, so should be fine.
-    // We need to protect acccess to the watcher. The context object (and in turn, the watcher)
+    // We pass a default-constructed QVariant() and an Unknown type, because
-    // could be destroyed while the continuation that emits the signal is running. We have to
+    // that's effectively the same as passing a nullptr continuationData, and
-    // prevent that.
+    // that's what the old code was doing.
-    // The mutex has to be recursive, because the continuation itself could delete the context
+    fi.setContinuation(context, ContinuationWrapper([slot = std::move(slot)]()
-    // object (and thus the watcher), which will try to lock the mutex from the same thread twice.
-    auto watcherMutex = std::make_shared<QRecursiveMutex>();
-    const auto destroyWatcher = [watcherMutex, watcher]() mutable {
-        QMutexLocker lock(watcherMutex.get());
-        delete watcher;
-    };
-
-    // ### we're missing a convenient way to `QObject::connect()` to a `QSlotObjectBase`...
-    QObject::connect(watcher, &QObjectContinuationWrapper::run,
-                     // for the following, cf. QMetaObject::invokeMethodImpl():
-                     // we know `slot` is a lambda returning `void`, so we can just
-                     // `call()` with `obj` and `args[0]` set to `nullptr`:
-                     context, [slot = std::move(slot)] {
-                         void *args[] = { nullptr }; // for `void` return value
-                         slot->call(nullptr, args);
-                     });
-    QObject::connect(watcher, &QObjectContinuationWrapper::run, watcher, destroyWatcher);
-
-    // We need to connect to destroyWatcher here, instead of delete or deleteLater().
-    // If the continuation is called from a separate thread, emit watcher->run() can't detect that
-    // the watcher has been deleted in the separate thread, causing a race condition and potential
-    // heap-use-after-free issue inside QObject::doActivate. destroyWatcher forces the deletion of
-    // the watcher to occur after emit watcher->run() completes and prevents the race condition.
-    QObject::connect(context, &QObject::destroyed, watcher, destroyWatcher);
-
-    fi.setContinuation([watcherMutex, watcher = QPointer(watcher)]
-                       (const QFutureInterfaceBase &parentData)
     {
-        Q_UNUSED(parentData);
+        void *args[] = { nullptr }; // for `void` return value
-        QMutexLocker lock(watcherMutex.get());
+        slot->call(nullptr, args);
-        if (watcher)
+    }), QVariant(), QFutureInterfaceBase::ContinuationType::Unknown);
-            emit watcher->run();
-    });
 }
 
 QFutureCallOutInterface::~QFutureCallOutInterface()
@@ -171,7 +142,7 @@ void QFutureInterfaceBase::cancel(QFutureInterfaceBase::CancelMode mode)
 
     // Cancel the continuations chain
     QFutureInterfaceBasePrivate *next = d->continuationData;
-    while (next) {
+    while (next && next->continuationType == ContinuationType::Then) {
         next->continuationState = QFutureInterfaceBasePrivate::Canceled;
         next = next->continuationData;
     }
@@ -883,6 +854,16 @@ void QFutureInterfaceBase::setContinuation(std::function<void(const QFutureInter
 void QFutureInterfaceBase::setContinuation(std::function<void(const QFutureInterfaceBase &)> func,
                                            QFutureInterfaceBasePrivate *continuationFutureData)
 {
+    // Backwards compatibility - the continuation data was used for
+    // then-continuations
+    setContinuation(std::move(func), continuationFutureData, ContinuationType::Then);
+}
+
+void QFutureInterfaceBase::setContinuation(std::function<void (const QFutureInterfaceBase &)> func,
+                                           void *continuationFutureData, ContinuationType type)
+{
+    auto *futureData = static_cast<QFutureInterfaceBasePrivate *>(continuationFutureData);
+
     QMutexLocker lock(&d->continuationMutex);
 
     // If the state is ready, run continuation immediately,
@@ -902,10 +883,91 @@ void QFutureInterfaceBase::setContinuation(std::function<void(const QFutureInter
                      "The existing continuation is overwritten.");
         }
         d->continuation = std::move(func);
-        d->continuationData = continuationFutureData;
+        if (futureData)
+            futureData->continuationType = type;
+        d->continuationData = futureData;
+        Q_ASSERT_X(!futureData || futureData->continuationType != ContinuationType::Unknown,
+                   "setContinuation", "Make sure to provide a correct continuation type!");
     }
 }
 
+/*
+    For continuations with context we expect all the needed data to be captured
+    directly by the continuation data, because this simplifies the slot
+    invocation. That's why func has no parameters.
+
+    We pass continuation data as a QVariant, because we need to keep the
+    QFutureInterface<T> for the entire lifetime of the continuation, but we
+    cannot pass a template type T as a parameter.
+*/
+void QFutureInterfaceBase::setContinuation(const QObject *context, std::function<void()> func,
+                                           const QVariant &continuationFuture,
+                                           ContinuationType type)
+{
+    Q_ASSERT(context);
+
+    using FuncType = void();
+    using Prototype = typename QtPrivate::Callable<FuncType>::Function;
+    auto slotObj = QtPrivate::makeCallableObject<Prototype>(std::move(func));
+
+    auto slot = QtPrivate::SlotObjUniquePtr(slotObj);
+
+    auto *watcher = new QObjectContinuationWrapper;
+    watcher->moveToThread(context->thread());
+
+   // We need to protect acccess to the watcher. The context object (and in turn, the watcher)
+   // could be destroyed while the continuation that emits the signal is running. We have to
+   // prevent that.
+   // The mutex has to be recursive, because the continuation itself could delete the context
+   // object (and thus the watcher), which will try to lock the mutex from the same thread twice.
+    auto watcherMutex = std::make_shared<QRecursiveMutex>();
+    const auto destroyWatcher = [watcherMutex, watcher]() mutable {
+        QMutexLocker lock(watcherMutex.get());
+        delete watcher;
+    };
+
+    // ### we're missing a convenient way to `QObject::connect()` to a `QSlotObjectBase`...
+    QObject::connect(watcher, &QObjectContinuationWrapper::run,
+                     // for the following, cf. QMetaObject::invokeMethodImpl():
+                     // we know `slot` is a lambda returning `void`, so we can just
+                     // `call()` with `obj` and `args[0]` set to `nullptr`:
+                     context, [slot = std::move(slot)] {
+                         void *args[] = { nullptr }; // for `void` return value
+                         slot->call(nullptr, args);
+                     });
+    QObject::connect(watcher, &QObjectContinuationWrapper::run, watcher, destroyWatcher);
+
+    // We need to connect to destroyWatcher here, instead of delete or deleteLater().
+    // If the continuation is called from a separate thread, emit watcher->run() can't detect that
+    // the watcher has been deleted in the separate thread, causing a race condition and potential
+    // heap-use-after-free issue inside QObject::doActivate. destroyWatcher forces the deletion of
+    // the watcher to occur after emit watcher->run() completes and prevents the race condition.
+    QObject::connect(context, &QObject::destroyed, watcher, destroyWatcher);
+
+    // Extract a QFutureInterfaceBasePrivate pointer from the QVariant. We rely
+    // on the fact that QVariant contains QFutureInterface<T>.
+    QFutureInterfaceBasePrivate *continuationFutureData = nullptr;
+    if (continuationFuture.isValid()) {
+        Q_ASSERT(QLatin1StringView(continuationFuture.typeName())
+                         .startsWith(QLatin1StringView("QFutureInterface")));
+        const auto continuationPtr =
+                static_cast<const QFutureInterfaceBase *>(continuationFuture.constData());
+        continuationFutureData = continuationPtr->d;
+    }
+
+    // Capture continuationFuture so that it lives as long as the continuation,
+    // and the continuation data remains valid.
+    setContinuation([watcherMutex, watcher = QPointer(watcher), continuationFuture]
+                    (const QFutureInterfaceBase &parentData)
+    {
+        Q_UNUSED(parentData);
+        Q_UNUSED(continuationFuture);
+        QMutexLocker lock(watcherMutex.get());
+        if (watcher)
+            emit watcher->run();
+    }, continuationFutureData, type);
+}
+
 void QFutureInterfaceBase::cleanContinuation()
 {
     if (!d)

src/corelib/thread/qfutureinterface.h

@@ -164,6 +164,7 @@ public:
 #ifndef QFUTURE_TEST
 private:
 #endif
+    friend class QFutureInterfaceBasePrivate;
     QFutureInterfaceBasePrivate *d;
 
 private:
@@ -188,9 +189,21 @@ private:
     friend class QPromise;
 
 protected:
+    enum class ContinuationType : quint8
+    {
+        Unknown,
+        Then,
+        OnFailed,
+        OnCanceled,
+    };
+
     void setContinuation(std::function<void(const QFutureInterfaceBase &)> func);
     void setContinuation(std::function<void(const QFutureInterfaceBase &)> func,
                          QFutureInterfaceBasePrivate *continuationFutureData);
+    void setContinuation(std::function<void(const QFutureInterfaceBase &)> func,
+                         void *continuationFutureData, ContinuationType type);
+    void setContinuation(const QObject *context, std::function<void()> func,
+                         const QVariant &continuationFuture, ContinuationType type);
     void cleanContinuation();
     void runContinuation() const;
 

src/corelib/thread/qfutureinterface_p.h

@@ -164,6 +164,8 @@ public:
     std::atomic<ContinuationState> continuationState { Default };
     bool continuationExecuted = false;
 
+    QFutureInterfaceBase::ContinuationType continuationType = QFutureInterfaceBase::ContinuationType::Unknown;
+
     inline QThreadPool *pool() const
     { return m_pool ? m_pool : QThreadPool::globalInstance(); }