From 731ba8ed08f80644b403556638c7f6229e678ebe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Str=C3=B8mme?= Date: Thu, 25 Oct 2012 12:32:52 +0200 Subject: [PATCH] Fix for leak in QFuture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To avoid leaking when converting a QFuture to a QFuture we need to have a separate ref. counter for QFuture. When the last QFuture goes out of scope, we need to clean out the result data. Task-number: QTBUG-27224 Change-Id: I965a64a11fffbb191ab979cdd030a9aafd4436c2 Reviewed-by: Jędrzej Nowacki --- src/corelib/thread/qfutureinterface.cpp | 10 +++++ src/corelib/thread/qfutureinterface.h | 15 +++++-- src/corelib/thread/qfutureinterface_p.h | 26 ++++++++++- .../qtconcurrentmap/tst_qtconcurrentmap.cpp | 43 +++++++++++++++++++ .../corelib/thread/qfuture/tst_qfuture.cpp | 30 +++++++++---- 5 files changed, 111 insertions(+), 13 deletions(-) diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index ce9eb143d79..e85f632fdf3 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -419,6 +419,16 @@ bool QFutureInterfaceBase::referenceCountIsOne() const return d->refCount.load() == 1; } +bool QFutureInterfaceBase::refT() const +{ + return d->refCount.refT(); +} + +bool QFutureInterfaceBase::derefT() const +{ + return d->refCount.derefT(); +} + QFutureInterfaceBasePrivate::QFutureInterfaceBasePrivate(QFutureInterfaceBase::State initialState) : refCount(1), m_progressValue(0), m_progressMinimum(0), m_progressMaximum(0), state(initialState), pendingResults(0), diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index b7bf7794123..3cbb9506ecc 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/src/corelib/thread/qfutureinterface.h @@ -130,6 +130,8 @@ public: protected: bool referenceCountIsOne() const; + bool refT() const; + bool derefT() const; public: #ifndef QFUTURE_TEST @@ -148,13 +150,17 @@ class QFutureInterface : public QFutureInterfaceBase public: QFutureInterface(State initialState = NoState) : QFutureInterfaceBase(initialState) - { } + { + refT(); + } QFutureInterface(const QFutureInterface &other) : QFutureInterfaceBase(other) - { } + { + refT(); + } ~QFutureInterface() { - if (referenceCountIsOne()) + if (!derefT()) resultStore().clear(); } @@ -163,7 +169,8 @@ public: QFutureInterface &operator=(const QFutureInterface &other) { - if (referenceCountIsOne()) + other.refT(); + if (!derefT()) resultStore().clear(); QFutureInterfaceBase::operator=(other); return *this; diff --git a/src/corelib/thread/qfutureinterface_p.h b/src/corelib/thread/qfutureinterface_p.h index a9081d4c897..ece5e56768e 100644 --- a/src/corelib/thread/qfutureinterface_p.h +++ b/src/corelib/thread/qfutureinterface_p.h @@ -129,7 +129,31 @@ class QFutureInterfaceBasePrivate public: QFutureInterfaceBasePrivate(QFutureInterfaceBase::State initialState); - QAtomicInt refCount; + // When the last QFuture reference is removed, we need to make + // sure that data stored in the ResultStore is cleaned out. + // Since QFutureInterfaceBasePrivate can be shared between QFuture + // and QFuture objects, we use a separate ref. counter + // to keep track of QFuture objects. + class RefCount + { + public: + inline RefCount(int r = 0, int rt = 0) + : m_refCount(r), m_refCountT(rt) {} + // Default ref counter for QFIBP + inline bool ref() { return m_refCount.ref(); } + inline bool deref() { return m_refCount.deref(); } + inline int load() const { return m_refCount.load(); } + // Ref counter for type T + inline bool refT() { return m_refCountT.ref(); } + inline bool derefT() { return m_refCountT.deref(); } + inline int loadT() const { return m_refCountT.load(); } + + private: + QAtomicInt m_refCount; + QAtomicInt m_refCountT; + }; + + RefCount refCount; mutable QMutex m_mutex; QWaitCondition waitCondition; QList outputConnections; diff --git a/tests/auto/concurrent/qtconcurrentmap/tst_qtconcurrentmap.cpp b/tests/auto/concurrent/qtconcurrentmap/tst_qtconcurrentmap.cpp index c50e3839b51..b6bc5b085bf 100644 --- a/tests/auto/concurrent/qtconcurrentmap/tst_qtconcurrentmap.cpp +++ b/tests/auto/concurrent/qtconcurrentmap/tst_qtconcurrentmap.cpp @@ -43,6 +43,7 @@ #include #include +#include #include @@ -75,6 +76,7 @@ private slots: void stlContainers(); void qFutureAssignmentLeak(); void stressTest(); + void persistentResultTest(); public slots: void throttling(); }; @@ -2395,5 +2397,46 @@ void tst_QtConcurrentMap::stressTest() } } +struct LockedCounter +{ + LockedCounter(QMutex *mutex, QAtomicInt *ai) + : mtx(mutex), + ref(ai) {} + + typedef int result_type; + int operator()(int x) + { + QMutexLocker locker(mtx); + ref->ref(); + return ++x; + } + + QMutex *mtx; + QAtomicInt *ref; +}; + +// The Thread engine holds the last reference +// to the QFuture, so this should not leak +// or fail. +void tst_QtConcurrentMap::persistentResultTest() +{ + QFuture voidFuture; + QMutex mtx; + QAtomicInt ref; + LockedCounter lc(&mtx, &ref); + QList list; + { + list << 1 << 2 << 3; + mtx.lock(); + QFuture future = QtConcurrent::mapped(list + ,lc); + voidFuture = future; + } + QCOMPARE(ref.loadAcquire(), 0); + mtx.unlock(); // Unblock + voidFuture.waitForFinished(); + QCOMPARE(ref.loadAcquire(), 3); +} + QTEST_MAIN(tst_QtConcurrentMap) #include "tst_qtconcurrentmap.moc" diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp index 81ba1b0fd9a..6d73755cfc0 100644 --- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp +++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp @@ -1251,18 +1251,32 @@ void tst_QFuture::throttling() void tst_QFuture::voidConversions() { - QFutureInterface iface; - iface.reportStarted(); + { + QFutureInterface iface; + iface.reportStarted(); - QFuture intFuture(&iface); + QFuture intFuture(&iface); + int value = 10; + iface.reportFinished(&value); - int value = 10; - iface.reportFinished(&value); + QFuture voidFuture(intFuture); + voidFuture = intFuture; - QFuture voidFuture(intFuture); - voidFuture = intFuture; + QVERIFY(voidFuture == intFuture); + } - QVERIFY(voidFuture == intFuture); + { + QFuture voidFuture; + { + QFutureInterface > iface; + iface.reportStarted(); + + QFuture > listFuture(&iface); + iface.reportResult(QList() << 1 << 2 << 3); + voidFuture = listFuture; + } + QCOMPARE(voidFuture.resultCount(), 0); + } }