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

@@ -75,8 +75,7 @@ public: // STATIC
 
 protected:
     friend struct QtPromisePrivate::PromiseFulfill<QPromise<T>>;
-    friend class QPromiseResolve<T>;
+    friend class QtPromisePrivate::PromiseResolver<T>;
-    friend class QPromiseReject<T>;
 
     QExplicitlySharedDataPointer<QtPromisePrivate::PromiseData<T>> m_d;
 };

src/qtpromise/qpromise.inl

@@ -9,94 +9,68 @@ template <class T>
 class QPromiseResolve
 {
 public:
-    QPromiseResolve(QPromise<T> p)
+    QPromiseResolve(QtPromisePrivate::PromiseResolver<T> resolver)
-        : m_promise(new QPromise<T>(std::move(p)))
+        : m_resolver(std::move(resolver))
     { }
 
     template <typename V>
     void operator()(V&& value) const
     {
-        Q_ASSERT(!m_promise.isNull());
+        m_resolver.resolve(std::forward<V>(value));
-        if (m_promise->isPending()) {
-            m_promise->m_d->resolve(std::forward<V>(value));
-            m_promise->m_d->dispatch();
     }
-    }
-
-private:
-    QSharedPointer<QPromise<T>> m_promise;
-};
-
-template <>
-class QPromiseResolve<void>
-{
-public:
-    QPromiseResolve(QPromise<void> p)
-        : m_promise(new QPromise<void>(std::move(p)))
-    { }
 
     void operator()() const
     {
-        Q_ASSERT(!m_promise.isNull());
+        m_resolver.resolve();
-        if (m_promise->isPending()) {
-            m_promise->m_d->resolve();
-            m_promise->m_d->dispatch();
-        }
     }
 
 private:
-    QSharedPointer<QPromise<void>> m_promise;
+    mutable QtPromisePrivate::PromiseResolver<T> m_resolver;
 };
 
 template <class T>
 class QPromiseReject
 {
 public:
-    QPromiseReject(QPromise<T> p)
+    QPromiseReject(QtPromisePrivate::PromiseResolver<T> resolver)
-        : m_promise(new QPromise<T>(std::move(p)))
+        : m_resolver(std::move(resolver))
     { }
 
     template <typename E>
     void operator()(E&& error) const
     {
-        Q_ASSERT(!m_promise.isNull());
+        m_resolver.reject(std::forward<E>(error));
-        if (m_promise->isPending()) {
-            m_promise->m_d->reject(std::forward<E>(error));
-            m_promise->m_d->dispatch();
-        }
     }
 
 private:
-    QSharedPointer<QPromise<T>> m_promise;
+    mutable QtPromisePrivate::PromiseResolver<T> m_resolver;
 };
 
 template <typename T>
 template <typename F, typename std::enable_if<QtPromisePrivate::ArgsOf<F>::count == 1, int>::type>
-inline QPromiseBase<T>::QPromiseBase(F resolver)
+inline QPromiseBase<T>::QPromiseBase(F callback)
     : m_d(new QtPromisePrivate::PromiseData<T>())
 {
-    QPromiseResolve<T> resolve(*this);
+    QtPromisePrivate::PromiseResolver<T> resolver(*this);
-    QPromiseReject<T> reject(*this);
 
     try {
-        resolver(resolve);
+        callback(QPromiseResolve<T>(resolver));
     } catch (...) {
-        reject(std::current_exception());
+        resolver.reject(std::current_exception());
     }
 }
 
 template <typename T>
 template <typename F, typename std::enable_if<QtPromisePrivate::ArgsOf<F>::count != 1, int>::type>
-inline QPromiseBase<T>::QPromiseBase(F resolver)
+inline QPromiseBase<T>::QPromiseBase(F callback)
     : m_d(new QtPromisePrivate::PromiseData<T>())
 {
-    QPromiseResolve<T> resolve(*this);
+    QtPromisePrivate::PromiseResolver<T> resolver(*this);
-    QPromiseReject<T> reject(*this);
 
     try {
-        resolver(resolve, reject);
+        callback(QPromiseResolve<T>(resolver), QPromiseReject<T>(resolver));
     } catch (...) {
-        reject(std::current_exception());
+        resolver.reject(std::current_exception());
     }
 }
 

src/qtpromise/qpromise_p.h

@@ -562,6 +562,68 @@ protected:
     }
 };
 
+template <typename T>
+class PromiseResolver
+{
+public:
+    PromiseResolver(QtPromise::QPromise<T> promise)
+        : m_d(new Data())
+    {
+        m_d->promise = new QtPromise::QPromise<T>(std::move(promise));
+    }
+
+    template <typename E>
+    void reject(E&& error)
+    {
+        auto promise = m_d->promise;
+        if (promise) {
+            Q_ASSERT(promise->isPending());
+            promise->m_d->reject(std::forward<E>(error));
+            promise->m_d->dispatch();
+            release();
+        }
+    }
+
+    template <typename V>
+    void resolve(V&& value)
+    {
+        auto promise = m_d->promise;
+        if (promise) {
+            Q_ASSERT(promise->isPending());
+            promise->m_d->resolve(std::forward<V>(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<T>* promise = nullptr;
+    };
+
+    QExplicitlySharedDataPointer<Data> 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

tests/auto/qtpromise/qpromise/construct/tst_construct.cpp

@@ -7,6 +7,9 @@
 // Qt
 #include <QtTest>
 
+// STL
+#include <memory>
+
 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<QObject> object(new QObject());
+
+    std::weak_ptr<int> wptr;
+
+    {
+        auto p = QPromise<std::shared_ptr<int>>([&](
+            const QPromiseResolve<std::shared_ptr<int>>& resolve,
+            const QPromiseReject<std::shared_ptr<int>>& reject) {
+
+            connect(object.data(), &QObject::objectNameChanged,
+                [=, &wptr](const QString& name) {
+                    std::shared_ptr<int> 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<int>()), 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<QObject> object(new QObject());
+
+    std::weak_ptr<int> wptr;
+
+    {
+        auto p = QPromise<int>([&](
+            const QPromiseResolve<int>& resolve,
+            const QPromiseReject<int>& reject) {
+
+            connect(object.data(), &QObject::objectNameChanged,
+                [=, &wptr](const QString& name) {
+                    std::shared_ptr<int> 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<int>()), wptr.lock());
+        QCOMPARE(wptr.use_count(), 1l); // "p" still holds a reference
+    }
+
+    QCOMPARE(wptr.use_count(), 0l);
+}