Chained coroutine task example segfaults

Question

In "C++20 - The Complete Guide", Josuttis uses the following code to show how making a coroutine implement the interface of an awaiter makes it possible for it to store sub-coroutines that are resumed automatically when the outer coroutine is resumed. I have understood the explanation and the code (I think), but surprisingly enough, when I compile and launch the code:

g++ -std=c++20 corocorosub.cpp -O3 -o corocorosub && ./corocorosub

I get a segfault error:

MAIN: callCoro() initialized
  callCoro(): CALL coro()
MAIN: callCoro() suspended
    coro(): PART1
MAIN: callCoro() suspended
    coro(): PART2
MAIN: callCoro() suspended
  callCoro(): coro() done
MAIN: callCoro() suspended
Segmentation fault (core dumped)

Even more surprisingly, on Compile Explorer it works just fine.

I have no clue what could be wrong, and where (the code, or the compiler).

Here's the code:

#include <coroutine>
#include <exception>
#include <iostream>

class [[nodiscard]] CoroTaskSub {
 public:
  struct promise_type;
  using CoroHdl = std::coroutine_handle<promise_type>;
 private:
  CoroHdl hdl;

 public:
  struct promise_type {
    CoroHdl subHdl = nullptr;

    auto get_return_object() {
      return CoroTaskSub{CoroHdl::from_promise(*this)};
    }
    auto initial_suspend() { return std::suspend_always{}; }
    void unhandled_exception() { std::terminate(); }
    void return_void() { }
    auto final_suspend() noexcept { return std::suspend_always{}; }
  };

  CoroTaskSub(auto h)
   : hdl{h} {
  }
  ~CoroTaskSub() {
    if (hdl) {
      hdl.destroy();
    }
  }
  CoroTaskSub(const CoroTaskSub&) = delete;
  CoroTaskSub& operator=(const CoroTaskSub&) = delete;

  bool resume() const {
    if (!hdl || hdl.done()) {
      return false;
    }
    CoroHdl innerHdl = hdl;
    while (innerHdl.promise().subHdl && !innerHdl.promise().subHdl.done()) {
      innerHdl = innerHdl.promise().subHdl;
    }
    innerHdl.resume();         
    return !hdl.done();
  }

  bool await_ready() { return false; }
  void await_suspend(auto awaitHdl) {
    awaitHdl.promise().subHdl = hdl;
  }
  void await_resume() {}
};

CoroTaskSub coro()
{
  std::cout << "    coro(): PART1\n";
  co_await std::suspend_always{};
  std::cout << "    coro(): PART2\n";
}

CoroTaskSub callCoro()
{
  std::cout << "  callCoro(): CALL coro()\n";
  co_await coro();
  std::cout << "  callCoro(): coro() done\n";
  co_await std::suspend_always{};
  std::cout << "  callCoro(): END\n";
}

int main()
{
  auto coroTask = callCoro();
  std::cout << "MAIN: callCoro() initialized\n";

  while (coroTask.resume()) {
    std::cout << "MAIN: callCoro() suspended\n";
  }

  std::cout << "MAIN: callCoro() done\n";
}

Answer 1

The problem here is that the "inner" coroutine completes and destroys itself, but the "outer" coroutine doesn't know that this happened, still has a handle to it, and is still trying to access the frame.

The solution is that there somehow needs to be more cooperation between the "inner" and "outer" coroutines. One simple way, though possibly not the best way (I am hardly a coroutine expert), is to have the coroutine promise itself track both its child and its parent:

struct promise_type {
    CoroHdl subHdl = nullptr;
    CoroHdl parent = nullptr;
};

Where parent gets additionally set in await_suspend (I went ahead and replaced the auto with the type that we know it is):

void await_suspend(CoroHdl awaitHdl) {
    awaitHdl.promise().subHdl = hdl;
    hdl.promise().parent = awaitHdl;
}

And await_resume removes itself:

void await_resume() {
    if (hdl.promise().parent) {
        hdl.promise().parent.promise().subHdl = nullptr;
    }
}

You can see that this is ASAN-clean, because now the "outer" coroutine will cease to have a child when the "inner" one completes.

A different way to do this, what std::generator is specified to do, is to have final_suspend return a distinct awaiter. You can see its reference implementation here.

Answer 2

For the fun of investigating, I've found a couple of other (sub-optimal) ways to fix the issue, at least in this specific example. I hope that can help other readers in exploring this fascinating topic of coroutines!

---

Since the problem is due to the CoroTaskSub retuend by coro() being destroyed right after the expression co_await coro(); is completely evaluated, and callCoro()'s frame still having the subHdl pointing to it and using it via innerHdl.promise().subHdl.done(), one solution is to give a name to coro() to keep it alive for as long as the callCoro() is alive:

CoroTaskSub callCoro()
{
  std::cout << "  callCoro(): CALL coro()\n";
  auto Coro = coro();
  co_await Coro;
  std::cout << "  callCoro(): coro() done\n";
  co_await std::suspend_always{};
  std::cout << "  callCoro(): END\n";
}

This means that

• the subHdl of callCoro()'s will always point to something valid, and .done() will keep returning true for it, so that the body of the while will not be executing, innerHdl will resolve to hdl, and no attempt to resume coro() will be made after it reaches the final suspend point;
• on the other hand, the solution above undesirably keep coro() alive more than it needs to be.

---

Another way is to set subHdl back to nullptr as soon as it is .done():

    while (innerHdl.promise().subHdl && !innerHdl.promise().subHdl.done()) {
      innerHdl = innerHdl.promise().subHdl;
    }
    if (innerHdl.promise().subHdl && innerHdl.promise().subHdl.done()) {
        innerHdl.promise().subHdl = nullptr;
    }

This way, next time .resume() is called for coroTask (i.e. callCoro()), it will not execute the body of the while, just like no sub-coroutine had ever been set.

To be precise, since going past the while means that either innerHdl.promise().subHdl.done() was true, or innerHdl.promise().subHdl was nullptr in the first place, the if is actually redundant, and we can just do this:

    while (innerHdl.promise().subHdl && !innerHdl.promise().subHdl.done()) {
      innerHdl = innerHdl.promise().subHdl;
    }
    innerHdl.promise().subHdl = nullptr;

After all, once we've got to the deepest innerHdl not-done() yet, we know that its subHdl is either .done() or nullptr as it was never set in the first place, in which case setting it to nullptr unconditionally should be just fine.