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(); }