jbaldwin / libcoro

C++20 coroutine library
Apache License 2.0
596 stars 61 forks source link

Unexpected Resumption Behavior in nested coroutine call. #268

Closed yuedai closed 4 months ago

yuedai commented 4 months ago

I’ve been experimenting with the libcoro library. I've come across some behavior that I didn't expect and would appreciate your guidance. Here is a minimal reproducible example:

#include <coro/task.hpp>
#include <iostream>

coro::task<void> functionB(int i) {
  for (int offset = 0; offset < 10; ++offset) {
    std::cout << "Inside functionB, offset: " << i + offset << std::endl;
    co_await std::suspend_always{};
    std::cout << "Resuming functionB, offset: " << i + offset << std::endl;
  }
  co_return;
}

coro::task<void> functionA() {
  std::cout << "Inside functionA" << std::endl;
  for (int i = 0; i < 10; ++i) {
    co_await functionB(i);
  }
  std::cout << "Resuming functionA" << std::endl;
  co_return;
}

int main() {
  auto task = functionA();
  while (!task.is_ready()) {
    task.resume(); // Start the coroutine
  }
  return 0;
}

Expected Behavior I expected the code to output something like this:

Inside functionA
Inside functionB, offset: 0
Resuming functionB, offset: 0
Inside functionB, offset: 1
Resuming functionB, offset: 1
Inside functionB, offset: 2
Resuming functionB, offset: 2
...
Inside functionB, offset: 9
Resuming functionB, offset: 9
Resuming functionA

Actual Behavior However, the actual output is:

Inside functionA
Inside functionB, offset: 0
Inside functionB, offset: 1
Inside functionB, offset: 2
Inside functionB, offset: 3
Inside functionB, offset: 4
Inside functionB, offset: 5
Inside functionB, offset: 6
Inside functionB, offset: 7
Inside functionB, offset: 8
Inside functionB, offset: 9
Resuming functionA

It seems that the coroutine does not resume as I expected. I would be grateful if you could point out what I might be doing wrong or if there is an issue with the library.

May I know what's the right way to do this ?

jbaldwin commented 4 months ago

There are two issues with this code that I can see.

First because the root functionA task is being resumed manually, not the child functionB task(s). This resumes the parent coroutine and not the child that suspended so the child will never execute again. This is because the example doesn't have access to it from the main function to properly resume from functionB since its in-accessible and hidden within the first functionA coroutine. Its not possible to resume chained coroutines from the parent, it just resume that parent where it's co_await suspended it. For all coroutines the child has a continuation pointer to the parent so when the child completes it will resume the parent, but not vice versa. Since you are manually resuming the parent here (functionA) the child never gets to resume.

The second problem is std::suspend_always isn't actionable within libcoro in that it will dead-end any coroutine. The proper way to suspend/pause a coroutine is with an executors yield() function, this would be on coro::thread_pool or coro::io_scheduler. I will note that if you had put the std::suspend_always into the functionA coroutine and manually resumed it that does work since you are manually resuming it, but in practice with libcoro this really doesn't work since any combination of parent/child coroutines awaiting will cause it to dead end and not resume, so I would recommend using an executor (coro::thread_pool or coro::io_scheduler) to properly handle executing and resuming tasks.

Here is a simple test case fixing the two issues:

TEST_CASE("issue-268", "[task]")
{
    // To properly execute/handle coroutines we need an execution context, we'll just make a 1 core thread pool as an example.
    auto tp = coro::thread_pool{coro::thread_pool::options{.thread_count = 1}};

    auto functionB = [&](int i) -> coro::task<void>
    {
        for (int offset = 0; offset < 10; ++offset)
        {
            std::cout << "Inside functionB, offset: " << i << ":" << offset << std::endl;
            co_await tp.yield();
            std::cout << "Resuming functionB, offset: " << i << ":" << offset << std::endl;
        }
        co_return;
    };

    auto functionA = [&]() -> coro::task<void>
    {
        // This schedules the coroutine onto the thread pool executor context.
        co_await tp.schedule();

        std::cout << "Inside functionA" << std::endl;
        for (int i = 0; i < 10; ++i)
        {
            co_await functionB(i);
        }
        std::cout << "Resuming functionA" << std::endl;
        co_return;
    };

    // Use libcoro's non-coroutine context synchronous wait function in conjunction with the executor context to properly
    // execute/pause/resume coroutines based on coro::task
    coro::sync_wait(functionA());
}
yuedai commented 4 months ago

Thank you so much for the input @jbaldwin.