A slot called by qt, which is a coroutine: handling exceptions

[syyyr]

Hi, I have this kind of code:

#include <iostream>
#include <QTimer>
#include <QCoreApplication>
#include <QCoroSignal>

struct Response {

};

class ServerConnection : public QObject {
    Q_OBJECT
public:
    ServerConnection()
    {
        // connect to the server...
        emit serverConnected();
    }
    Q_SIGNAL void serverConnected();

};

QCoro::Task<Response> sendMessage();

QCoro::Task<> onServerConnected()
{
    std::cerr << "doStuff()\n";
    // ... do stuff, maybe use sendMessage with co_await
    //
    // ...
    //
    throw std::runtime_error("Can't catch this");
}

int main(int argc, char* argv[])
{
    QCoreApplication app(argc, argv);
    auto srv_conn = new ServerConnection();
    QObject::connect(srv_conn, &ServerConnection::serverConnected, onServerConnected);
    app.exec();
}

I want to rework my current app to use qcoro. However, I don't want to rewrite all of the stuff at once. In the above code, I'd like to use coroutines inside the doStuff function. This means that doStuff itself must be a couroutine. My issue is that if doStuff throws, I have no way to handle that exception, because Qt doesn't call it with co_await (I'm not even sure how that would work!).

What do you think is the correct approach? Should I just wrap my "top-level" couroutine with a try-catch block and handle the exceptions there?

[danvratil]

Hi,

generally, exceptions are pain in Qt, exactly for the reason you describe - how to handle an exception thrown from a slot invoked from an event loop? There's no (easy) way, really. Which is why exceptions in Qt are discouraged.

With coroutines a new pattern emerges: error from asynchronous operations can propagate to caller the same way as result. My favorite pattern is using the proposed std::expected (e.g. tl::expected) to propagate result/error from the coroutine to the awaiter. There are two main advantages (IMO) to this: the API clearly indicates the possible error condition, and you are forced to handle the error (which suits lazy people like me :D)

QCoro::Task<tl::expected<QByteArray, Error>> sendMessage(const QString &msg) {
    if (!connected) {
        co_return tl::make_unexpected(Error::NetworkError);
    }

    const QByteArray response = ....
    co_return response;
}


// Depending on whether `onServerConnected()` would be a 'top-level" coroutine, you may or may not
// want to propagate an error further up the coroutine chain
QCoro::Task<> onServerConnected() {
    const auto result = co_await sendMessage(...);
    if (!result.has_value()) {
        qWarning() << "Failed to send message, reason: " << result.error();
        co_return;
    }

    const auto parsed_response = parseResponse(*result);
    if (!parsed_response.has_value()) {
        qWarnnig() << "Failed to parse message, reason: " << parsed_response.error();
        co_return;
    }

    // do something with parsed_response
}

[syyyr]

I see. Well, it won't be possible for me to rework everything to std::expected (although I like the approach). I was wondering: would it be possible to assign a custom handler for unhandled exceptions? Or perhaps add an option to change the default behavior of silently ignoring the exceptions to "terminate the program"? That would help with debugging (I could log the exception) and possibly some exceptions coming out of logic errors (a std::logic_error).

[danvratil]

I see two options here, if you want to stick to exceptions:

1. In every top-level coroutine, handle all exceptions globally:

QCoro::Task<> onServerConnected() try {
    ...
    ...
} catch (const std::exception &e) {
    qCritical() << "Unhandled exception in foobar:" << e.what();
    std::terminate();
}

2. Change QCoro to abort on unhandled exception

The idea is that if a coroutine which is invoked from an event loop throws an exception, QCoro will rethrow the exception immediately (which will propagate to Qt event loop and cause an abort), instead of storing it.

On the other hand this would be somewhat violating the coroutine contract, which says that exception is rethrown inside the awaiter when the awaited coroutine throws, which implies that if there's no awaiter, the exception (as well as the coroutine result) are silently discarded. So I'm somewhat hesitant to do this, or at least it would have to be hidden behind some QCoro-wide global option you would call in main(), e.g. QCoro::rethrowExceptionWhenCoroutineIsInvokedFromEventLoop() (yeah, naming is hard 😆 )

[syyyr]

Ideally, I would still like to preserve throwing from co_await coro;, and only abort, when the coroutine is NOT awaited. I'm not sure if that's even possible.

So I'm somewhat hesitant to do this, or at least it would have to be hidden behind some QCoro-wide global option you would call in main(), e.g. QCoro::rethrowExceptionWhenCoroutineIsInvokedFromEventLoop() (yeah, naming is hard laughing )

Yes, I agree, it would have to be an option. I understand that it would imply some global state which is suboptimal. But it's something that's probably set only once in a program's lifetime, so it could be an environmental variable or maybe a compile-time option.

Also, another possible solution: Task could have an optional template parameter, that would specify exception handling. That way, I could mark my "top-level" coroutines with "abort on exceptions" and other ones with "discard non-awaited exceptions". This won't have any runtime state. Also possibly no runtime overhead, because the option would be known at compile time. And I wouldn't have to wrap my functions in a try-catch block. 😁 What do you think?

One more thought: in JS, if you throw from a Promise (which can be awaited, similarly to a C++ coroutine), the program aborts. However, a C++ exception thrown in a coroutine is more like a rejection of a Promise in JS: if you await a promise in JS and it rejects, the await throws. If you do NOT await the Promise and it rejects, the program throws an exception that can't be handled. Interestingly, not too long ago, JS worked similarly to QCoro: unhandled rejections were (caught) and discarded. It might be beneficial to know why exactly they made this change. I'll look into it.

[syyyr]

This is the discussion thread for JS: nodejs/node#20392 It seems that it's a matter of opinion rather than some kind of a technical issue. It seems that the creators of Node.js eventually agreed that unhandled rejections should just terminate the program.

If similar behavior were to be implemented in QCoro, the default would be to terminate on unhandled exceptions. co_await can still rethrow them as normal.

IMHO, non-awaited exceptions should just crash the program. Ignoring them could lead to undefined behavior and/or memory leaks.

[danvratil]

Ideally, I would still like to preserve throwing from co_await coro;, and only abort, when the coroutine is NOT awaited. I'm not sure if that's even possible.

Coincidentally, this is possible with QCoro. We have to have a special trick where the coroutine is aware of the Task's lifetime. Normally the lifetime of a coroutine is bound to the lifetime of the Task, so when Task is destroyed, the coroutine is destroyed. But this only works when the coroutine is always co_awaited, which is not the case with a coroutine being called from an event loop - the event loop will discard the Task returned from the coroutine, which would normally destroy the coroutine before it would start. So what we do is that if Task is destroyed while the coroutine is still running, it will not destroy the coroutine, but instead the coroutine will self-destruct when it reaches the final suspend point.

To put it shortly, yes, we are able to detect when coroutine has finished without being co_awaited, so we could rethrow the exception at that point, if it holds any.

I like your idea of an additional template parameter to Task to enable this behavior. I'll try to put something together and send you a link to PR so you can try it out.

[danvratil]

Some observation from my current implementation:

1. Adding a template argument to the Task template class effectively affects its API by breaking any forward-declarations in user code.

I might work around this by introducing an UnawaitableTask return type which will implement the special handling for exceptions, rather than overloading the Task template arguments.

2. There's no way to re-throw the unhandled exception from inside QCoro without the C++ runtime causing an abort() due to the exception inadvertently propagating through a noexcept methods

Normally when a Qt slot throws and the exception propagates all the way into Qt's event loop, you only get a warning from Qt about an unhandled exception reaching the event loop, but the exception will propagate further up from the QEventLoop::exec(), so it's possible to write code like this:

int main(int argc, char **argv) {
    QCoreApplication app(argc, argv);

    MyApp myApp;
    try {
        app.exec();
    } catch (const std::exception &e) {
        std::cerr << "The application has thrown an exception, the program will end now" << std::endl;
        myApp.emergencyCleanup();
        return 1;
    }
    return 0;
}

This is not possible when handling the exception inside QCoro's coroutine code, the only way is to print some warning and then call std::terminate() to terminate the program.

[danvratil]

The current PoC code lives in the feature/task-exception-handling branch.

[syyyr]

Thanks for the update. I'm going to try the new feature.

1. Yeah, I'm fine with another class instead of a template argument. However, UnawaitableTask implies that you can't use co_await on that task. Is that intentional? I would expect the name to be something like... TaskWhereUnhandledExceptionsAbort. Or something. Naming really is hard. In my opinion, the aborting on exception should be the default (and only available?) behavior. But maybe there is a usecase where you would want the exceptions to be completely ignored and I just can't see it.

2. That is fine by me. :) (and by Node.js, which I've mentioned before).

Also, I was wondering whether it would be possible to implement the same thing QCoro::Generator. My usecase is this: I'm testing an application that sends and accepts messages. I'm using the generator to control the flow of the test. The generator (the test driver) sends a message to the client, then yields. After the code accepts a response, it gets saved somewhere, and then a generator iterator gets incremented. This advances the coroutine further, and the test continues. I'm using a library for assertions in the coroutine and the library uses exceptions. If they're caught by QCoro, the program won't exit on exception failure.

One more thing: would it be possible to allow QCoro::Generator<void>? In my tests I really only use yield to pause the execution, I don't really to generate anything. However, using void isn't possible because of compile errors. Maybe it's not possible to implement at all? For now, I've just been using Generator<int> and throwing the integer away.

I don't really depend on the Generator stuff, I know that I'm bending it a little and that it's not supposed to be used like that.

[syyyr]

I have tried the new branch, but it seems that I'm doing something wrong. Exceptions are still being caught. Or rather, I would think that this program would crash, but it doesn't:

#include <QCoreApplication>
#include <QTimer>
#include <QCoroTask>

QCoro::Task<void, QCoro::TaskOptions<QCoro::Options::AbortOnException>> my_coro()
{
    throw std::exception();
    co_return;
}

int main(int argc, char* argv[])
{
    QCoreApplication app(argc, argv);

    auto tmr = new QTimer();
    QObject::connect(tmr, &QTimer::timeout, my_coro);
    tmr->start(1000);
    app.exec();
}

[danvratil]

Regarding the Generator & exceptions - the exception from the generator should be re-thrown when you dereference the iterator, which, combined with the TaskWhereUnhandledExceptionsAbort should cause the code to terminate. Or does it not? There might be a bug in the generator code for sure, I think I need to actually write tests for exception support in generators.

Regarding Generator<void> - I don't know. It adds a bit of complexity and I don't see much value in a generator that generates nothing :-) In any case, would you mind creating a new issue for it?

Finally, regarding your test code: I pushed a fix to the branch, please retry :)

[syyyr]

Regarding the Generator & exceptions - the exception from the generator should be re-thrown when you dereference the iterator, which, combined with the TaskWhereUnhandledExceptionsAbort should cause the code to terminate. Or does it not? There might be a bug in the generator code for sure, I think I need to actually write tests for exception support in generators.

I got this sample code:

#include <iostream>
#include <QCoreApplication>
#include <QCoroGenerator>

QCoro::Generator<int> my_generator()
{
    co_yield 123;
    throw std::exception();
}

int main(int argc, char* argv[])
{
    auto gen = my_generator();
    auto it = gen.begin();
    std::cerr << "*it = " << *it << "\n";
    ++it;
    std::cerr << "*it = " << *it << "\n";

}

It crashes with a segfault:

$ ./a.out 
*it = 123
*it = AddressSanitizer:DEADLYSIGNAL
=================================================================
==11600==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000018 (pc 0x556423246580 bp 0x7ffdbb2867c0 sp 0x7ffdbb2867a0 T0)
==11600==The signal is caused by a READ memory access.
==11600==Hint: address points to the zero page.
    #0 0x556423246580 in std::__exception_ptr::exception_ptr::exception_ptr(std::__exception_ptr::exception_ptr const&) /usr/include/c++/12.1.0/bits/exception_ptr.h:190
    #1 0x556423248217 in QCoro::detail::GeneratorPromise<int>::exception() const /usr/include/qcoro5/qcoro/qcorogenerator.h:93
    #2 0x55642324744e in QCoro::GeneratorIterator<int>::operator*() const /usr/include/qcoro5/qcoro/qcorogenerator.h:177
    #3 0x556423245ef5 in main /home/vk/main.cpp:18
    #4 0x7fe1ffe2928f  (/usr/lib/libc.so.6+0x2928f)
    #5 0x7fe1ffe29349 in __libc_start_main (/usr/lib/libc.so.6+0x29349)
    #6 0x5564232442b4 in _start (/home/vk/a.out+0x82b4)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /usr/include/c++/12.1.0/bits/exception_ptr.h:190 in std::__exception_ptr::exception_ptr::exception_ptr(std::__exception_ptr::exception_ptr const&)
==11600==ABORTING

Also, I didn't realize I would have to derefence to iterator to rethrow the exception. I suppose it makes sense. I did think that the operator++ would throw it though.

Regarding Generator - I don't know. It adds a bit of complexity and I don't see much value in a generator that generates nothing :-)

My usecase was to just have a simple (synchronous) coroutine, that I could run and pause (yield). Without the coroutine I would need to have a function that would work kind of like a state machine, which is undesirable for me. But Generator<void> is not easily replaceable by Generator<int> with (I hope) no real overhead.

In any case, would you mind creating a new issue for it?

Done: #100

Finally, regarding your test code: I pushed a fix to the branch, please retry :)

Yep, now it seems to work.

[syyyr]

Also, it seems that it does not exactly where the iterator is, if one exception was thrown, you can advance the iterator as much as you want and the next dereference will throw the exception (it crashes with segfault right now, but that's a detail). This behavior kind of makes me think, that the operator++ should be the function that rethrows. I'm not sure though. I don't have a strong opinion on this.

#include <iostream>
#include <QCoreApplication>
#include <QCoroGenerator>

QCoro::Generator<int> my_generator()
{
    co_yield 123;
    throw std::exception();
    co_yield 321;
}

int main(int argc, char* argv[])
{
    auto gen = my_generator();
    auto it = gen.begin();
    ++it;
    ++it;
    ++it;
    ++it;
    ++it;
    *it;
}

I should probably open another issue for this.

[danvratil]

I think I need to formalize the behavior of the iterator a bit more in the documentation (and check the code if it matches the expections describe below):

When an exception is thrown from the generator, it terminates the generator and thus the next iterator becomes invalid. What you do in your example is undefined behavior:

int main(int argc, char* argv[])
{
    auto gen = my_generator();
    auto it = gen.begin(); // *it == 123
    ++it;                            // *it == std::exception
    ++it;                            // it == gen.end()
    ++it;                            // undefined behavior: incrementing a past-the-end iterator
    ++it;                            // ditto
    ++it;                            // ditto
    *it;                               // undefined behavior: dereferencing a past-the-end iterator
}

I think exception handling in generators needs better documentation (and tests) :-) I'll try to look into it.

Sorry for the delay in the terminating-generator, I wanted to complete another WIP change. I'll get back to the generators this week.

[syyyr]

I didn't realize, that throwing an exception would make the next iterator the .end iterator. I think that makes sense: if an error occured like that, you shouldn't be able to generate more stuff. Thanks for looking into this. I think this can be solved in the documentation (as you suggest).

[danvratil]

Hi,

regarding the exceptions in generators:

1. there was a bug in the sync generator that would cause a crash when exception was thrown inside the generator
2. after some considerations I realized that your initial expectation that the exception should be thrown from the operator++() made sense, so I adjusted the behavior for both sync and async generators
3. I added an explicit Exceptions section to the documentation for both sync and async generators

Everything is done in #105.

[syyyr]

Hi, I was wondering: what do you think about merging feature/task-exception-handling into main?

[danvratil]

Hi, after more thought, I'm not really comfortable with the additional template argument, so I'm not going to merge the branch in the current state.

I'm trying to think of some better way to implement thids, possibly being able to auto-detect whether we are being called from inside the event loop.

[syyyr]

Hi, is there any progress on this? I can see that the development is QCoro is continuing, so maybe there's been a patch that addresses this? For now, I've been using my own branch here, but I'm not able to rebase it anymore, because QCoro changed too much for me to easily do that.

Do you think there's a chance for QCoro to rethrow unawaited (and therefore uncatchable) exceptions instead of silently throwing them away? I can't really think of a usecase, where I would be happy about just ignoring all exceptions (other than wrapping my whole program in a try-catch block). I suppose it's really a matter of opinion.

However, I'm not really happy about maintaining my own branch. I understand, that this issue is quite difficult to solve and it might take a while. If that's the case, maybe I'll just revert back to catching all exceptions with try-catch in the "top-level" coroutine and calling std::terminate.

[danvratil]

Hi, sorry there's been no progress on this front. I couldn't (yet) find a good way how to rethrow the unhandled exception from a non-coawaited coroutine.

I understand it would be consistent with regular functions if an uncaught exception would have been logged inside QCoreApplication and propagated to main(), I just can't see a way to achieve this at this moment, sorry :(