danvratil / qcoro

C++ Coroutines for Qt
https://qcoro.dvratil.cz
MIT License
326 stars 53 forks source link

Prevent coroutine from being stuck if sender is deleted #221

Open shaan7 opened 1 month ago

shaan7 commented 1 month ago

DRAFT: I'm only starting to use coroutines in a real project, so treat this as a PoC. I'd first like to know if this approach is acceptable. I also saw the guardThis implementation (#191) but that seems to solving a different problem. If yes, I'll clean this up so to share some of the QCoroSignal code.

It is possible for a sender QObject to be deleted before it gets a chance to emit a signal that we are co_await'ing on. In such a case, the coroutine will never be resumed. A side-effect of this is that all variables that are local to that coroutine will never get deallocated and will stay in limbo forever.

For example, the following snippet:

    SomeObject obj;

    m_msgBox = new QMessageBox(this);
    m_msgBox->setStandardButtons(QMessageBox::Yes | QMessageBox::No | QMessageBox::Cancel);
    m_msgBox->setModal(false);
    m_msgBox->show();
    auto res = co_await qCoro(ad, &AnotherDialog::finished);

    // Do something with obj

The object obj will never get freed if this is deleted by something else (in a real life case, this can happen due to some conditions which require us to reload the current "project" in our app).

Using the new function qCoroWatch instead of qCoro let you do this:

    SomeObject obj;

    m_msgBox = new QMessageBox(this);
    m_msgBox->setStandardButtons(QMessageBox::Yes | QMessageBox::No | QMessageBox::Cancel);
    m_msgBox->setModal(false);
    m_msgBox->show();
    auto res = co_await qCoroWatch(ad, &AnotherDialog::finished);
    if (!res.has_value())    // Sender deleted
        co_return;

    // Do something with obj

As I said above, I'm only starting to use coroutines so it is possible that this has some limitations that I'm unaware of. I'm open to trying other approaches to solve the problem.

danvratil commented 1 month ago

Hi Shantanu, thanks for looking at this! I like the idea to resume the coroutine instead of basically leaking it if the sender object is destroyed. I think that we as poor design decision on my side when I initially implemented that into the qCoro() wrapper. Unfortunately I don't think we can easily retro-fit that into qCoro(), so I'm all for your solution with qCoroWatch().

shaan7 commented 1 month ago

@danvratil I've cleaned up the implementation and also updated the documentation. Just to make sure the API and behavior isn't awkward, I'll use it for a few days in a real project. If everything is good, I'll mark this PR ready for review.

shaan7 commented 1 month ago

Thanks for taking another look @danvratil, I still have to finish the implementation in the project I used it in. Once it has gone through some testing and I'm more confident of this, I'll open the PR for review.

I think I also need to check at the CI jobs that are failing on this PR, lets see when I get to it.