Naios / continuable

C++14 asynchronous allocation aware futures (supporting then, exception handling, coroutines and connections)
https://naios.github.io/continuable/
MIT License
815 stars 44 forks source link

out of order destruction #61

Open Spongman opened 1 year ago

Spongman commented 1 year ago

@Naios

I'm not sure if this is an issue with the continuable lib, or if I'm doing something wrong, or if I just funamentally don't understand how coroutines are supposed to work...

The issue is that the destructor for a local object in a coroutine is getting called after the co_return has 'returned' control to the caller:

https://gcc.godbolt.org/z/cT7rv93hP

#include <string>
#include <iostream>

#include <coroutine>
#include <https://gist.githubusercontent.com/Spongman/e775a7cd44246f861c146e307fea2761/raw/77b84c9f1cbb33abaecc6b43f2067bd537f092d1/continuable-63e3ed4edccfc3bcc3c8ceb12b857432eb4ee9a3-clang-fix.hpp>

struct RAII {
    std::string name_;
    RAII(std::string name) : name_{name} { std::cerr << name_ << "\n"; }
    ~RAII() { std::cerr << "~" << name_ << "\n"; }
};

cti::continuable<> outer()
{
    RAII _{__FUNCTION__};
    std::cerr << __FUNCTION__ << " co_return\n";
    co_return;
}

int main() {

    outer()
        .then([]() {
            std::cerr << "then\n";
        })
        .fail([](std::exception_ptr p) {
            std::cerr << "fail\n";
        })
        .apply(cti::transforms::wait());

    return 0;
}

this outputs:

outer
outer co_return
then
~outer

i would expect that last two lines to be swapped, as it would be if this were a normal synchronous function.

maybe I'm managing the lifetime of the future incorrectly? or that just the way it's supposed to work with coroutines?


Commit Hash

master (plus my clang fix)

Naios commented 1 year ago

@Spongman I'm quite certain that this behaviour is as expected with the following reasoning:

  1. The return value of outer() is part of a larger expression and therefore kept alive to the end of the entire expression.
  2. co_return performs a "final suspend" on the coroutine, therefore the coroutine is put to sleep until the coroutine_handle is destroyed. Therefore the stack inside the coroutine is kept alive.
  3. Because the coroutine_handle is destroyed when destructing the return type of outer() and not when reaching co_return, the message is printed at the end of the expression.
  4. As far as I remind, destroying the coroutine_handle from outside is neccessary to prevent a dangling handle.
Spongman commented 1 year ago

Yeah, i figured it had something to do with the lifetime of the coroutine. i'm not familiar with the internal details, but it does seem to me like the frame should be deleted before the callback is invoked.

Here's another example that illustrates this independent of the way it's invoked:

cti::coroutine<> inner()     |  void inner()
{                            |  {
  Foo local;                 |    Foo local;
  co_return;                 |    return;
}                            |  }
                             | 
cti::coroutine<> outer()     |  void outer()
{                            |  {
  co_await inner();          |    inner();
  cerr << "inner done\n";    |    cerr << "inner done\n";
  co_return;                 |    return;
}                            |  }

In the synchronous version on the right, obviously local's destructor is called before "inner done" is printed.

However, in the async version on the left, local's destructor isn't called until after "inner done" is printed.

Regardless of the internal implementation, that just doesn't seem right to me: it means the scope rules of RAII are essentially broken in coroutines.

Naios commented 1 year ago

Ok, I totally agree with you now.

I'm not sure if this is easy to fix, since in general it is not explicitly specified by the library design when the destruction of the outer continuation occurs after it was used. Therefore, the behavior you describe may also vary depending on whether you resolve the continuable/coroutine asynchronously or synchronously. For callable objects this behavior has never been an issue before, but for coroutine handles we should not keep the scope of the coroutine alive because this can lead to very unexpected results as you have said.