diff options
author | Sona Kurazyan <sona.kurazyan@qt.io> | 2022-01-18 13:07:43 +0100 |
---|---|---|
committer | Sona Kurazyan <sona.kurazyan@qt.io> | 2022-01-21 01:51:20 +0100 |
commit | 614847eae99bd4b6ce375f9f5572acfb3b513307 (patch) | |
tree | c32138e946654043eb47662a5140db62ab960b15 /src/corelib/thread/qfuture_impl.h | |
parent | a8794503ebdefec53a7a42479c477d04f0efa067 (diff) |
Use QPromise when creating continuations to avoid memory leaks
Continuations were using QFutureInterface to create and return the
associated future to the user. Attaching a continuation to the returned
future could cause memory leaks (described in an earlier commit). Use a
QPromise when saving the continuation, to make sure that the attached
continuation is cleaned in the destructor of the associated QPromise, so
that it doesn't keep any ref-counted copies to the shared data, thus
preventing it from being deleted.
Task-number: QTBUG-99534
Pick-to: 6.3 6.2
Change-Id: I52d5501292095d41d1e060b7dd140c8e5d01335c
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: MÃ¥rten Nordheim <marten.nordheim@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Diffstat (limited to 'src/corelib/thread/qfuture_impl.h')
-rw-r--r-- | src/corelib/thread/qfuture_impl.h | 238 |
1 files changed, 102 insertions, 136 deletions
diff --git a/src/corelib/thread/qfuture_impl.h b/src/corelib/thread/qfuture_impl.h index 869a791b15..112ac21f9e 100644 --- a/src/corelib/thread/qfuture_impl.h +++ b/src/corelib/thread/qfuture_impl.h @@ -332,9 +332,8 @@ class Continuation { public: template<typename F = Function> - Continuation(F &&func, const QFuture<ParentResultType> &f, - const QFutureInterface<ResultType> &p) - : promise(p), parentFuture(f), function(std::forward<F>(func)) + Continuation(F &&func, const QFuture<ParentResultType> &f, QPromise<ResultType> &&p) + : promise(std::move(p)), parentFuture(f), function(std::forward<F>(func)) { } virtual ~Continuation() = default; @@ -342,15 +341,15 @@ public: bool execute(); template<typename F = Function> - static void create(F &&func, QFuture<ParentResultType> *f, QFutureInterface<ResultType> &p, + static void create(F &&func, QFuture<ParentResultType> *f, QFutureInterface<ResultType> &fi, QtFuture::Launch policy); template<typename F = Function> - static void create(F &&func, QFuture<ParentResultType> *f, QFutureInterface<ResultType> &p, + static void create(F &&func, QFuture<ParentResultType> *f, QFutureInterface<ResultType> &fi, QThreadPool *pool); template<typename F = Function> - static void create(F &&func, QFuture<ParentResultType> *f, QFutureInterface<ResultType> &p, + static void create(F &&func, QFuture<ParentResultType> *f, QFutureInterface<ResultType> &fi, QObject *context); private: @@ -367,7 +366,7 @@ protected: void runFunction(); protected: - QFutureInterface<ResultType> promise; + QPromise<ResultType> promise; QFuture<ParentResultType> parentFuture; Function function; }; @@ -377,9 +376,9 @@ class SyncContinuation final : public Continuation<Function, ResultType, ParentR { public: template<typename F = Function> - SyncContinuation(F &&func, const QFuture<ParentResultType> &f, - const QFutureInterface<ResultType> &p) - : Continuation<Function, ResultType, ParentResultType>(std::forward<F>(func), f, p) + SyncContinuation(F &&func, const QFuture<ParentResultType> &f, QPromise<ResultType> &&p) + : Continuation<Function, ResultType, ParentResultType>(std::forward<F>(func), f, + std::move(p)) { } @@ -395,12 +394,12 @@ class AsyncContinuation final : public QRunnable, { public: template<typename F = Function> - AsyncContinuation(F &&func, const QFuture<ParentResultType> &f, - const QFutureInterface<ResultType> &p, QThreadPool *pool = nullptr) - : Continuation<Function, ResultType, ParentResultType>(std::forward<F>(func), f, p), + AsyncContinuation(F &&func, const QFuture<ParentResultType> &f, QPromise<ResultType> &&p, + QThreadPool *pool = nullptr) + : Continuation<Function, ResultType, ParentResultType>(std::forward<F>(func), f, + std::move(p)), threadPool(pool) { - this->promise.setRunnable(this); } ~AsyncContinuation() override = default; @@ -429,15 +428,15 @@ class FailureHandler public: template<typename F = Function> static void create(F &&function, QFuture<ResultType> *future, - const QFutureInterface<ResultType> &promise); + const QFutureInterface<ResultType> &fi); template<typename F = Function> - static void create(F &&function, QFuture<ResultType> *future, - QFutureInterface<ResultType> &promise, QObject *context); + static void create(F &&function, QFuture<ResultType> *future, QFutureInterface<ResultType> &fi, + QObject *context); template<typename F = Function> - FailureHandler(F &&func, const QFuture<ResultType> &f, const QFutureInterface<ResultType> &p) - : promise(p), parentFuture(f), handler(std::forward<F>(func)) + FailureHandler(F &&func, const QFuture<ResultType> &f, QPromise<ResultType> &&p) + : promise(std::move(p)), parentFuture(f), handler(std::forward<F>(func)) { } @@ -450,7 +449,7 @@ private: void handleAllExceptions(); private: - QFutureInterface<ResultType> promise; + QPromise<ResultType> promise; QFuture<ResultType> parentFuture; Function handler; }; @@ -460,7 +459,7 @@ private: template<typename Function, typename ResultType, typename ParentResultType> void Continuation<Function, ResultType, ParentResultType>::runFunction() { - promise.reportStarted(); + promise.start(); Q_ASSERT(parentFuture.isFinished()); @@ -497,10 +496,10 @@ void Continuation<Function, ResultType, ParentResultType>::runFunction() } #ifndef QT_NO_EXCEPTIONS } catch (...) { - promise.reportException(std::current_exception()); + promise.setException(std::current_exception()); } #endif - promise.reportFinished(); + promise.finish(); } template<typename Function, typename ResultType, typename ParentResultType> @@ -516,17 +515,17 @@ bool Continuation<Function, ResultType, ParentResultType>::execute() // the user may want to catch the exception inside the continuation, to not // interrupt the continuation chain, so don't report anything yet. if constexpr (!std::is_invocable_v<std::decay_t<Function>, QFuture<ParentResultType>>) { - promise.reportStarted(); - promise.reportException(parentFuture.d.exceptionStore().exception()); - promise.reportFinished(); + promise.start(); + promise.setException(parentFuture.d.exceptionStore().exception()); + promise.finish(); return false; } } else #endif { - promise.reportStarted(); - promise.reportCanceled(); - promise.reportFinished(); + promise.start(); + promise.future().cancel(); + promise.finish(); return false; } } @@ -550,7 +549,7 @@ template<typename Function, typename ResultType, typename ParentResultType> template<typename F> void Continuation<Function, ResultType, ParentResultType>::create(F &&func, QFuture<ParentResultType> *f, - QFutureInterface<ResultType> &p, + QFutureInterface<ResultType> &fi, QtFuture::Launch policy) { Q_ASSERT(f); @@ -564,22 +563,24 @@ void Continuation<Function, ResultType, ParentResultType>::create(F &&func, // If the parent future was using a custom thread pool, inherit it as well. if (launchAsync && f->d.threadPool()) { pool = f->d.threadPool(); - p.setThreadPool(pool); + fi.setThreadPool(pool); } } - p.setLaunchAsync(launchAsync); + fi.setLaunchAsync(launchAsync); - auto continuation = [func = std::forward<F>(func), p, pool, + auto continuation = [func = std::forward<F>(func), fi, promise = QPromise(fi), pool, launchAsync](const QFutureInterfaceBase &parentData) mutable { const auto parent = QFutureInterface<ParentResultType>(parentData).future(); Continuation<Function, ResultType, ParentResultType> *continuationJob = nullptr; if (launchAsync) { - continuationJob = new AsyncContinuation<Function, ResultType, ParentResultType>( - std::forward<Function>(func), parent, p, pool); + auto asyncJob = new AsyncContinuation<Function, ResultType, ParentResultType>( + std::forward<Function>(func), parent, std::move(promise), pool); + fi.setRunnable(asyncJob); + continuationJob = asyncJob; } else { continuationJob = new SyncContinuation<Function, ResultType, ParentResultType>( - std::forward<Function>(func), parent, p); + std::forward<Function>(func), parent, std::move(promise)); } bool isLaunched = continuationJob->execute(); @@ -591,29 +592,26 @@ void Continuation<Function, ResultType, ParentResultType>::create(F &&func, continuationJob = nullptr; } }; - if constexpr (!std::is_copy_constructible_v<Function>) - f->d.setContinuation(ContinuationWrapper(std::move(continuation)), p.d); - else - f->d.setContinuation(std::move(continuation), p.d); + f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d); } template<typename Function, typename ResultType, typename ParentResultType> template<typename F> void Continuation<Function, ResultType, ParentResultType>::create(F &&func, QFuture<ParentResultType> *f, - QFutureInterface<ResultType> &p, + QFutureInterface<ResultType> &fi, QThreadPool *pool) { Q_ASSERT(f); - p.setLaunchAsync(true); - p.setThreadPool(pool); + fi.setLaunchAsync(true); + fi.setThreadPool(pool); - auto continuation = [func = std::forward<F>(func), p, + auto continuation = [func = std::forward<F>(func), promise = QPromise(fi), pool](const QFutureInterfaceBase &parentData) mutable { const auto parent = QFutureInterface<ParentResultType>(parentData).future(); auto continuationJob = new AsyncContinuation<Function, ResultType, ParentResultType>( - std::forward<Function>(func), parent, p, pool); + std::forward<Function>(func), parent, std::move(promise), pool); bool isLaunched = continuationJob->execute(); // If continuation is successfully launched, AsyncContinuation will be deleted // by the QThreadPool which has started it. @@ -622,37 +620,32 @@ void Continuation<Function, ResultType, ParentResultType>::create(F &&func, continuationJob = nullptr; } }; - - if constexpr (!std::is_copy_constructible_v<Function>) - f->d.setContinuation(ContinuationWrapper(std::move(continuation)), p.d); - else - f->d.setContinuation(std::move(continuation), p.d); + f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d); } template<typename Function, typename ResultType, typename ParentResultType> template<typename F> void Continuation<Function, ResultType, ParentResultType>::create(F &&func, QFuture<ParentResultType> *f, - QFutureInterface<ResultType> &p, + QFutureInterface<ResultType> &fi, QObject *context) { Q_ASSERT(f); - auto continuation = [func = std::forward<F>(func), p, context = QPointer<QObject>(context)]( + auto continuation = [func = std::forward<F>(func), promise = QPromise(fi), + context = QPointer<QObject>(context)]( const QFutureInterfaceBase &parentData) mutable { Q_ASSERT(context); const auto parent = QFutureInterface<ParentResultType>(parentData).future(); - QMetaObject::invokeMethod(context, [func = std::forward<F>(func), p, parent]() mutable { - SyncContinuation<Function, ResultType, ParentResultType> continuationJob( - std::forward<Function>(func), parent, p); - continuationJob.execute(); - }); + QMetaObject::invokeMethod( + context, + [func = std::forward<F>(func), promise = std::move(promise), parent]() mutable { + SyncContinuation<Function, ResultType, ParentResultType> continuationJob( + std::forward<Function>(func), parent, std::move(promise)); + continuationJob.execute(); + }); }; - - if constexpr (!std::is_copy_constructible_v<Function>) - f->d.setContinuation(ContinuationWrapper(std::move(continuation)), p.d); - else - f->d.setContinuation(std::move(continuation), p.d); + f->d.setContinuation(ContinuationWrapper(std::move(continuation)), fi.d); } template<typename Function, typename ResultType, typename ParentResultType> @@ -686,32 +679,27 @@ template<typename Function, typename ResultType, typename ParentResultType> template<class... Args> void Continuation<Function, ResultType, ParentResultType>::fulfillPromise(Args &&... args) { - if constexpr (std::is_copy_constructible_v<ResultType>) - promise.reportResult(std::invoke(function, std::forward<Args>(args)...)); - else - promise.reportAndMoveResult(std::invoke(function, std::forward<Args>(args)...)); + promise.addResult(std::invoke(function, std::forward<Args>(args)...)); } template<class T> -void fulfillPromise(QFutureInterface<T> &promise, QFuture<T> &future) +void fulfillPromise(QPromise<T> &promise, QFuture<T> &future) { if constexpr (!std::is_void_v<T>) { if constexpr (std::is_copy_constructible_v<T>) - promise.reportResult(future.result()); + promise.addResult(future.result()); else - promise.reportAndMoveResult(future.takeResult()); + promise.addResult(future.takeResult()); } } template<class T, class Function> -void fulfillPromise(QFutureInterface<T> &promise, Function &&handler) +void fulfillPromise(QPromise<T> &promise, Function &&handler) { if constexpr (std::is_void_v<T>) handler(); - else if constexpr (std::is_copy_constructible_v<T>) - promise.reportResult(handler()); else - promise.reportAndMoveResult(handler()); + promise.addResult(handler()); } #ifndef QT_NO_EXCEPTIONS @@ -719,49 +707,44 @@ void fulfillPromise(QFutureInterface<T> &promise, Function &&handler) template<class Function, class ResultType> template<class F> void FailureHandler<Function, ResultType>::create(F &&function, QFuture<ResultType> *future, - const QFutureInterface<ResultType> &promise) + const QFutureInterface<ResultType> &fi) { Q_ASSERT(future); - auto failureContinuation = [function = std::forward<F>(function), - promise](const QFutureInterfaceBase &parentData) mutable { + auto failureContinuation = [function = std::forward<F>(function), promise = QPromise(fi)]( + const QFutureInterfaceBase &parentData) mutable { const auto parent = QFutureInterface<ResultType>(parentData).future(); FailureHandler<Function, ResultType> failureHandler(std::forward<Function>(function), - parent, promise); + parent, std::move(promise)); failureHandler.run(); }; - if constexpr (!std::is_copy_constructible_v<Function>) - future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation))); - else - future->d.setContinuation(std::move(failureContinuation)); + future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation))); } template<class Function, class ResultType> template<class F> void FailureHandler<Function, ResultType>::create(F &&function, QFuture<ResultType> *future, - QFutureInterface<ResultType> &promise, + QFutureInterface<ResultType> &fi, QObject *context) { Q_ASSERT(future); auto failureContinuation = - [function = std::forward<F>(function), promise, + [function = std::forward<F>(function), promise = QPromise(fi), context = QPointer<QObject>(context)](const QFutureInterfaceBase &parentData) mutable { Q_ASSERT(context); const auto parent = QFutureInterface<ResultType>(parentData).future(); - QMetaObject::invokeMethod( - context, [function = std::forward<F>(function), promise, parent]() mutable { - FailureHandler<Function, ResultType> failureHandler( - std::forward<Function>(function), parent, promise); - failureHandler.run(); - }); + QMetaObject::invokeMethod(context, + [function = std::forward<F>(function), + promise = std::move(promise), parent]() mutable { + FailureHandler<Function, ResultType> failureHandler( + std::forward<Function>(function), parent, std::move(promise)); + failureHandler.run(); + }); }; - if constexpr (!std::is_copy_constructible_v<Function>) - future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation))); - else - future->d.setContinuation(std::move(failureContinuation)); + future->d.setContinuation(ContinuationWrapper(std::move(failureContinuation))); } template<class Function, class ResultType> @@ -769,7 +752,7 @@ void FailureHandler<Function, ResultType>::run() { Q_ASSERT(parentFuture.isFinished()); - promise.reportStarted(); + promise.start(); if (parentFuture.d.hasException()) { using ArgType = typename QtPrivate::ArgResolver<Function>::First; @@ -781,7 +764,7 @@ void FailureHandler<Function, ResultType>::run() } else { QtPrivate::fulfillPromise(promise, parentFuture); } - promise.reportFinished(); + promise.finish(); } template<class Function, class ResultType> @@ -794,21 +777,17 @@ void FailureHandler<Function, ResultType>::handleException() } catch (const ArgType &e) { try { // Handle exceptions matching with the handler's argument type - if constexpr (std::is_void_v<ResultType>) { + if constexpr (std::is_void_v<ResultType>) handler(e); - } else { - if constexpr (std::is_copy_constructible_v<ResultType>) - promise.reportResult(handler(e)); - else - promise.reportAndMoveResult(handler(e)); - } + else + promise.addResult(handler(e)); } catch (...) { - promise.reportException(std::current_exception()); + promise.setException(std::current_exception()); } } catch (...) { // Exception doesn't match with handler's argument type, propagate // the exception to be handled later. - promise.reportException(std::current_exception()); + promise.setException(std::current_exception()); } } @@ -822,7 +801,7 @@ void FailureHandler<Function, ResultType>::handleAllExceptions() try { QtPrivate::fulfillPromise(promise, std::forward<Function>(handler)); } catch (...) { - promise.reportException(std::current_exception()); + promise.setException(std::current_exception()); } } } @@ -834,68 +813,55 @@ class CanceledHandler { public: template<class F = Function> - static QFuture<ResultType> create(F &&handler, QFuture<ResultType> *future, - QFutureInterface<ResultType> &promise) + static void create(F &&handler, QFuture<ResultType> *future, QFutureInterface<ResultType> &fi) { Q_ASSERT(future); - auto canceledContinuation = [promise, handler = std::forward<F>(handler)]( + auto canceledContinuation = [promise = QPromise(fi), handler = std::forward<F>(handler)]( const QFutureInterfaceBase &parentData) mutable { auto parentFuture = QFutureInterface<ResultType>(parentData).future(); - run(std::forward<F>(handler), parentFuture, promise); + run(std::forward<F>(handler), parentFuture, std::move(promise)); }; - - if constexpr (!std::is_copy_constructible_v<Function>) - future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation))); - else - future->d.setContinuation(std::move(canceledContinuation)); - - return promise.future(); + future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation))); } template<class F = Function> - static QFuture<ResultType> create(F &&handler, QFuture<ResultType> *future, - QFutureInterface<ResultType> &promise, QObject *context) + static void create(F &&handler, QFuture<ResultType> *future, QFutureInterface<ResultType> &fi, + QObject *context) { Q_ASSERT(future); - - auto canceledContinuation = [promise, handler = std::forward<F>(handler), + auto canceledContinuation = [promise = QPromise(fi), handler = std::forward<F>(handler), context = QPointer<QObject>(context)]( const QFutureInterfaceBase &parentData) mutable { Q_ASSERT(context); auto parentFuture = QFutureInterface<ResultType>(parentData).future(); - QMetaObject::invokeMethod( - context, [promise, parentFuture, handler = std::forward<F>(handler)]() mutable { - run(std::forward<F>(handler), parentFuture, promise); - }); + QMetaObject::invokeMethod(context, + [promise = std::move(promise), parentFuture, + handler = std::forward<F>(handler)]() mutable { + run(std::forward<F>(handler), parentFuture, std::move(promise)); + }); }; - if constexpr (!std::is_copy_constructible_v<Function>) - future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation))); - else - future->d.setContinuation(std::move(canceledContinuation)); - - return promise.future(); + future->d.setContinuation(ContinuationWrapper(std::move(canceledContinuation))); } template<class F = Function> - static void run(F &&handler, QFuture<ResultType> &parentFuture, - QFutureInterface<ResultType> &promise) + static void run(F &&handler, QFuture<ResultType> &parentFuture, QPromise<ResultType> &&promise) { - promise.reportStarted(); + promise.start(); if (parentFuture.isCanceled()) { #ifndef QT_NO_EXCEPTIONS if (parentFuture.d.hasException()) { // Propagate the exception to the result future - promise.reportException(parentFuture.d.exceptionStore().exception()); + promise.setException(parentFuture.d.exceptionStore().exception()); } else { try { #endif QtPrivate::fulfillPromise(promise, std::forward<F>(handler)); #ifndef QT_NO_EXCEPTIONS } catch (...) { - promise.reportException(std::current_exception()); + promise.setException(std::current_exception()); } } #endif @@ -903,7 +869,7 @@ public: QtPrivate::fulfillPromise(promise, parentFuture); } - promise.reportFinished(); + promise.finish(); } }; |