From fa987a50442e4b982864ce4e083f44e3c046e163 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Mon, 7 May 2018 10:46:00 +0200 Subject: [PATCH] Cleanup promise captured in resolve/reject Make sure that QPromiseResolve and QPromiseReject release their associated promise as soon as one of them is resolved or rejected, ensuring that the promise data is cleaned up once resolved (that fixes cases where one or both of them are captured in a signal/slot connection lambda) --- src/qtpromise/qpromise.h | 3 +- src/qtpromise/qpromise.inl | 60 ++++---------- src/qtpromise/qpromise_p.h | 62 +++++++++++++++ .../qpromise/construct/tst_construct.cpp | 79 +++++++++++++++++++ 4 files changed, 159 insertions(+), 45 deletions(-) diff --git a/src/qtpromise/qpromise.h b/src/qtpromise/qpromise.h index 2516689..47252f2 100644 --- a/src/qtpromise/qpromise.h +++ b/src/qtpromise/qpromise.h @@ -75,8 +75,7 @@ public: // STATIC protected: friend struct QtPromisePrivate::PromiseFulfill>; - friend class QPromiseResolve; - friend class QPromiseReject; + friend class QtPromisePrivate::PromiseResolver; QExplicitlySharedDataPointer> m_d; }; diff --git a/src/qtpromise/qpromise.inl b/src/qtpromise/qpromise.inl index f5a97db..d270bf3 100644 --- a/src/qtpromise/qpromise.inl +++ b/src/qtpromise/qpromise.inl @@ -9,94 +9,68 @@ template class QPromiseResolve { public: - QPromiseResolve(QPromise p) - : m_promise(new QPromise(std::move(p))) + QPromiseResolve(QtPromisePrivate::PromiseResolver resolver) + : m_resolver(std::move(resolver)) { } template void operator()(V&& value) const { - Q_ASSERT(!m_promise.isNull()); - if (m_promise->isPending()) { - m_promise->m_d->resolve(std::forward(value)); - m_promise->m_d->dispatch(); - } + m_resolver.resolve(std::forward(value)); } -private: - QSharedPointer> m_promise; -}; - -template <> -class QPromiseResolve -{ -public: - QPromiseResolve(QPromise p) - : m_promise(new QPromise(std::move(p))) - { } - void operator()() const { - Q_ASSERT(!m_promise.isNull()); - if (m_promise->isPending()) { - m_promise->m_d->resolve(); - m_promise->m_d->dispatch(); - } + m_resolver.resolve(); } private: - QSharedPointer> m_promise; + mutable QtPromisePrivate::PromiseResolver m_resolver; }; template class QPromiseReject { public: - QPromiseReject(QPromise p) - : m_promise(new QPromise(std::move(p))) + QPromiseReject(QtPromisePrivate::PromiseResolver resolver) + : m_resolver(std::move(resolver)) { } template void operator()(E&& error) const { - Q_ASSERT(!m_promise.isNull()); - if (m_promise->isPending()) { - m_promise->m_d->reject(std::forward(error)); - m_promise->m_d->dispatch(); - } + m_resolver.reject(std::forward(error)); } private: - QSharedPointer> m_promise; + mutable QtPromisePrivate::PromiseResolver m_resolver; }; template template ::count == 1, int>::type> -inline QPromiseBase::QPromiseBase(F resolver) +inline QPromiseBase::QPromiseBase(F callback) : m_d(new QtPromisePrivate::PromiseData()) { - QPromiseResolve resolve(*this); - QPromiseReject reject(*this); + QtPromisePrivate::PromiseResolver resolver(*this); try { - resolver(resolve); + callback(QPromiseResolve(resolver)); } catch (...) { - reject(std::current_exception()); + resolver.reject(std::current_exception()); } } template template ::count != 1, int>::type> -inline QPromiseBase::QPromiseBase(F resolver) +inline QPromiseBase::QPromiseBase(F callback) : m_d(new QtPromisePrivate::PromiseData()) { - QPromiseResolve resolve(*this); - QPromiseReject reject(*this); + QtPromisePrivate::PromiseResolver resolver(*this); try { - resolver(resolve, reject); + callback(QPromiseResolve(resolver), QPromiseReject(resolver)); } catch (...) { - reject(std::current_exception()); + resolver.reject(std::current_exception()); } } diff --git a/src/qtpromise/qpromise_p.h b/src/qtpromise/qpromise_p.h index 6d75d24..723a422 100644 --- a/src/qtpromise/qpromise_p.h +++ b/src/qtpromise/qpromise_p.h @@ -562,6 +562,68 @@ protected: } }; +template +class PromiseResolver +{ +public: + PromiseResolver(QtPromise::QPromise promise) + : m_d(new Data()) + { + m_d->promise = new QtPromise::QPromise(std::move(promise)); + } + + template + void reject(E&& error) + { + auto promise = m_d->promise; + if (promise) { + Q_ASSERT(promise->isPending()); + promise->m_d->reject(std::forward(error)); + promise->m_d->dispatch(); + release(); + } + } + + template + void resolve(V&& value) + { + auto promise = m_d->promise; + if (promise) { + Q_ASSERT(promise->isPending()); + promise->m_d->resolve(std::forward(value)); + promise->m_d->dispatch(); + release(); + } + } + + void resolve() + { + auto promise = m_d->promise; + if (promise) { + Q_ASSERT(promise->isPending()); + promise->m_d->resolve(); + promise->m_d->dispatch(); + release(); + } + } + +private: + struct Data : public QSharedData + { + QtPromise::QPromise* promise = nullptr; + }; + + QExplicitlySharedDataPointer m_d; + + void release() + { + Q_ASSERT(m_d->promise); + Q_ASSERT(!m_d->promise->isPending()); + delete m_d->promise; + m_d->promise = nullptr; + } +}; + } // namespace QtPromise #endif // ifndef QTPROMISE_QPROMISE_H diff --git a/tests/auto/qtpromise/qpromise/construct/tst_construct.cpp b/tests/auto/qtpromise/qpromise/construct/tst_construct.cpp index c4ffc80..b23a287 100644 --- a/tests/auto/qtpromise/qpromise/construct/tst_construct.cpp +++ b/tests/auto/qtpromise/qpromise/construct/tst_construct.cpp @@ -7,6 +7,9 @@ // Qt #include +// STL +#include + using namespace QtPromise; class tst_qpromise_construct : public QObject @@ -30,6 +33,8 @@ private Q_SLOTS: void rejectSync_void(); void rejectAsync(); void rejectAsync_void(); + void connectAndResolve(); + void connectAndReject(); }; QTEST_MAIN(tst_qpromise_construct) @@ -228,3 +233,77 @@ void tst_qpromise_construct::rejectThrowTwoArgs_void() QCOMPARE(waitForValue(p, -1, 42), -1); QCOMPARE(waitForError(p, QString()), QString("foo")); } + +// https://github.com/simonbrunel/qtpromise/issues/6 +void tst_qpromise_construct::connectAndResolve() +{ + QScopedPointer object(new QObject()); + + std::weak_ptr wptr; + + { + auto p = QPromise>([&]( + const QPromiseResolve>& resolve, + const QPromiseReject>& reject) { + + connect(object.data(), &QObject::objectNameChanged, + [=, &wptr](const QString& name) { + std::shared_ptr sptr(new int(42)); + + wptr = sptr; + + if (name == "foobar") { + resolve(sptr); + } else { + reject(42); + } + }); + }); + + QCOMPARE(p.isPending(), true); + + object->setObjectName("foobar"); + + QCOMPARE(waitForValue(p, std::shared_ptr()), wptr.lock()); + QCOMPARE(wptr.use_count(), 1l); // "p" still holds a reference + } + + QCOMPARE(wptr.use_count(), 0l); +} + +// https://github.com/simonbrunel/qtpromise/issues/6 +void tst_qpromise_construct::connectAndReject() +{ + QScopedPointer object(new QObject()); + + std::weak_ptr wptr; + + { + auto p = QPromise([&]( + const QPromiseResolve& resolve, + const QPromiseReject& reject) { + + connect(object.data(), &QObject::objectNameChanged, + [=, &wptr](const QString& name) { + std::shared_ptr sptr(new int(42)); + + wptr = sptr; + + if (name == "foobar") { + reject(sptr); + } else { + resolve(42); + } + }); + }); + + QCOMPARE(p.isPending(), true); + + object->setObjectName("foobar"); + + QCOMPARE(waitForError(p, std::shared_ptr()), wptr.lock()); + QCOMPARE(wptr.use_count(), 1l); // "p" still holds a reference + } + + QCOMPARE(wptr.use_count(), 0l); +}