8000 Early co_returns do not resume the caller · Issue #24 · qcoro/qcoro · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Early co_returns do not resume the caller #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rgriebl opened this issue Nov 30, 2021 · 5 comments
8000
Closed

Early co_returns do not resume the caller #24

rgriebl opened this issue Nov 30, 2021 · 5 comments
Assignees
@danvratil
Labels
bug Something isn't working

Comments

@rgriebl
Copy link
rgriebl commented Nov 30, 2021

Maybe I'm not understanding correctly how coroutines work, but I think the following example should work for both cases: returning early and awaited from testReturn.
What I'm seeing (gcc 11 and MSVC 2019) is that testImmediate just hangs and never returns from its co_await

static QCoro::Task<bool> testReturn(bool immediate)
{
    qWarning() << "Running testReturn(immediate:" << immediate << ")";
    if (immediate) {
        co_return true;
    } else {
        QTimer t;
        t.start(100);
        co_await(t);
        co_return true;
    }
}

static QCoro::Task<> testImmediate()
{
    qWarning() << "Starting Immediate...";
    bool b = co_await testReturn(true);
    qWarning() << "Immediate result:" << b;
}

static QCoro::Task<> testDelayed()
{
    qWarning() << "Starting Delayed...";
    bool b = co_await testReturn(false);
    qWarning() << "Delayed result:" << b;
}

int main(int argc, char **argv)
{
    QCoreApplication a(argc, argv);
    QMetaObject::invokeMethod(qApp, []{ testImmediate(); }, Qt::QueuedConnection);
    QMetaObject::invokeMethod(qApp, []{ testDelayed(); }, Qt::QueuedConnection);
    return a.exec();
}

Output:

Starting Immediate...
Running testReturn(immediate: true )
Starting Delayed...
Running testReturn(immediate: false )
Delayed result: true
@danvratil
Copy link
Collaborator

Indeed, this should work as you described. Thanks for the nice reproducer.

@danvratil danvratil added the bug Something isn't working label Nov 30, 2021
@danvratil danvratil self-assigned this Nov 30, 2021
@rgriebl rgriebl mentioned this issue Dec 2, 2021
Closed
@rgriebl
Copy link
Author
rgriebl commented Dec 3, 2021

Is there anything I could help with debugging? I'm just starting out with co-routines, so I'm still a bit lost in all the template magic, but maybe you have an idea/hint what may go wrong here?

@danvratil
Copy link
Collaborator

Hi, sorry for not fixing this yet, I'm a bit busy with work :( The problem is that the fact that the coroutine has finished synchronously doesn't propagate to the Task<> that is returned to the caller, so when you co_await that Task, it goes to a regular suspend, expecting that it will be awoken eventually when the coroutine finishes - which is where things break, because the coroutine has already finished.

The fix here is to check for the state of the Task before suspending it...

@rgriebl
Copy link
Author
rgriebl commented Dec 5, 2021

Hi, no big deal. I now tried to fix this myself, but I can't get it to work 100%:

  • Thanks to the logic around the mResumeAwaiter atomic, it should work already fine in theory.
  • The problem is the finishedCoroutine.destroy() call in TaskFinalSuspend, because this destroys the result value too early in the immediate co_return case.
  • If I comment out that destroy() call then the program works as expected, but this creates a memory leak (at least I think it does ;) )
  • Looking at cppcoro, I see that they are destroying the co-routine in ~Task, but if I do the same, then the second case (the delayed return) hangs forever.

Any ideas?

@danvratil
Copy link
Collaborator

Your investigation is correct and you are heading in the right direction.There are two cases that main need to be handled:

  1. When the Task<> returned from coroutine is ignored by the caller (this happens when your coroutine is invoked from the Qt event loop)
  2. When the Task<> returned from coroutine is not ignored by the caller (i.e. it's co_awaited y the caller of the coroutine)

I think the right approach that I'm pursuing now, is to indeed destroy the coroutine from ~Task, but only if the coroutine is finished when the Task is destroyed. If the coroutine is only suspended when the Task is destroyed (which happens in case 1.), then the destruction of the coroutine must be left to the TaskFinalSuspend (which is basically a kind of coroutine "self-destruct".

rgriebl added a commit to rgriebl/qcoro that referenced this issue Dec 6, 2021
@rgriebl
Do not destroy co-routines too early.
b920f2e 
Co-routines returning immediately with co_return would have an invalid
result, because the co-routine handle was destroyed before the caller
had a chance to get the result.

We do have to make sure to still destroy co-routines in final-suspend,
if nobody is awaiting the result (e.g. when called from Qt's event
loop).

Fixes: qcoro#24
@rgriebl rgriebl mentioned this issue Dec 6, 2021
Closed
rgriebl added a commit to rgriebl/qcoro that referenced this issue Dec 8, 2021
@rgriebl
Do not destroy co-routines too early.
8b7b312 
Co-routines returning immediately with co_return would have an invalid
result, because the co-routine handle was destroyed before the caller
had a chance to get the result.

We do have to make sure to still destroy co-routines in final-suspend,
if nobody is awaiting the result (e.g. when called from Qt's event
loop).

Fixes: qcoro#24
ivknv added a commit to ivknv/qcoro that referenced this issue Jan 4, 2022
@ivknv
Fix issue qcoro#24 (Early co_return does not resume the caller)
8f1a64c
ivknv added a commit to ivknv/qcoro that referenced this issue Jan 4, 2022
@ivknv
Add a test for issue qcoro#24
99c98b2
ivknv added a commit to ivknv/qcoro that referenced this issue Jan 4, 2022
@ivknv
Add a test for issue qcoro#24
f5c5ba2
ivknv added a commit to ivknv/qcoro that referenced this issue Jan 4, 2022
@ivknv
Fix issue qcoro#24 (Early co_return does not resume the caller)
026f3c8
ivknv added a commit to ivknv/qcoro that referenced this issue Jan 4, 2022
@ivknv
Add a test for issue qcoro#24
0055c07
ivknv added a commit to ivknv/qcoro that referenced this issue Jan 4, 2022
@ivknv
Fix issue qcoro#24 (Early co_return does not resume the caller)
5aa1e17
@ivknv ivknv mentioned this issue Jan 4, 2022
Merged
danvratil added a commit that referenced this issue Jan 5, 2022
@danvratil
Merge pull request #35 from ivknv/fix-issue-24
d599729 
Fix issue #24: Early co_returns do not resume the caller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@danvratil danvratil

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not destroy co-routines too early. rgriebl/qcoro
2 participants
@danvratil @rgriebl
0