From b0e82f598193f7d2c331a7ff3da7e2d3114db714 Mon Sep 17 00:00:00 2001 From: Ivan Solovev Date: Mon, 27 Jan 2025 14:49:52 +0100 Subject: [PATCH] QFutureInterfaceBase: rework setContinuation() overload set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 --- src/corelib/thread/qfuture_impl.h | 39 ++++--- src/corelib/thread/qfutureinterface.cpp | 144 +++++++++++++++++------- src/corelib/thread/qfutureinterface.h | 13 +++ src/corelib/thread/qfutureinterface_p.h | 2 + 4 files changed, 141 insertions(+), 57 deletions(-) diff --git a/src/corelib/thread/qfuture_impl.h b/src/corelib/thread/qfuture_impl.h index 2f709a56621..864222acb1a 100644 --- a/src/corelib/thread/qfuture_impl.h +++ b/src/corelib/thread/qfuture_impl.h @@ -16,6 +16,7 @@ #include #include #include +#include #include @@ -491,8 +492,13 @@ struct ContinuationWrapper ContinuationWrapper(ContinuationWrapper &&other) = default; ContinuationWrapper &operator=(ContinuationWrapper &&) = default; + template , bool> = true> void operator()(const QFutureInterfaceBase &parentData) { function(parentData); } + template , bool> = true> + void operator()() { function(); } + private: Function function; }; @@ -544,7 +550,8 @@ void CompactContinuation::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 @@ -572,16 +579,8 @@ void CompactContinuation::create(F &&fun continuationJob = nullptr; } }; - f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d); -} - -template -void watchContinuation(const QObject *context, Continuation &&c, QFutureInterfaceBase &fi) -{ - using Prototype = typename QtPrivate::Callable::Function; - watchContinuationImpl(context, - QtPrivate::makeCallableObject(std::forward(c)), - fi); + f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d, + QFutureInterfaceBase::ContinuationType::Then); } template @@ -604,7 +603,9 @@ void CompactContinuation::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 @@ -678,7 +679,8 @@ void FailureHandler::create(F &&function, QFutured.setContinuation(ContinuationWrapper(std::move(failureContinuation))); + future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation)), fi.d, + QFutureInterfaceBase::ContinuationType::OnFailed); } template @@ -696,7 +698,9 @@ void FailureHandler::create(F &&function, QFutured); + future->d.setContinuation(context, ContinuationWrapper(std::move(failureContinuation)), + QVariant::fromValue(fi), + QFutureInterfaceBase::ContinuationType::OnFailed); } template @@ -776,7 +780,8 @@ public: auto parentFuture = QFutureInterface(parentData).future(); run(std::forward(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 @@ -790,7 +795,9 @@ public: run(std::forward(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 diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index 2236b5d1db1..3252a0def54 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/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; - 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(); - 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) + // That is now a double-inderection, because the setContinuation() overload + // also uses QSlotObjectBase approach. But that's a solution for backwards + // compatibility, so should be fine. + // We pass a default-constructed QVariant() and an Unknown type, because + // that's effectively the same as passing a nullptr continuationData, and + // that's what the old code was doing. + fi.setContinuation(context, ContinuationWrapper([slot = std::move(slot)]() { - Q_UNUSED(parentData); - QMutexLocker lock(watcherMutex.get()); - if (watcher) - emit watcher->run(); - }); + void *args[] = { nullptr }; // for `void` return value + slot->call(nullptr, args); + }), QVariant(), QFutureInterfaceBase::ContinuationType::Unknown); } 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 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 func, + void *continuationFutureData, ContinuationType type) +{ + auto *futureData = static_cast(continuationFutureData); + QMutexLocker lock(&d->continuationMutex); // If the state is ready, run continuation immediately, @@ -902,10 +883,91 @@ void QFutureInterfaceBase::setContinuation(std::functioncontinuation = 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 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 func, + const QVariant &continuationFuture, + ContinuationType type) +{ + Q_ASSERT(context); + + using FuncType = void(); + using Prototype = typename QtPrivate::Callable::Function; + auto slotObj = QtPrivate::makeCallableObject(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(); + 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. + QFutureInterfaceBasePrivate *continuationFutureData = nullptr; + if (continuationFuture.isValid()) { + Q_ASSERT(QLatin1StringView(continuationFuture.typeName()) + .startsWith(QLatin1StringView("QFutureInterface"))); + const auto continuationPtr = + static_cast(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) diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index 5e28cecda1d..2c25473f799 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/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 func); void setContinuation(std::function func, QFutureInterfaceBasePrivate *continuationFutureData); + void setContinuation(std::function func, + void *continuationFutureData, ContinuationType type); + void setContinuation(const QObject *context, std::function func, + const QVariant &continuationFuture, ContinuationType type); void cleanContinuation(); void runContinuation() const; diff --git a/src/corelib/thread/qfutureinterface_p.h b/src/corelib/thread/qfutureinterface_p.h index c573ba8274d..fdf80779db8 100644 --- a/src/corelib/thread/qfutureinterface_p.h +++ b/src/corelib/thread/qfutureinterface_p.h @@ -164,6 +164,8 @@ public: std::atomic continuationState { Default }; bool continuationExecuted = false; + QFutureInterfaceBase::ContinuationType continuationType = QFutureInterfaceBase::ContinuationType::Unknown; + inline QThreadPool *pool() const { return m_pool ? m_pool : QThreadPool::globalInstance(); }