Question about executing context

[durkmurder]

I use qtpromise a lot in my project and I am constantly running into next situation

static QtPromise::QPromise<void> DoSomething()
{
    return QtPromise::resolve().delay(5000);
}
////
struct Foo : public QObject {
    void doSomethingAndDoSomethingElse()
    {
        QPointer<Foo> self(this);
        DoSomething().then([self] {
            if(self) {
                // do something with self.
                qDebug() << "Self is ok";
            }
            else {
                qDebug() << "Foo already deleted";
            }
        });
    }
};

Foo *foo = new Foo;
foo->doSomethingAndDoSomethingElse();
// here foo gets deleted(by deleteLater for instance)

So it can happen that Foo gets deleted before promise is resolved, and since function is called as callback I need to check if this is still valid in some way otherwise I get segfault.
I was thinking a lot about it and it seems like a pretty common thing.
I see 3 ways how to resolve this situation.

1. Check this as I am doing it right now.
2. Store started promises and wait for them in destructor.
3. Try to support same behavior as Qt signal & slots system in terms of executing slot in receivers context.
   I like 3rd case the most but it requires changes to the library(which I can do). I want to discuss here if it makes sense at all and if you see any problems with it.
   So the idea is to provide executing context to continuations, introduce new call like, thenVia(QObject *, callback) or something like then(callback).via(QObject*) which will be used as context for delivering callback. If such functionality exists then my use case will be resolved just by using context for continuations since if context is already deleted, callback won't be executed(same behavior with slots). Implementation wise it shouldn't be that hard to do it, since it requires just some modifications to qtpromise_defer.

Looking forward to discussing it

[simonbrunel]

Hi @durkmurder,

It looks similar to #20 but with a different use case. I also find myself doing this check a lot and I agree that we may need to make it easier to handle. I already thought about your option 3, however, even if it seems the most natural way to implement it, I don't like it for the reason explained in this comment.

---

Option 3 (the one you suggested):

3.1: the context object is needed in all continuation methods (i.e. .then, .fail, .finally, .map, etc.) and I don't think it's a good idea to duplicate (i.e. thenVia, .failVia, etc.) or override (i.e. then(callback, context = nullptr), .fail(callback, context = nullptr), etc.) all those methods.

3.2: then(callback).via(context*) doesn't work because the result of then() depends on callback, however .via(context*).then(callback) could maybe work. Though I'm not sure about that approach because it requires a few internal changes to track this context. And you still need to access the context object inside the callback. We could pass it as an extra argument to all handlers (i.e. .then([](res, context) { ... })) but for the same reason as above, I dislike the idea to modify the signature of all callbacks. Alternatively, you could still capture it in your lambdas:

promise.via(context).then([context](res) { ... })

---

Option 4 (one solution I implemented in my projects):

It's quite similar to your option 1 but using a new QPromiseContext wrapper (with a dedicated ::context() helper). It captures the context from a QObject* or a Qt/STL shared or weak pointer. This QPromiseContext throws a QPromiseContextException if you try to access it when the monitored pointer is already destroyed:

// `this` is a QObject based class with a process() method:

auto ctx = QtPromise::context(this); // or QtPromise::context(std::shared_ptr<T>*), or...
promise.then([ctx](result) {
    ctx->process(result);
})
// ... and optionally ...
.fail([](const QPromiseContextException& e) {
    // handle the destroyed context if needed
})

---

Option 5 (just thought about it while writing this reply):

Maybe we could wrap the callback using a ::bind() helper (or whatever name), and do that check just before calling the callback. I'm not sure how easily it could be implemented but I like the fact that it doesn't change the API of all existing continuation methods and callbacks.

promise.then(QtPromise::bind(this, [](ctx, result) {
    ctx->process(result);  // ctx being `this`
}).fail([](const QPromiseContextException& e) {
    // handle the destroyed context if needed
})

---

To sum up:

• I would eliminate option 3.1 because it complexifies the entire QPromise API,
• I would also eliminate option 3.2 since it requires too many internal changes,
• I like option 4 for its simplicity: easy to use and easy to implement, no API changes,
• I like option 5 (maybe better than option 4) but I would need to evaluate its feasibility.

Thoughts ?

[durkmurder]

I was inspired by Folly futures, they are very flexible in terms of executing context of continuations. I think breaking current API is not desired. For me personally I would like this flow:
promise.via(context).then([context](res) { ... }) it will mean that all callbacks after via will be executed on that context. Just because of simplicity and not breaking the API. I understand that it will need some changes into internals.
Folly supports via() in different way but I like more the way you are describing. Also they support
then(executor, callback) that's also not too bad, but again requires to write it for fails as well because usually you want fail to be handled in same context as then

I am also ok with options 4 and 5(they are kinda the same as I see), but they add a lot of boilerplate even if you do simple .then().fail() and as I understand it really breaks general fail and you will need to handle QPromiseContextException in every promise call.

[simonbrunel]

Folly supports via() in different way ...

Folly .via() seems to define where (thread) the callback is executed, not if (QPointer) the callback should be executed. Your example is about the if and I'm not sure we should try to address both with the same API (while both where and if are interesting features). Let's focus on the if, if it's what you are looking for.

... as I understand it really breaks general fail ...

The empty .fail() will be called if the context is destroyed and the error haven't been previously handled by .fail(QPromiseContextException&)). Whatever solution is adopted, it needs to throw this context exception because some use cases need to recover from a destroyed context and let the calling code continue the promise chain.

... it will mean that all callbacks after via will be executed on that context ...

While usage of options 3.2 and 4 are very similar (see below), with option 3.2, the promise returned by Foo::execute() will never get resolved and the calling code will remain in a pending state indefinitely, which I think it's not desirable.

// 3.2 (.via() renamed to .bind())
QPromise<int> Foo::execute() {
  return promise
    .bind(this)
    .then([this](int res) { this->process(res); })
    .fail([this](int err) { this->handler(err); })
    .finally([this]() { this->finalize(); });
}

vs

// 4
QPromise<int> Foo::execute() {
  auto ctx = QtPromise::context(this);
  return promise
    .then([ctx](int res) { ctx->process(res); })
    .fail([ctx](int err) { ctx->handler(err); })
    .finally([ctx]() { ctx->finalize(); })
    //...
}

[durkmurder]

I should have made it clear earlier, talking about context I meant receiver in terms of signals & slots, so it's not only if it's also where at the same time. My plan was to use QMetaObject::invokeMethod to deal with it. I am not even sure they are should be executed in case context is already deleted, just maybe finalize has to be called no matter what.

4 looks good just the error handling is a little bit too much. You basically get the same problem with general fail you have no way to know if it was context destroyed or something else(from this you don't know if it's safe to use this or no) it will result in writing general fail and placing if inside to check object lifetime or handling context destroyed in such cases(even if you don't need it and just want to be sure that this is valid)

[simonbrunel]

Your first example doesn't reflect the where because the then() callback is a lambda. Do you have another example maybe that would help to understand better your use case?

Option 4 is already implemented in a local branch (just missing the docs). I haven't contributed it yet because I'm still unsure that's the best API, so I'm really interested in feedback / alternatives that work well with all use cases.

[simonbrunel]

@pwuertz @dpurgin any thoughts?

[durkmurder]

Of course. Why I am talking about signals & slot it's because of next behavior
When you use this approach:
connect(obj, &Object::signal, [this] { // do something });
you get kind same behavior as with callbacks in your library. This behavior is dangerous because of unknown lifetime of this and also thread affinity. In this approach slot will be called as direct connection. In case of promises they will be resolved on current thread.
When you use next approach:
connect(obj, &QObject::signal, this, [this] { // do something });
slot will be resolved in thread of receiver object(this in this case) and only if receiver is still valid.

Adding context support with if and where support the same way signals & slots are resolved will give great flexibility.
Instead of

QPromise<int>([context](const auto &resolve, const auto &reject) {
     QMetaObject::invokeMethod(context, [resolve] {
             // do something in another thread
             resolve(42);
     });
}).then([](int result) {
     // handle some result
     // maybe add another invokeMethod to execute next portion on another thread.
});

Something like this can be written:

QPromise<int>::resolve().via(context).then([] { return 42 }).via(anotherContext).then([](int result) { // handle result });

I am not really sure that's the exact syntax, but I do those manipulations with QMetaObject::invokeMethod and QPointer<> self(this) a lot in my code to make promises work in async and multi threaded manner.

[simonbrunel]

Ok, I fully get it now, thanks! So indeed, option 4 doesn't address the where part. Option 5 would work but I agree that it requires more code (though I still like it because it doesn't impact internals).

I would probably pick another name than .via() (what about .bind()?) and we need to be able to control the context of the returned promise in my previous Foo::execute() example. Also, if the context is destroyed, the promise should reject with a QPromiseContextException, same as the QtPromise::connect() helper.

foo->execute()
  .then([] { ... })  // should be call in the current thread, whatever the thread of m_context in execute().
  .fail([] { ... }); // should be call even if m_context used in execute() is destroyed.

Maybe supporting QPromise::bind(nullptr) or QPromise::unbind() would allow such use case:

QPromise<int> Foo::execute() {
  return m_context->asyncCall()
    .bind(m_context)
    .then([m_context](int res) { m_context->process(res); })
    .fail([m_context](int err) { m_context->handler(err); })
    .finally([m_context]() { m_context->finalize(); })
    .unbind();
}

[simonbrunel]

Still thinking about option 5: it's very close to 3.1 and to me, it's less error prone than 3.2 because it doesn't need to unbind() before returning the promise.

QPromise<int> Foo::execute() {
  return m_context->asyncCall()
    .then(QtPromise::bind(m_context, [m_context](int res) { m_context->process(res); }))
    .fail(QtPromise::bind(m_context, [m_context](int err) { m_context->handler(err); }))
    .finally(QtPromise::bind(m_context, [m_context] { m_context->finalize(); }));
}

instead of:

QPromise<int> Foo::execute() {
  return m_context->asyncCall()
    .then(m_context, [m_context](int res) { m_context->process(res); })
    .fail(m_context, [m_context](int err) { m_context->handler(err); })
    .finally(m_context, [m_context] { m_context->finalize(); });
}

QtPromise::bind(QObject* context, F callback) would return a functor that accepts the same arguments and return the same value as callback. When called, the functor executes callback in the same thread as context and only if context is not destroyed, else throws a QPromiseContextException.

[durkmurder]

I agree that .bind( might be a little bit too much in a sense of forgetting to unbind. The main motivation for 5 was that it doesn't require changes to internals, but since discussing that context should also support where the callback will be executed it, same changes has to be made for 3.1 and 5 if I am not mistaken. Based on this .then(context, callback) overload to existing .then(callback) since like less verbose to write/read option compared to QtPromise::bind()

[dpurgin]

Hi Simon,
Hi Yura,

this is an interesting problem indeed, I had stumbled upon it once but solved in a way that ensured that my QObjects outlive the promises.

Ad "if"-Issue: I like the idea of the automatic fail if the "context" object is destroyed. It would be consistent with the Qt framework behavior. We could probably add one or more "watched" QObjects at the beginning of the promise chain, so that there is no need in either "bind" or "via" calls or extra parameters. Something like:

class Foo : public QObject
{
    QPromise<int> bar() 
    {
        return QPromise<int>{this, 
            [](auto resolve, auto reject) {
                 // ...
            }
        };
    }
};

Foo foo;
foo.bar()
    .then([](int r) {
         return QString::number(r);
    })
    .then([](QString s) {
        // ...
    })
    .fail([](const QPromiseContextException&) {
    })
    .fail([]() {
    });

I like that the handling of the QPromiseContextException is optional and could be simply ignored.

Ad "where"-Issue: I think that the then(context, [context]() { }) is better than the then(QtPromise::bind()) for three reasons:

1. It is more readable.
2. I think that bind is the right name here but I am not sure whether it would not somehow interfere with the knowledge of what std::bind does.
3. I've seen a lot of .cpp-files with using namespace std which I don't do, but I tend to do using namespace QtPromise myself. So I wonder what happens if there is a person who uses both and there is a name clash since QtPromise::bind() would a free function.

I think the 1. is the most important even if it leads to the duplication of the chain functions. At the end of the day, the library should be easy to use rather than easy to develop.

As for the general idea of "where" and multithreading, it seems to me that the computation is being transferred into a chained promise handler using the thread affinity of the context object instead of being encapsulated in the object itself. I would probably refactor the application in a way that the computation is performed in some closed algorithms using the QtConcurrent in the promise chain. Or, if these are some long-living objects, this could be a case of the agent model of computation, so one should remain at message passing (signals/slots). In this case, we should either do the QMetaObject::invokeMethod() in a lambda, as Yuri does, or have an option for .then() to bind member functions of an object, kind of:

class Foo : public QObject {};
QtPromise::resolve(42).then(QtPromise::bind(&foo, &Foo::method1, _1));

In this case, the QtPromise::bind does actually resemble the std::bind with the difference that it should detect thread affinity and call the QMetaObject::invokeMethod() when needed.

Cheers
Dmitriy

[durkmurder]

Hi Dmitriy,

Thanks for your feedback. I believe that context has to handle both if and where if used as .then(context, callback). The big issue with using it in constructor of promise is when promise is exposed by some class that you don't want/can't to modify. If that's the case you will need to wrap up every promise call into new promise, which is way too much.
About multithreading, I kinda use promises in my apps a lot and have developed next pattern:

Promise<void> ChainManager::loadChains(std::vector<AssetID> chains)
{
    return Promise<void>([=](const auto &resolver, const auto &reject) {
        QMetaObject::invokeMethod(_executionContext, [=] {
            try
            {
                // do something
                resolver();
            }
            catch(std::exception &ex)
            {
                reject(ex);
            }
        });
    });
}

Note: _executionContext is just QObject that is used to identify same thread as this. This is done to support const methods(QMetaObject::invokeMethod accepts as first param QObject* but not const QObject*)
I have a one or more long running worker threads where heavy logic happens through lifetime of application. UI thread uses promises and event loop to communicate with worker thread.

Somewhere from UI thread

{
      chainManager.loadChains({}).then([this] {
             // do something on main thread.
      }) ;
}

Generally it works ok since I make sure that context is valid or add some extra checks.
Adding .then(context, callback) will solve all of my issues and even simplify writing of multi-thread code without using new QPromise + QMetaObject::invokeMethod. That pattern will be changed to something like this:

QtPromise::resolve(this).then(_executionContext, [] {
    // All logic here 
    // Without need of new promise and explicit exception handling
}

Hopefully it makes sense.

[dpurgin]

Hey Yura,

that's an interesting use case indeed. So if I understand correctly, you queue work units into an existing thread worker and perform postprocessing after a particular work unit is ready, thus organizing a pipeline. That's quite an ingenious solution with QMetaObject::invokeMethod!

For what it's worth if you don't need to share the worker's state between the work units, you could remove the thread awareness of the worker and instantiate it in a QtConcurrent call. I solved a file copying problem with a progress report in this manner:

// a pseudocode snippet, by a happy coincidence looking like C++/QtPromise
QPromise<void> Foo::copyFiles(QVector<QString> fileList, QString destination)
{
    return QtPromise::resolve(QtConcurrent::run([this, fileList, destination]() {
        // now in a thread from the global thread pool
        // FileWorker is unaware of threads, it just copies files sequentially and emits signals
        FileWorker worker;
        connect(&worker, &FileWorker::progressChanged, this, &Foo::copyProgressChanged);
        worker.copy(fileList, destination); // may throw, or return a bool and throw here
  });
}

Transferring it to your example, the processing logic involved in the _executionContext could be implemented in a separate thread-unaware processor class ChainLoader. So the ChainManager runs in the main thread and starts an instance of the ChainLoader in a QtConcurrent::run() call.

But, again, if you have to share the state of the ChainLoader between the calls, it won't work. Of course, if ChainManager is destroyed, you run again into the if-problem but from what I can comprehend from the name of the class, the ChainManager is a singleton and exists while the application runs. Again, there are things to consider when the application ends while the promise is still running, but let's keep it simple to illustrate the idea.

Cheers
Dmitriy

[durkmurder]

Hi Dmitriy
I understand what are you trying to say. Strictly speaking I kinda support the same behavior as using QtConcurrent::run() but my classes do just a bit more in terms of sending task to event loop using invokeMethod. It's kinda the same flow as using QtConcurrent::run(). The idea is that there is interface exposed to user where he can use promises to get data in async fashion. How the data is retrieved it's kinda implementation dependent.
Supporting .then(context, callback) will solve two problems. if the callback should be executed at all and where it has to be executed in terms of thread affinity. Which will be very powerful for concurrent Qt applications.

[durkmurder]

Hey @simonbrunel, any input on last comments?

[simonbrunel]

Hi guys! Thank you for your feedback, it's really interesting to learn about how you use QtPromise. And sorry for the late reply, I'm currently focused on something else far from this lib. Note that I'm on QtMob slack if you want a more interactive exchange :)

@durkmurder ...same changes has to be made for 3.1 and 5 if I am not mistaken.

QtPromise::bind() doesn't require internal changes to support if and where: it can call the passed callback using QMetaObject::invokeMethod or throw if the context captured with a QPointer is null. I did some experiments implementing QtPromise::bind() but I'm facing some issues deducing arguments of the returned functor.

@dpurgin At the end of the day, the library should be easy to use rather than easy to develop.

That's a really good point which makes me reconsider option 3.1. Though, I'm still concern about maintainability if this new pattern .{chain_method}(context, callback) doesn't scale (e.g. needs more options). For example QFuture v6 already has 3 .then() variants but that's the only continuation method they have. And I was also thinking to support shared/weak pointers as context.

@dpurgin I've seen a lot of .cpp-files with using namespace std which I don't do...

That's personal but I would consider using namespace a bad practice and always recommend to explicitly use the QtPromise:: prefix (I recently changed the docs). QtPromise already contains methods that can conflict: map, each, all, ... so I'm not too worry to introduce another one. And if we need a better name, I'm not that attached to bind.

@dpurgin

About having context part of the promise constructor: to me, dealing with a context is because we need to access it from a continuation (async) callback. The constructor callback is executed synchronously and I don't see any reason to invalidate the entire promise chain later if the context isn't needed by the rest of the chain. Though, I'm open for discussion if you think there is legit use cases for it but maybe on Slack or another GH ticket.

@durkmurder @dpurgin

It looks like option 3.1 is the favorite and would solve most use cases. It's similar to the ::bind() proposal in term of behavior and usage so I think I would be happy with it. Though, I would like first to evaluate the amount of changes it requires and impacts on the public API.