From 933b606af57133d890e64abb708c2c3cf751b915 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Wed, 15 Nov 2023 15:04:01 +0100 Subject: [PATCH] QFutureWatcher: Fix race for initial emission of resultReadyAt() When connecting a QFutureWatcher to the QFuture it will connect to the output interface, which will queue up events to notify about the current state. This happens in the thread of the QFutureWatcher. Since 07d6d31a4c0c17d8c897d783a9b0841df6834b02 unfortunately the sending of those events was done outside the lock, meaning the worker-thread could _also_ send events at the same time, leading to a race on which events would be sent first. To fix this we move the emission of the events back into the lock and because it is now inside the lock again anyway, we will revert back to posting the callout events immediately, so this patch also partially reverts 07d6d31a4c0c17d8c897d783a9b0841df6834b02 Fixes: QTBUG-119169 Pick-to: 6.6 Change-Id: If29ab6712a82e7948c0ea4866340b6fac5aba5ef Reviewed-by: Arno Rehn Reviewed-by: Jarek Kobus (cherry picked from commit 63b2cf8a457f0eea861fa060c610a74b35450ba6) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/thread/qfutureinterface.cpp | 26 +++++++------------ .../qfuturewatcher/tst_qfuturewatcher.cpp | 23 ++++++++++++++++ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index 5fc186b1e45..536467b6cdd 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -806,23 +806,21 @@ void QFutureInterfaceBasePrivate::connectOutputInterface(QFutureCallOutInterface { QMutexLocker locker(&m_mutex); - QVarLengthArray, 3> events; - const auto currentState = state.loadRelaxed(); if (currentState & QFutureInterfaceBase::Started) { - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::Started)); + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::Started)); if (m_progress) { - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::ProgressRange, + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::ProgressRange, m_progress->minimum, m_progress->maximum)); - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::Progress, + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::Progress, m_progressValue, m_progress->text)); } else { - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::ProgressRange, + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::ProgressRange, 0, 0)); - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::Progress, + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::Progress, m_progressValue, QString())); } @@ -833,7 +831,7 @@ void QFutureInterfaceBasePrivate::connectOutputInterface(QFutureCallOutInterface while (it != data.m_results.end()) { const int begin = it.resultIndex(); const int end = begin + it.batchSize(); - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::ResultsReady, + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::ResultsReady, begin, end)); it.batchedAdvance(); @@ -841,21 +839,17 @@ void QFutureInterfaceBasePrivate::connectOutputInterface(QFutureCallOutInterface } if (currentState & QFutureInterfaceBase::Suspended) - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::Suspended)); + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::Suspended)); else if (currentState & QFutureInterfaceBase::Suspending) - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::Suspending)); + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::Suspending)); if (currentState & QFutureInterfaceBase::Canceled) - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::Canceled)); + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::Canceled)); if (currentState & QFutureInterfaceBase::Finished) - events.emplace_back(new QFutureCallOutEvent(QFutureCallOutEvent::Finished)); + iface->postCallOutEvent(QFutureCallOutEvent(QFutureCallOutEvent::Finished)); outputConnections.append(iface); - - locker.unlock(); - for (auto &&event : events) - iface->postCallOutEvent(*event); } void QFutureInterfaceBasePrivate::disconnectOutputInterface(QFutureCallOutInterface *iface) diff --git a/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp b/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp index a5b447470ce..40aa89ded4a 100644 --- a/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp +++ b/tests/auto/corelib/thread/qfuturewatcher/tst_qfuturewatcher.cpp @@ -26,6 +26,7 @@ private slots: void cancelAndFinish(); void resultAt(); void resultReadyAt(); + void orderedResultReadyAt(); void futureSignals(); void watchFinishedFuture(); void watchCanceledFuture(); @@ -280,6 +281,28 @@ void tst_QFutureWatcher::resultReadyAt() QVERIFY(resultSpy.wait()); } +void tst_QFutureWatcher::orderedResultReadyAt() +{ + for (int i = 0; i < 1000; ++i) { + QObject context; + QFuture f = run([](QPromise &fi) { + fi.addResult("First"); + fi.addResult("Second"); + }); + QList actualIndices; + + QFutureWatcher watcher; + connect(&watcher, &QFutureWatcherBase::resultReadyAt, &context, + [&actualIndices](int index) { actualIndices.append(index); }); + watcher.setFuture(f); + f.waitForFinished(); + QCoreApplication::processEvents(); + const QList expectedIndices{0, 1}; + QCOMPARE(actualIndices.size(), expectedIndices.size()); + QCOMPARE(actualIndices, expectedIndices); + } +} + class SignalSlotObject : public QObject { Q_OBJECT