From 5d26d40a5596be048be87f309df9264bac741be9 Mon Sep 17 00:00:00 2001 From: Sona Kurazyan Date: Mon, 30 Nov 2020 13:03:41 +0100 Subject: Fix memory leaks in QFuture's continuations There were two issues: - Some of the continuations were allocating memory for the continuation's context dynamically, but deleting the allocated memory only if they were actually invoked. Since the continuations may not be invoked at all, this could cause memory leaks. Fixed by postponing the allocations to the point when the continuations need to be invoked. - In other cases the parent future is captured by copy in the continuation's lambda, which is then saved in the parent. This causes the following problem: the data of the ref-counted parent will be deleted as soon as its last copy gets deleted. But the saved continuation will prevent it from being deleted, since it holds a copy of parent. To break the circular dependency, instead of capturing the parent inside the lambda, we can pass the parent's data directly to continuation when calling it. Fixes: QTBUG-87289 Pick-to: dev 6.0 Change-Id: If340520b68f6e960bc80953ca18b796173d34f7b Reviewed-by: Andrei Golubev Reviewed-by: Ivan Solovev Reviewed-by: Karsten Heimrich --- src/corelib/thread/qfuture_impl.h | 50 ++++++++++++++++++--------------- src/corelib/thread/qfutureinterface.cpp | 6 ++-- src/corelib/thread/qfutureinterface.h | 5 +++- src/corelib/thread/qfutureinterface_p.h | 2 +- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/corelib/thread/qfuture_impl.h b/src/corelib/thread/qfuture_impl.h index 2d3ea148e9..4998fa79aa 100644 --- a/src/corelib/thread/qfuture_impl.h +++ b/src/corelib/thread/qfuture_impl.h @@ -464,18 +464,20 @@ void Continuation::create(F &&func, } } - Continuation *continuationJob = nullptr; - if (launchAsync) { - continuationJob = new AsyncContinuation( - std::forward(func), *f, p, pool); - } else { - continuationJob = new SyncContinuation( - std::forward(func), *f, p); - } - p.setLaunchAsync(launchAsync); - auto continuation = [continuationJob, launchAsync]() mutable { + auto continuation = [func = std::forward(func), p, pool, + launchAsync](const QFutureInterfaceBase &parentData) mutable { + const auto parent = QFutureInterface(parentData).future(); + Continuation *continuationJob = nullptr; + if (launchAsync) { + continuationJob = new AsyncContinuation( + std::forward(func), parent, p, pool); + } else { + continuationJob = new SyncContinuation( + std::forward(func), parent, p); + } + bool isLaunched = continuationJob->execute(); // If continuation is successfully launched, AsyncContinuation will be deleted // by the QThreadPool which has started it. Synchronous continuation will be @@ -498,12 +500,14 @@ void Continuation::create(F &&func, { Q_ASSERT(f); - auto continuationJob = new AsyncContinuation( - std::forward(func), *f, p, pool); p.setLaunchAsync(true); p.setThreadPool(pool); - auto continuation = [continuationJob]() mutable { + auto continuation = [func = std::forward(func), p, + pool](const QFutureInterfaceBase &parentData) mutable { + const auto parent = QFutureInterface(parentData).future(); + auto continuationJob = new AsyncContinuation( + std::forward(func), parent, p, pool); bool isLaunched = continuationJob->execute(); // If continuation is successfully launched, AsyncContinuation will be deleted // by the QThreadPool which has started it. @@ -584,12 +588,12 @@ void FailureHandler::create(F &&function, QFuture *failureHandler = - new FailureHandler(std::forward(function), *future, promise); - - auto failureContinuation = [failureHandler]() mutable { - failureHandler->run(); - delete failureHandler; + auto failureContinuation = [function = std::forward(function), + promise](const QFutureInterfaceBase &parentData) mutable { + const auto parent = QFutureInterface(parentData).future(); + FailureHandler failureHandler(std::forward(function), + parent, promise); + failureHandler.run(); }; future->d.setContinuation(std::move(failureContinuation)); @@ -664,12 +668,14 @@ class CanceledHandler public: template static QFuture create(F &&handler, QFuture *future, - QFutureInterface promise) + QFutureInterface &promise) { Q_ASSERT(future); - auto canceledContinuation = [parentFuture = *future, promise, - handler = std::forward(handler)]() mutable { + auto canceledContinuation = [promise, handler = std::forward(handler)]( + const QFutureInterfaceBase &parentData) mutable { + auto parentFuture = QFutureInterface(parentData).future(); + promise.reportStarted(); if (parentFuture.isCanceled()) { diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index 57ddc7407e..7a8ba08103 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -744,14 +744,14 @@ void QFutureInterfaceBasePrivate::setState(QFutureInterfaceBase::State newState) state.storeRelaxed(newState); } -void QFutureInterfaceBase::setContinuation(std::function func) +void QFutureInterfaceBase::setContinuation(std::function func) { QMutexLocker lock(&d->continuationMutex); // If the state is ready, run continuation immediately, // otherwise save it for later. if (isFinished()) { lock.unlock(); - func(); + func(*this); } else { d->continuation = std::move(func); } @@ -762,7 +762,7 @@ void QFutureInterfaceBase::runContinuation() const QMutexLocker lock(&d->continuationMutex); if (d->continuation) { lock.unlock(); - d->continuation(); + d->continuation(*this); } } diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index 8467047809..2432503cf7 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/src/corelib/thread/qfutureinterface.h @@ -192,7 +192,7 @@ private: #endif protected: - void setContinuation(std::function func); + void setContinuation(std::function func); void runContinuation() const; void setLaunchAsync(bool value); @@ -215,6 +215,7 @@ public: { refT(); } + QFutureInterface(const QFutureInterfaceBase &dd) : QFutureInterfaceBase(dd) { refT(); } ~QFutureInterface() { if (!derefT()) @@ -424,6 +425,8 @@ public: : QFutureInterfaceBase(initialState) { } + QFutureInterface(const QFutureInterfaceBase &dd) : QFutureInterfaceBase(dd) { } + static QFutureInterface canceledResult() { return QFutureInterface(State(Started | Finished | Canceled)); } diff --git a/src/corelib/thread/qfutureinterface_p.h b/src/corelib/thread/qfutureinterface_p.h index 083d6d3962..df71d3d4b6 100644 --- a/src/corelib/thread/qfutureinterface_p.h +++ b/src/corelib/thread/qfutureinterface_p.h @@ -195,7 +195,7 @@ public: void setState(QFutureInterfaceBase::State state); // Wrapper for continuation - std::function continuation; + std::function continuation; QBasicMutex continuationMutex; bool launchAsync = false; -- cgit v1.2.3