Memory Leak #255

dantti · 2024-10-27T18:19:40Z

Hi,
I've sent you a long email about some questions that I have and one of them I was able to reproduce and also fix it, but before pushing a patch I'd like to see if yo agree with my change:

#include "qcorotask.h"
#include "qcorotimer.h"

#include <QCoreApplication>
#include <QDateTime>
#include <QTimer>

#include <chrono>
#include <iostream>

using namespace std::chrono_literals;

class Bar
{
public:
    Bar() {
        std::cout << "Bar" << std::endl;
    }

    ~Bar() {
        std::cout << "~Bar" << std::endl;
    }
};

class Dialog
{
public:
    Dialog() {
        std::cout << "Dialog" << std::endl;
        timer.setInterval(2s);
        timer.start();
        QObject::connect(&timer, &QObject::destroyed, []{
            std::cout << "~timer" << std::endl;
        });
    }

    ~Dialog() {
        std::cout << "~Dialog" << std::endl;
    }
    QCoro::Task<> doSomething() {
        std::cout << "doSomething started" << std::endl;
        auto bar = std::make_unique<Bar>();
        co_await timer;
        std::cout << "doSomething ticked!" << std::endl;
        qApp->quit();
    }

    QTimer timer;
};

QCoro::Task<> runMainTimer3(Dialog *d) {
    std::cout << "runMainTimer3 started" << std::endl;
    co_await d->doSomething();
    std::cout << "runMainTimer3 finished" << std::endl;
}

void runMainTimer() {
    auto d = new Dialog;
    std::cout << "runMainTimer started" << std::endl;
    runMainTimer3(d);

    // This will make the bar leak
    delete d;
    std::cout << "runMainTimer finished" << std::endl;
}

int main(int argc, char **argv) {
    QCoreApplication app{argc, argv};
    QTimer ticker;
    QObject::connect(&ticker, &QTimer::timeout, &app, []() {
        std::cout << QDateTime::currentDateTime().toString(Qt::ISODateWithMs).toStdString()
                  << " Secondary timer tick!" << std::endl;
    });
    ticker.start(200ms);

    QTimer::singleShot(0, runMainTimer);
    return app.exec();
}

You can test this and this code will make the bar variable leak, and this is a valid Qt code, say you click save() button, and then close the Window.

I was able to solve this at qcorotimer.cpp::await_suspend() adding:

        QObject::connect(mTimer, &QObject::destroyed, [awaitingCoroutine]{
            awaitingCoroutine.destroy();
        });

Since QCoroTimer is not a subclass this means all specialization of Qt classes should have this, not a big deal, just a bit of code duplication. Should I go and add these?

QCoroSignal which IMHO could be the base of these classes also lack the hability to destroy the handler.

The text was updated successfully, but these errors were encountered:

danvratil · 2024-10-27T20:53:04Z

Hi Daniel, I'm working on the reply to your email, but I figure I might explain some stuff here first, since I can then refer to them from the email :-)

Regarding your patch - it would work in this very particular case, but in general case, what it really does is that it just pushes the memory leak one level up in the callstack. Consider the following modification of your example:

#include "qcorotask.h"
#include "qcorotimer.h"

#include <QCoreApplication>
#include <QDateTime>
#include <QTimer>

#include <chrono>
#include <iostream>

using namespace std::chrono_literals;

class Bar
{
public:
    Bar() {
        std::cout << "Bar" << std::endl;
    }

    ~Bar() {
        std::cout << "~Bar" << std::endl;
    }
};

class Baz
{
public:
    Baz() { std::cout << "Baz" << std::endl; }
    ~Baz() {  std::cout << "~Baz" << std::endl; }
};

class Dialog
{
public:
    Dialog() {
        std::cout << "Dialog" << std::endl;
        timer.setInterval(2s);
        timer.start();
        QObject::connect(&timer, &QObject::destroyed, []{
            std::cout << "~timer" << std::endl;
        });
    }

    ~Dialog() {
        std::cout << "~Dialog" << std::endl;
    }
    QCoro::Task<> doSomething() {
        std::cout << "doSomething started" << std::endl;
        auto bar = std::make_unique<Bar>();
        co_await timer;
        std::cout << "doSomething ticked!" << std::endl;
        qApp->quit();
    }

    QCoro::Task<> doSomethingOutter() {
        std::cout << "doSomethingOutter started" << std::endl;
        auto baz = std::make_unique<Baz>();
        co_await doSomething();
        std::cout << "doSomethingOutter finished" << std::endl;
    }

    QTimer timer;
};

QCoro::Task<> runMainTimer3(Dialog *d) {
    std::cout << "runMainTimer3 started" << std::endl;
    co_await d->doSomethingOutter();
    std::cout << "runMainTimer3 finished" << std::endl;
}

void runMainTimer() {
    auto d = new Dialog;
    std::cout << "runMainTimer started" << std::endl;
    runMainTimer3(d);

    // This will make the bar leak
    delete d;
    std::cout << "runMainTimer finished" << std::endl;
}

int main(int argc, char **argv) {
    QCoreApplication app{argc, argv};
    QTimer ticker;
    QObject::connect(&ticker, &QTimer::timeout, &app, []() {
        std::cout << QDateTime::currentDateTime().toString(Qt::ISODateWithMs).toStdString()
                  << " Secondary timer tick!" << std::endl;
    });
    ticker.start(200ms);

    QTimer::singleShot(0, runMainTimer);
    return app.exec();
}

With your patch, when the QTimer is destroyed, you also destroy the doSomething() coroutine that is co_awaiting on the timer, ensuring that the Bar object allocated there is free'd. However, you have now leaked Baz and the entire doSomethingOutter() coroutine, because it's suspended waiting to be resumed by doSomething().

You would need to chain up the .destroy() calls through the entire chain of coroutines. Or you would need to make sure to wake up the coroutine that is awaiting the "failed" coroutine - but here you run into a problem with the coroutine result. It's easy in case of the QTimer, since the result type is void. However in case of other coroutines (i.e. QCoroIODevice::readAll()) where you need to return some value to the awaiter, there's a problem: you don't have any value to return to them at this point. And even the chaining-up is not without trouble - if you chain up all the way to the outer-most coroutine, if that coroutine has result type that is not void, what are you going to return to the caller?

The missing handling for cancelation/internal failures in QCoro is a repeating problem that is blocking many useful features. I neglected this case in the early stages of QCoro development, since I was building it "on the go" as I was learning coroutines.

There's now a bunch of tickets about QCoro2 API, where I want to take all the lessons learned so far and requirements that I've collected over the time into account and do actual proper design. The major change compared to QCoro is the idea to have a QCoro2::Task<T> that would yield std::expected<T, QCoro2::Error> (or some equivalent), making it possible to indicate cancelation/failure, and thus resume the awaiter even if the coroutine was unable to produce a value of T. QCoro2 API is still a long way away. I might create QCoro::CancellableTask<T> first, which would have some of the intended features of QCoro2::Task<T>, but I can't just change every single coroutine in the library from Task to CancellableTask, that would be a major API break - so you would be able to use it in your code, but still would make it impossible to fix the example you posted (therefore the QCoro2 API that would allow me to break source compatibility).

PS: sorry for all the edits and typos, I'm on a phone...

dantti · 2024-10-27T22:13:29Z

Is it possible to chain up and destroy all coroutines? Eventually something is not coawaiting, runMainTimer calls runMainTimer3 for example, if it returned a bool it would not depend on runMainTimer3 result.

int main() can't be a coroutine so destroying() till event loop should be fine.

About my patch I think it's better with it than without, and I think you should also put a disclaimer of the limitations and current drawbacks of it.

99% of my code would not chain coroutines, but certainly will destroy the caller like in my example. I can't use QCoro as is right now, but would be fine if I knew this limitation or there was some warning/build error agains it.

danvratil · 2024-10-27T22:35:01Z

As long as the entire stack are QCoro::Tasks, then we are able to traverse that internally (but even with QCoro itself, that might not be the case as we use awaitable wrappers internally in a some places that would need to be made aware of the "chainup"). Alternatively it could be tracked externally....

Eventually something is not coawaiting, runMainTimer calls runMainTimer3 for example, if it returned a bool it would not depend on runMainTimer3 result.

Again, in this specific scenario, it would work. However, consider this scenario, where you are "spawning" a coroutine from a regular function and waiting for its result synchronously (waitForFinsihed runs a nested QEventLoop, so it's not blocking blocking, but just a little blocking ;-))

void myFunc() {
    // ..
    const auto result = QCoro::waitForFinished([]() -> QCoro::Task<std::unique_ptr<Result>> {
        // ...
    });
}

In this case, you always need to provide some result to the caller and, what's worse, the result type is not default-constructible, so you can't even fabricate one. QCoro2 to the rescue! :)

About my patch I think it's better with it than without, and I think you should also put a disclaimer of the limitations and current drawbacks of it.

I agree with the second part (the docs need a big overhaul for a long time), not so much with the first part. While it would solve the problem for your particular use-case, that's not the case for everyone and this would make the behavior quite strange IMO (it's already hard to debug coroutines with the current tooling as it is).

I can't use QCoro as is right now

Hopefully we can fix that with the QCoro2 API - so far from what I've seen here and i the email, it all boils down to handling cancellation properly and being able to express that to the caller. Then we can hook into events like object we are awaiting on being destroyed.

but would be fine if I knew this limitation or there was some warning/build error agains it.

I'd love to be able to detect those issues at compile time, alas documentation is all I can do right now :)

dantti · 2024-10-27T23:09:02Z

that's not the case for everyone and this would make the behavior quite strange IMO (it's already hard to debug coroutines with the current tooling as it is).

I would argue that this wouldn't make the behavior different than what it is today, as the chained awaitable will still leak, and I think the wait for finished will likely be stuck there too and leak as well, so it's leaking less it seems :D

waitForFinished is the root of all issues :P had that for a while in Cutelyst just to regret later, and explicit state on the docs that one should never use that, like image how easy it is to stack overflow if you get 500 network clients all creating QEventLoops...

Is there something I could do to help with QCoro2? couldn't find a branch named like that btw..

danvratil · 2024-10-28T16:21:29Z

QCoro2 is still in what I like to think about as a design phase: https://github.com/qcoro/qcoro/issues?q=is%3Aissue+is%3Aopen+label%3Aqcoro2

Your inputs, especially about underlying QObjects going away, are definitely something I'll be considering when I revisit the documents. Of course feel free to comment under the issues if you feel something is wrong or missing - it's definitely not complete yet and I'll appreciate any early feedback, otherwise it's just me talking to myself in Github issues :D

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory Leak #255

Memory Leak #255

Memory Leak #255

Memory Leak #255

Comments