From 24dedaeaa1a94bfe9ade2da2a2c9aa112241b07a Mon Sep 17 00:00:00 2001 From: Sona Kurazyan Date: Fri, 9 Sep 2022 16:08:33 +0200 Subject: [PATCH] QFuture: fix handling of cancelled continuation chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To support cancellation of continuations attached via the parent future, for each future returned by a continuation we store a pointer to its parent (i.e. future the continuation is attached to). Later, before executing a continuation, we go through chain of parents and check if any of them is cancelled. However, if one of the parents is destroyed while the chain is executing, the next continuations' parent pointers will become invalid. So storing the parent pointers isn't safe. This commit changes the logic of handling the cancelled continuation chain in the following way: - Instead of storing a parent pointer in the continuation future's data, we do the opposite: we store a pointer to continuation's future in the parent. - When a future is cancelled, we mark all continuation futures in the chain with a flag indicating that the chain is cancelled. - To guarantee that the pointers to continuation future's data don't become invalid, we clean the continuation (that stores a copy of its future's data and keeps it alive) only when the associated promise is destructed, instead of cleaning it after the continuation is run. Fixes: QTBUG-105182 Fixes: QTBUG-106083 Pick-to: 6.2 6.3 6.4 Change-Id: I48afa98152672c0fc737112be4ca3b1b42f6ed30 Reviewed-by: Qt CI Bot Reviewed-by: Mårten Nordheim --- src/corelib/compat/removed_api.cpp | 18 ----- src/corelib/thread/qfutureinterface.cpp | 57 +++++++++------ src/corelib/thread/qfutureinterface.h | 2 - src/corelib/thread/qfutureinterface_p.h | 5 +- src/corelib/thread/qpromise.h | 1 + .../corelib/thread/qfuture/tst_qfuture.cpp | 71 +++++++++++++++++++ 6 files changed, 113 insertions(+), 41 deletions(-) diff --git a/src/corelib/compat/removed_api.cpp b/src/corelib/compat/removed_api.cpp index f3f741d830d..213b07f6554 100644 --- a/src/corelib/compat/removed_api.cpp +++ b/src/corelib/compat/removed_api.cpp @@ -173,24 +173,6 @@ QCalendar::QCalendar(QLatin1StringView name) #include "qcollator.h" // inline function compare(ptr, n, ptr, n) (for MSVC) -#if QT_CONFIG(future) - -#include "qfutureinterface.h" -#include "private/qfutureinterface_p.h" - -void QFutureInterfaceBase::cleanContinuation() -{ - if (!d) - return; - - // This was called when the associated QPromise was being destroyed, - // but isn't used anymore. - QMutexLocker lock(&d->continuationMutex); - d->continuation = nullptr; -} - -#endif // QT_CONFIG(future) - #include "qhashfunctions.h" size_t qHash(const QByteArray &key, size_t seed) noexcept diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index a82b7af8734..b62ceaf91d3 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -105,6 +105,13 @@ void QFutureInterfaceBase::cancel(QFutureInterfaceBase::CancelMode mode) break; } + // Cancel the continuations chain + QFutureInterfaceBasePrivate *next = d->continuationData; + while (next) { + next->continuationState = QFutureInterfaceBasePrivate::Canceled; + next = next->continuationData; + } + d->waitCondition.wakeAll(); d->pausedWaitCondition.wakeAll(); @@ -816,45 +823,55 @@ void QFutureInterfaceBase::setContinuation(std::functioncontinuationMutex); - if (continuationFutureData) - continuationFutureData->parentData = d; - // If the state is ready, run continuation immediately, // otherwise save it for later. if (isFinished()) { lock.unlock(); func(*this); - } else { - d->continuation = std::move(func); + lock.relock(); } + // Unless the continuation has been cleaned earlier, we have to + // store the move-only continuation, to guarantee that the associated + // future's data stays alive. + if (d->continuationState != QFutureInterfaceBasePrivate::Cleaned) { + d->continuation = std::move(func); + d->continuationData = continuationFutureData; + } +} + +void QFutureInterfaceBase::cleanContinuation() +{ + if (!d) + return; + + QMutexLocker lock(&d->continuationMutex); + d->continuation = nullptr; + d->continuationState = QFutureInterfaceBasePrivate::Cleaned; } void QFutureInterfaceBase::runContinuation() const { QMutexLocker lock(&d->continuationMutex); if (d->continuation) { - auto fn = std::exchange(d->continuation, nullptr); + // Save the continuation in a local function, to avoid calling + // a null std::function below, in case cleanContinuation() is + // called from some other thread right after unlock() below. + auto fn = std::move(d->continuation); lock.unlock(); fn(*this); + + lock.relock(); + // Unless the continuation has been cleaned earlier, we have to + // store the move-only continuation, to guarantee that the associated + // future's data stays alive. + if (d->continuationState != QFutureInterfaceBasePrivate::Cleaned) + d->continuation = std::move(fn); } } bool QFutureInterfaceBase::isChainCanceled() const { - if (isCanceled()) - return true; - - auto parent = d->parentData; - while (parent) { - // If the future is in Canceled state because it had an exception, we want to - // continue checking the chain of parents for cancellation, otherwise if the exception - // is handled inside the chain, it won't be interrupted even though cancellation has - // been requested. - if ((parent->state.loadRelaxed() & Canceled) && !parent->hasException) - return true; - parent = parent->parentData; - } - return false; + return isCanceled() || d->continuationState == QFutureInterfaceBasePrivate::Canceled; } void QFutureInterfaceBase::setLaunchAsync(bool value) diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index 820faa9ec20..eb251c7d142 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/src/corelib/thread/qfutureinterface.h @@ -183,9 +183,7 @@ protected: void setContinuation(std::function func); void setContinuation(std::function func, QFutureInterfaceBasePrivate *continuationFutureData); -#if QT_CORE_REMOVED_SINCE(6, 4) void cleanContinuation(); -#endif void runContinuation() const; void setLaunchAsync(bool value); diff --git a/src/corelib/thread/qfutureinterface_p.h b/src/corelib/thread/qfutureinterface_p.h index ec3517bab36..6258e61de72 100644 --- a/src/corelib/thread/qfutureinterface_p.h +++ b/src/corelib/thread/qfutureinterface_p.h @@ -141,7 +141,10 @@ public: QThreadPool *m_pool = nullptr; // Wrapper for continuation std::function continuation; - QFutureInterfaceBasePrivate *parentData = nullptr; + QFutureInterfaceBasePrivate *continuationData = nullptr; + + enum ContinuationState : quint8 { Default, Canceled, Cleaned }; + std::atomic continuationState { Default }; RefCount refCount = 1; QAtomicInt state; // reads and writes can happen unprotected, both must be atomic diff --git a/src/corelib/thread/qpromise.h b/src/corelib/thread/qpromise.h index d0f6e6ae66e..43c30582be1 100644 --- a/src/corelib/thread/qpromise.h +++ b/src/corelib/thread/qpromise.h @@ -41,6 +41,7 @@ public: d.cancelAndFinish(); // cancel and finalize the state d.runContinuation(); } + d.cleanContinuation(); } // Core QPromise APIs diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp index 638dc380f7c..81120c6f673 100644 --- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp +++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp @@ -2240,6 +2240,26 @@ void tst_QFuture::then() QVERIFY(threadId1 != QThread::currentThreadId()); QVERIFY(threadId2 != QThread::currentThreadId()); } + + // QTBUG-106083 & QTBUG-105182 + { + QThread thread; + thread.start(); + + QObject context; + context.moveToThread(&thread); + + auto future = QtConcurrent::run([] { + return 42; + }).then([] (int result) { + return result + 1; + }).then(&context, [] (int result) { + return result + 1; + }); + QCOMPARE(future.result(), 44); + thread.quit(); + thread.wait(); + } } template @@ -3116,6 +3136,57 @@ void tst_QFuture::cancelContinuations() QCOMPARE(checkpoint, 3); } #endif // QT_NO_EXCEPTIONS + + // Check notifications from QFutureWatcher + { + QPromise p; + auto f = p.future(); + + auto f1 = f.then([] {}); + auto f2 = f1.then([] {}); + + QFutureWatcher watcher1, watcher2; + int state = 0; + QObject::connect(&watcher1, &QFutureWatcher::started, [&] { + QCOMPARE(state, 0); + ++state; + }); + QObject::connect(&watcher1, &QFutureWatcher::canceled, [&] { + QCOMPARE(state, 1); + ++state; + }); + QObject::connect(&watcher1, &QFutureWatcher::finished, [&] { + QCOMPARE(state, 2); + ++state; + }); + QObject::connect(&watcher2, &QFutureWatcher::started, [&] { + QCOMPARE(state, 3); + ++state; + }); + QObject::connect(&watcher2, &QFutureWatcher::canceled, [&] { + QCOMPARE(state, 4); + ++state; + }); + QObject::connect(&watcher2, &QFutureWatcher::finished, [&] { + QCOMPARE(state, 5); + ++state; + }); + + watcher1.setFuture(f1); + watcher2.setFuture(f2); + + p.start(); + f.cancel(); + p.finish(); + + qApp->processEvents(); + + QCOMPARE(state, 6); + QVERIFY(watcher1.isFinished()); + QVERIFY(watcher1.isCanceled()); + QVERIFY(watcher2.isFinished()); + QVERIFY(watcher2.isCanceled()); + } } void tst_QFuture::continuationsWithContext()