Promise from Qt Signals helper

[pwuertz]

(Small recap of #6 (comment)): When setting up promises from Qt signals, the signal connections used for resolving are tied to the lifetime of the signal source only, i.e. not to the state of the QPromise they resolve/reject. This is fine for signal sources that are already designed as promises to be deleted after resolve (like QNetworkReply).
However, if a signal source is a persistent object there is no intrinsic mechanism for tearing down signal connections after resolve/reject. This is an easy to exploit memory leak and may cause other problems if the lifetime of other captured resources is crucial to an applications logic (RAII).

Unfortunately, keeping track of signal connections and disconnecting them all at once is kind of an ugly task. I ended up creating ConnectionGuard for conveniently collecting and disconnecting multiple signals in the context of QPromise construction (pwuertz@09a709e):

// -- In 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();
});

The next step would be a high level API for creating a QPromise from resolve and reject signals that automatically connects to and disconnects from signal sources like your suggestion:

qPromise(QObject* fulfiller, F fsignal, QObject* rejecter, R rsignal)

I think this could cover a lot of use cases.

However, in some scenarios I still ended up with stale situations where none of the signals would emit. Possible problems are:

• Signal source might be destroyed before resolve. Could be handled by automatically adding the destroyed signal as rejection.
• Signal source might not emit at all and a timeout is desired. Could be handled by an optional timeout argument or signal.

So it would seem to me that there is a high chance of requiring more than one reason for rejection, which is why I'd be keen on the idea of allowing multiple rejection signals.

I know you prefer the idea of handling such cases by race, but that implies that there must be a cancellation feature as well. This is of course a powerful tool but certainly more difficult to implement and from what I've read cancellation is still a highly discussed topic without general consensus (unlike the A+ pattern).

[simonbrunel]

I'm not sure I would try to handle multiple signals/slots in one helper with a map as suggested in this comment, because all signals would need to have the same signature which may not fit many use cases? We could indeed expose something similar to ConnectionGuard for maximum flexibilty, though, I would maybe use an internal explicit shared data pointer to not deal with a shared pointer:

QPromiseConnections connections;
return QPromise<void>([&](const auto& resolve, const auto& reject) {
    connections << connect(...);
    connections << connect(...);
    // ...
}.finally([=]() {
    connections.disconnect();
});

In case the connections shared data is destroyed with alive connections, we should output a warning.

Could be handled by automatically adding the destroyed signal as rejection.

I think it should reject with a special reason. I may add a new feature, QPromiseContext, that allows to "control" the lifetime of a promise based on a QPointer or QSharedPointer validity. It triggers QPromiseContextException if the pointer is gone so maybe we should use the same exception here.

void QObjectBaseClass::download(const QUrl& url)
{
    auto ctx = QPromiseContext(this);
    manager->request(url)
        .then([ctx](const QByteArray& data) {
            ctx->process(data);  // throws QPromiseContextException if ctx is null 
        });
}

Could be handled by an optional timeout argument or signal.

It could be handled using qPromise(obj, &O::signal).timeout(5000), however connections will not be removed (in this case cancellation would be great). I'm not fan of methods with tons of arguments, adding timeout in qPromise() means a delay and the timeout exception to be consistent with the timeout method. I would not implement that feature in a first time and take time to think more about it.

[pwuertz]

Allright, I hope I addressed your points to get the PR in a more mature state.

You are right in that multiple resolve signals are probably too cumbersome for a general purpose API, so those are off the table. For such corner cases it is indeed much better to give users a tool for handling this conveniently without overcomplicating qPromise().

I renamed the connection guard to QPromiseConnections, which now derives from private std::shared_ptr and thus hides any pointer mechanics from the user. This does indeed look a bit more pleasant now. The class is documented in Manually creating QPromise<T> from signals for low level use cases.

I agree that pushing timeout into the API feels a bit rushed and promise cancellation would be the best way to cleanup after timeout. Thinking about it, QPromise::timeout already is a viable solution if the signal source is either cancellable or destructible. It just requires the user to remember cancelling or destroying it in timeout. Cases where the source is neither cancellable nor destructible are covered by manual promise construction and calling reject & QPromiseConnections::disconnect from a single shot timer. (I could probably add those last 3 sentences to the documentation..)

My previous idea of automatically connecting to destroyed probably isn't that good. Maybe a user considers the destruction of an object as success and the promise needs to resolve, not reject. Maybe the lifetime of the signal source is guaranteed and the destroyed connection is superfluous. Maybe it's best to leave it up to the user to handle this event.

That being said, I think this makes supporting multiple rejection signals important since rejection reasons can be manifold (errors of different types occurring, operation canceled, resource destroyed). In contrast to resolve, multiple rejection signals with different types are naturally mapped to the typed QPromise::fail handlers. An example in the documentation illustrates such a use case.

In between I added support for automatic QPromise<std::tuple> construction from signals with multiple arguments, but had to remove it again since MSVC's template capabilities were lacking behind. It does work in MSVC 2017, but I assume MSVC 2015 support isn't something one likes to drop at this time.

[geiseri]

@pwuertz in my tuples branch I have some helpers in qpromise_p.h that might help. Most of the helpers are around ripping a tuple apart and converting it though. Check there maybe there is something useful in there. That being said I do not know how often there is a signal with more than one argument you would want to wait on.

[pwuertz]

@geiseri This is so weird. I used the exact same construct here, and MSVC < 2017 always choked on it in my tests, stating that it doesn't support expanding the Args pack here. Maybe it's those extra definitions that make it work for MSVC instead of using the std definitions to do the job..

[simonbrunel]

I plan to release 0.4 first, then iterate on the signals PR. Actually, I would like to simplify the API a bit more for a first version. I'm also re-organizing helpers (QtPromise::map instead of qPromiseMap, deprecating qPromise in favor of QtPromise::resolve, etc.) to be more in line with the Bluebird API. So we may need to find another name since ::resolve don't really make sense. Maybe QtPromise::connect ...

I would prefer to keep the std::tuple outside the signal PR to make things easier to review and integrate.

BTW, I'm on the QtMob Slack if you guys prefer a more interactive exchange.

[pwuertz]

Right, I'll simplify and refactor the PR on top of 0.4 once it is settled and get back to you.

[pwuertz]

Ok keeping it simple. I reduced the PR to just the QPromiseConnections helper, test, and documentation on how to create promises from signals manually.

[msarehn]

Is there any update on this? This type of functionality could make our code a lot cleaner.

[simonbrunel]

@msarehn I totally forgot about #13 (sorry for that @pwuertz). I would like to update a few things before merging but most of the work is here so I hope to get it in before hopefully before the end of the month. The API will remain the same, so you can checkout and test this branch to make sure it works as expected.

[pwuertz]

@simonbrunel np ;) Just ping me if there is anything I can help with.

I've been using the signal helpers for quite some time now. For what it's worth, they seem quite solid for given use cases, i.e. no further API changes.