Problems using smart pointers

[naviialto]

#include <QtPromise>
using namespace QtPromise;

typedef QSharedPointer< QNetworkReply > NetworkReplyPtr;
QPromise< NetworkReplyPtr > download(const QUrl& url)
{
	return QPromise<NetworkReplyPtr>( [=](
		const QPromiseResolve< NetworkReplyPtr >& resolve,
		const QPromiseReject< NetworkReplyPtr >& reject) {

		QNetworkReply* reply = NVHttpManager::ptr()->get( QNetworkRequest(url) );
		QObject::connect( reply, &QNetworkReply::destroyed, []() {
			qDebug() << "reply destroyed";
		} );

		QObject::connect( reply, &QNetworkReply::finished, [=]() {
			if( reply->error() == QNetworkReply::NoError ) {
				resolve( NetworkReplyPtr( reply, &QNetworkReply::deleteLater ) );
			} else {
				reject( NetworkReplyPtr( reply, &QNetworkReply::deleteLater ) );
			}
		});
	});
}

int main() {
	download( QUrl( "http://www.google.com" ) )
			  .then( []( NetworkReplyPtr result ) {
				  qDebug() << result->readAll();
				  return result;
			  } )
}

its has error in vs2015

C2668: ambiguous call to overloaded function [duplicate]

So I switched to shared_ptr.
typedef std::shared_ptr< QNetworkReply > NetworkReplyPtr;

its compile is done. but not delete QNetworkReply.

resolve( NetworkReplyPtr( reply, std::bind( &QNetworkReply::deleteLater, reply ) ) );
reject( NetworkReplyPtr( reply, std::bind( &QNetworkReply::deleteLater, reply ) ) );
////
	download( QUrl( "http://www.google.com" ) )
			  .then( []( NetworkReplyPtr result ) {
				  qDebug() << result->readAll();
				  return result;
			  } )
	.finally( []( NetworkReplyPtr result ) {
		qDebug() << result.use_count(); // output 3. not destroy.
	} );

I want to use QSharedPointer to destroy it when QPromise is done.
Please help me.

[simonbrunel]

C2668: ambiguous call to overloaded function [duplicate]

That's interesting, there is an implicit conversion between QSharedPointer<QPromiseError> and QSharedPointer<QNetworkReply>. That's certainly a bug, I will investigate.

.finally([](NetworkReplyPtr result) {

That's also interesting, finally() lambda is not supposed to take any argument because it's called for resolve and reject. It works in your example because you resolve and reject with the same value (which is unusual) but in most other cases, it doesn't make sense and can be really confusing. I may change the finally implementation to prevent this usage.

qDebug() << result.use_count(); // output 3. not destroy.

It outputs 2 (not 3) when I run your example and that's expected because your promise chain is not terminated at this point, so all your promises still exist. When reaching the finally handler, there are 2 references of the shared pointer: one in the promise returned by download and one in the promise returned by then.

That's because resolve (and reject) keep a reference on the associated promise. When you connect your reply &finished signal, both are captured by your lambda and will live as long as it's connected. So for example, if you explicitly disconnect your signal in your slot, then your reply get deleted.

QObject::connect(reply, &QNetworkReply::finished, [=]() {
    reply->disconnect(SIGNAL(finished()));
    if (reply->error() == QNetworkReply::NoError) {
        // ...
});

I will need to find a way to release the captured promise in QPromiseResolve and QPromiseReject once the promise is fully resolved to prevent issues like yours to happen.

[naviialto]

yes. disconnect is right!.
your library is great. I hope that QSharedPointer is supported.
thankyou.

[simonbrunel]

Sounds good! I will try to add support for QSharedPointer as rejection reason. Though, as I said, it's a bit unusual to reject with the same value as resolve. Did you consider the implementation from the docs?

QPromise<QByteArray> download(const QUrl& url)
{
    return QPromise<QByteArray>([&](
        const QPromiseResolve<QByteArray>& resolve,
        const QPromiseReject<QByteArray>& reject) {

        QNetworkReply* reply = manager->get(QNetworkRequest(url));
        QObject::connect(reply, &QNetworkReply::finished, [=]() {
            if (reply->error() == QNetworkReply::NoError) {
                resolve(reply->readAll());
            } else {
                reject(reply->error());
            }

            reply->deleteLater();
        });
    });
}

[pwuertz]

I also stumbled over the problem of the lambda lifetimes in conjunction with QObject::connect. It's too easy to create promises from signals the wrong way due to the fact that Qt signals/connect isn't really designed for the promise pattern. With QNetworkReply it works out fine because the reply object itself is designed as a kind of promise. You usually delete it directly after finished and everything is fine because Qt tears down all connections.

If your source is a persistent QObject and you want to build promises resolved/rejected by its signals, things get a bit more complicated. Without a "QObject sacrifice" for tearing down connections, you have to manually keep track of all signal connections and disconnect them after resolve/reject to free promise references and any other resource you may have captured.

I ended up doing something like this:

// -- promise-returning method --
auto connections = ConnectionGuard::create();
return QPromise<void>([&](const auto& resolve, const auto& reject) {
    // Resolve on signal1
    *connections << connect(this, &Class::signal1, this, [=]() {
        resolve();
    });
    // Reject on signal2
    *connections << connect(this, &Class::signal2, this, [=]() {
        reject();
    });
    // Reject on signal3 if condition is met
    *connections << connect(this, &Class::signal3, this, [=]() {
        if (condition) reject();
    });
}).finally([=]() {
    connections->disconnect();
});

implemented like this pwuertz@09a709e

I don't know if that is the most beautiful solution, but it does the job for quite a few use cases. QtPromise already knows how to transform futures into promises. Maybe it is possible to create an adapter for building promises from signals as well? Something like this?

return QtPromise::fromSignals({
    {object, &Class::resolveSignal1},
    {object, &Class::resolveSignal2},
}, {
    {object, &Class::rejectSignal1},
    {object, &Class::rejectSignal2},
});

[simonbrunel]

@pwuertz I already thought about integrating with Qt signals but didn't find a great API yet. I was thinking of qPromise(QObject* fulfiller, F fsignal, QObject* rejecter, R rsignal). Not sure about the multiple fulfillers/rejecters as in your fromSignals, it may be better to implement a QPromise::race for that case.

The lambda lifetimes issue should be relatively easy to fix though, we need to release the promise in QPromiseResolve and QPromiseReject as soon as the promise is resolved. Not sure yet how to track that because either resolve or reject is called, not both.

[pwuertz]

The way I understand the situation it is that the promise references held by QPromiseResolve and QPromiseReject are not the actual problem. I think the problem lies in the Qt signal connections which have unlimited lifetime until disconnected explicitly. If you just release the QPromise within QPromiseResolve and QPromiseReject, wouldn't the lambda that lives within the signal connection still hold on to QPromiseResolve, QPromiseReject and any other captured resource?

If I make sure that all connections tied to the promise are disconnected after resolve/reject, the QPromiseResolve, QPromiseReject and thus QPromise references are automatically released. So no further action should be required.

If this is true, this also means that you cannot construct a race of signals by composition of promises. A race-promise made from N promises from N rejection signals rejects if any one signal fires, but it has no means of disconnecting the other N-1 promises from their signals which may never fire, thus loosing resources at every race. A single promise built from N signals however just stores and disconnects all N connections once the race finished.

Maybe this is a situation where an intrinsic cancellation mechanism is required, but from what I read the people doing the promise specs came to the conclusion that a generic cancellation method is difficult/impossible to define.

Maybe I'm completely wrong on this since these promise patterns are quite new for me. But please tell me if such discussions go beyond the scope of what you are willing to invest in this project (which is indeed nicely done!).

[simonbrunel]

... QPromiseResolve and QPromiseReject are not the actual problem

I think it is: these are the only two objects, related to the generated promise, that can be captured in the signal lambda, so if we clear the internal promises when one of them get called, no more reference on the promise data will be held by the connection.

Once fixed, the race approach would work, but you right, it's not the same as handling multiple signals in only one promise. The cancellation feature is already in my mind but it's not an easy one (I would like to implement the bluebird "don't care" approach).

... tell me if such discussions go beyond the scope of what you are willing to invest in this project

It's fine, that's good for a project to receive feedback and external point of views. Though, it may be better to kickoff these discussions in separate tickets. I have a pretty long todo list on which many items are waiting for a decent API (and of course the time to implement it). However, there are a few features I will not implement, such as deferred or progress.

[simonbrunel]

Both issues reported in this ticket should now be resolved and will be released in version 0.4

• 7b0cba5 QSharedPointer as rejection reason is now supported
• fa987a5 QPromiseResolve/Reject now release their promise when resolved

@pwuertz would you mind to checkout master and verify if fa987a5 fixes the situation you described in this comment: the promise (and thus its data) should now be released as soon as it get resolved, even if the QPromiseResolve and QPromiseReject are captured in the (still alive) connection.

[pwuertz]

I can verify that this does not solve the problem I was referring to, and you seem to be aware of it mentioning that the signal connection used for resolving the promise is still alive.

My point was that this connection keeps QPromiseResolve, QPromiseReject and any other resource captured by the lambda tied to the lifetime of the signal source. So it is very easy to produce memory leaks or get into other troubles if resources captured by the lambda are used in a RAII pattern. Solving this problem would have made fa987a5 unnecessary or a far less pressing concern.

I'll create a new issue for discussions about qt-signal-helper ideas.

[simonbrunel]

I agree, we need an API to integrate with Qt signals since resources captured by the lambda will not be released as long as the connection lives (which is your point). fa987a5 wasn't intended to fix it but only the reported issue which was that the QNetwork shared pointer wasn't released when the promise was resolved and gone.

You right, auto-disconnecting would fix that issue too but it doesn't only concern signal/slot but all cases where QPromiseResolve and/or QPromiseReject are captured (e.g. a deferred implementation). So I don't think fa987a5 is useless since it enhances memory management in case the user keeps references on QPromiseResolve and/or QPromiseReject after the promise is resolved.

+1 for a qt-signal-helper specific ticket.

[simonbrunel]

Released in 0.4.0