API v2
Open, NormalPublic

Description

We have discussed some more or less intrusive changes to the API that could resolve some of the awkwardness around lifetimes in our current API.

Likely not source compatible and material for a major version bump.

We should draft the changes we're looking for and then go for it without backwards compatibility IMO.

cmollekopf triaged this task as Normal priority.

My idea is following:

  1. Introduce Promise and Future template classes, modeled after C++'s std::promise and std::future: The Promise is constructed first by the (a)synchronous task. It only has setter for result value (or error) and a getter to obtain a Future, which can the be returned from the task implementation back to KAsync to wait for a result/error. The Future and the Promise have a shared state (basically a shared pointer to a common private class that holds the result/error. It's possible to have multiple Futures created from the same Promise.

    The benefit is that it completely separates the lifetime of the Promise/Future from the Execution and rest of KAsync. This means that we no longer need to pass Future<T> & into every single task that should be asynchronous, cluttering the function's signature. Instead, the function indicates it is an asynchronous task by simply returning Future<T> instead of just T. Another benefit is that this Promise/Future API is more common in modern asynchronous frameworks.
  1. Get rid of all the different types of continuations (API-wise) and handle everything inside of KAsync using TMP magic. All functions will simply accept a Callable and can treat it differently internally, depending on whether it returns a value T, or Future<T>. This approach can be more easily adopted to C++20 concepts, once they are available to us.

    We can switch to C++17 now (I saw that Sink already uses it and I'm planning to go for C++17 in Akonadi sooner rather than later, too) which gained a lot of powers when it comes to template class and function argument deduction and we can implemented our own deduction guides so KAsync user's wont need to specify almost any template parameters manually.
  1. Powerful error handling. I'm still trying to think this one through. I think we should not allow for tasks that look like [](KAsync::Error errorFromPreviousJob, int resultOfPreviousJob), because we never have both the error and the result - it's always only one. At the same time, I'm not sure that introducing something like std::expected (P0323, basically a std::variant<Result, Error>) would make the API nicer... I like the onError() handler that allows user to manually add error handling to a job that it receives from a function, but I do not yet have a clear idea how to handle error propagation and how to allow the execution to continue after an error is handled.

    Considering following task:

    job.then(TASK1).then(TASK2).onError(HANDLE1).then(TASK3).onError(HANDLE2);

    a) Should HANDLE1 be able to handle failure of both TASK1 and TASK2, or just TASK2?

    b) Should TASK2 be skipped, if TASK1 has failed?

    c) Should it be possible for HANDLE1 to tell KAsync that the error was handled successfully and that the execution can continue? I see a lot of problems there, e.g. if TASK3 expects the result of TASK2, where do we take it, since TASK2 was skipped?

    d) Other options...?

Alternatively, should it be possible to pass two lambdas to KAsync functions: first that implements the operation and second that provides error handling when the first one fails? This allows to couple task and it's error handling into a single place, but it makes it harder for users to provide custom error handling for tasks inside a Job returned from a function....

My idea is following:

  1. Introduce Promise and Future template classes, modeled after C++'s std::promise and std::future: The Promise is constructed first by the (a)synchronous task. It only has setter for result value (or error) and a getter to obtain a Future, which can the be returned from the task implementation back to KAsync to wait for a result/error. The Future and the Promise have a shared state (basically a shared pointer to a common private class that holds the result/error. It's possible to have multiple Futures created from the same Promise.

    The benefit is that it completely separates the lifetime of the Promise/Future from the Execution and rest of KAsync. This means that we no longer need to pass Future<T> & into every single task that should be asynchronous, cluttering the function's signature. Instead, the function indicates it is an asynchronous task by simply returning Future<T> instead of just T. Another benefit is that this Promise/Future API is more common in modern asynchronous frameworks.

I think we may even be able to used std::future and std::packaged_task directly, which I would prefer to another custom future/promise.

  1. Get rid of all the different types of continuations (API-wise) and handle everything inside of KAsync using TMP magic. All functions will simply accept a Callable and can treat it differently internally, depending on whether it returns a value T, or Future<T>. This approach can be more easily adopted to C++20 concepts, once they are available to us.

Yeah, what we have is just workarounds for template magic limitations that we hit, so that sounds good to me.

We can switch to C++17 now (I saw that Sink already uses it and I'm planning to go for C++17 in Akonadi sooner rather than later, too) which gained a lot of powers when it comes to template class and function argument deduction and we can implemented our own deduction guides so KAsync user's wont need to specify almost any template parameters manually.

+1

  1. Powerful error handling. I'm still trying to think this one through. I think we should not allow for tasks that look like [](KAsync::Error errorFromPreviousJob, int resultOfPreviousJob), because we never have both the error and the result - it's always only one. At the same time, I'm not sure that introducing something like std::expected (P0323, basically a std::variant<Result, Error>) would make the API nicer... I like the onError() handler that allows user to manually add error handling to a job that it receives from a function, but I do not yet have a clear idea how to handle error propagation and how to allow the execution to continue after an error is handled.

For normal tasks and errors there is no problem. They only get invoked if there is an error or a result.
For the [] (Error, Result) variant, which basically allows to handle both cases in the same continuation a std::expected would be a good option IMO (we can review in the sink codebase how valid/necessary this approach is).

Considering following task:

`job.then(TASK1).then(TASK2).onError(HANDLE1).then(TASK3).onError(HANDLE2);`
 
a) Should //HANDLE1// be able to handle failure of both //TASK1// and //TASK2//, or just //TASK2//?

Both.

b) Should //TASK2// be skipped, if //TASK1// has failed?

Yes.

c) Should it be possible for //HANDLE1// to tell KAsync that the error was handled successfully and that the execution can continue?

Yes

I see a lot of problems there, e.g. if TASK3 expects the result of TASK2, where do we take it, since TASK2 was skipped?

The error handler must forward/create the result matching what TASK3 expects.

d) Other options...?

Alternatively, should it be possible to pass two lambdas to KAsync functions: first that implements the operation and second that provides error handling when the first one fails? This allows to couple task and it's error handling into a single place, but it makes it harder for users to provide custom error handling for tasks inside a Job returned from a function....

I'm not a fan of this option I think.

I think we may even be able to used std::future and std::packaged_task directly, which I would prefer to another custom future/promise.

std::future does not have any state signalling, since it's all, so that might be a bit tricky. We still probably need a Qt-friendly wrapper to implement on top of std::future to provide some signal-slot interface and a FutureWatcher. std::packaged_task is an interesting idea, I will certainly look into it.

The error handler must forward/create the result matching what TASK3 expects.

You cannot create a result out of thin air. If the execution has failed, you simply do not have a result of the previous task. Also what if you have a type that is not default-constructible? This is partially something that we had problems with in the current implementation and it is something that I'd like to have designed more nicely and clearly.

I think we may even be able to used std::future and std::packaged_task directly, which I would prefer to another custom future/promise.

std::future does not have any state signalling, since it's all, so that might be a bit tricky. We still probably need a Qt-friendly wrapper to implement on top of std::future to provide some signal-slot interface and a FutureWatcher. std::packaged_task is an interesting idea, I will certainly look into it.

The error handler must forward/create the result matching what TASK3 expects.

You cannot create a result out of thin air. If the execution has failed, you simply do not have a result of the previous task.

Of course not. If you can recover from an error you *must* be able to produce a result though, otherwise that's pretty much the definition of not being able to recover, and in that case you also cannot execute the follow up tasks.

Also what if you have a type that is not default-constructible? This is partially something that we had problems with in the current implementation and it is something that I'd like to have designed more nicely and clearly.

I don't think I'm suggesting anything that has implications regarding default-constructibility.

Handling an error should allow you to recover and thus continuing execution of all follow up tasks. Recovering always implies delivering a result that matches what the follow up task expects.

A simple example without a value would be:

}).then([=] (const KAsync::Error &error) {
    if (error) {
        SinkWarningCtx(mLogCtx) << "Examine failed: " << error;
        if (error.errorCode == Imap::CommandFailed) {
            //Ignore the error because we don't want to fail the synchronization for all folders
            return KAsync::null();
        }
        return KAsync::error(error);
    }
    return KAsync::null();
});

I haven't found a case where I recover with a value, but it seems simple enough if the value is e.g. an enum or so.

This is a slightly different case where we don't recover, but execute extra cleanup steps in the error handler:

.then([=] (const KAsync::Error &error, const QByteArray &remoteId) {
            if (error) {
                SinkWarning() << "Error during changereplay: " << error.errorMessage;
                return imap->logout()
                    .then(KAsync::error<QByteArray>(getError(error)));
            }
            return imap->logout()
                .then(KAsync::value(remoteId));
        });