diff --git a/src/qtpromise/qpromise_p.h b/src/qtpromise/qpromise_p.h index 24ce195..9539161 100644 --- a/src/qtpromise/qpromise_p.h +++ b/src/qtpromise/qpromise_p.h @@ -331,11 +331,12 @@ struct PromiseCatcher template class PromiseData; -template +template class PromiseDataBase : public QSharedData { public: using Error = QtPromise::QPromiseError; + using Handler = std::pair, std::function >; using Catcher = std::pair, std::function >; virtual ~PromiseDataBase() {} @@ -356,6 +357,12 @@ public: return !m_settled; } + void addHandler(std::function handler) + { + QWriteLocker lock(&m_lock); + m_handlers.append({QThread::currentThread(), std::move(handler)}); + } + void addCatcher(std::function catcher) { QWriteLocker lock(&m_lock); @@ -376,15 +383,23 @@ public: return; } - if (m_error.isNull()) { - notify(); - return; - } + // A promise can't be resolved multiple times so once settled, its state can't + // change. When fulfilled, handlers must be called (a single time) and catchers + // ignored indefinitely (or vice-versa when the promise is rejected), so make + // sure to clear both handlers AND catchers when dispatching. This also prevents + // shared pointer circular reference memory leaks when the owning promise is + // captured in the handler and/or catcher lambdas. m_lock.lockForWrite(); + QVector handlers(std::move(m_handlers)); QVector catchers(std::move(m_catchers)); m_lock.unlock(); + if (m_error.isNull()) { + notify(handlers); + return; + } + QSharedPointer error = m_error; Q_ASSERT(!error.isNull()); @@ -406,26 +421,21 @@ protected: m_settled = true; } - virtual void notify() = 0; + virtual void notify(const QVector&) = 0; private: bool m_settled = false; + QVector m_handlers; QVector m_catchers; QSharedPointer m_error; }; template -class PromiseData : public PromiseDataBase +class PromiseData : public PromiseDataBase { - using Handler = std::pair, std::function >; + using Handler = typename PromiseDataBase::Handler; public: - void addHandler(std::function handler) - { - QWriteLocker lock(&this->m_lock); - m_handlers.append({QThread::currentThread(), std::move(handler)}); - } - void resolve(T&& value) { Q_ASSERT(m_value.isNull()); @@ -440,12 +450,8 @@ public: this->setSettled(); } - void notify() Q_DECL_OVERRIDE + void notify(const QVector& handlers) Q_DECL_OVERRIDE { - this->m_lock.lockForWrite(); - QVector handlers(std::move(m_handlers)); - this->m_lock.unlock(); - QSharedPointer value(m_value); Q_ASSERT(!value.isNull()); @@ -458,38 +464,24 @@ public: } private: - QVector m_handlers; QSharedPointer m_value; }; template <> -class PromiseData : public PromiseDataBase +class PromiseData : public PromiseDataBase { - using Handler = std::pair, std::function >; + using Handler = typename PromiseDataBase::Handler; public: - void addHandler(std::function handler) - { - QWriteLocker lock(&m_lock); - m_handlers.append({QThread::currentThread(), std::move(handler)}); - } - void resolve() { setSettled(); } protected: - void notify() Q_DECL_OVERRIDE + void notify(const QVector& handlers) Q_DECL_OVERRIDE { - this->m_lock.lockForWrite(); - QVector handlers(std::move(m_handlers)); - this->m_lock.unlock(); - for (const auto& handler: handlers) { qtpromise_defer(handler.second, handler.first); } } - -private: - QVector m_handlers; }; } // namespace QtPromise