GorNishanov / coroutines-ts

20 stars 2 forks source link

Behaviour of an `unhandled_exception()` method that throws an exception is under-specified #17

Open lewissbaker opened 7 years ago

lewissbaker commented 7 years ago

If a coroutine's promise object throws an exception from promise_type::unhandled_exception() (eg. if it rethrows the current exception), what state is the coroutine left in?

Is the coroutine suspended prior to the exception propagating out of the initial-call/resume-call?

Is the coroutine frame safe to destroy by the caller or is it already destroyed?

Is the promise object destroyed? The pseudo-code would imply that it is.

Is the coroutine done()? The definition of done() implies that it's only true if suspended at final_suspend() but control flow of the psuedo-code in [N4663] implies that final_suspend() would be skipped in this case.

Wording of [N4663] section 8.4.4(3) doesn't seem to clearly define semantics of this case.

lewissbaker commented 7 years ago

@CaseyCarter suggested that unhandled_exception() implementations should be declared noexcept. ie. treat an exception propagating out of unhandled_exception() the same as an exception being thrown during stack-unwinding and call std::terminate().

lewissbaker commented 7 years ago

Thinking about this a bit more, perhaps the wording in section 8.4.4(9) covers this situation:

The coroutine state is destroyed when control flows off the end of the coroutine or the destroy member function (18.11.2.5) of an object of type std::experimental::coroutine_handle<P> associated with this coroutine is invoked. In the latter case objects with automatic storage duration that are in scope at the suspend point are destroyed in the reverse order of the construction. The storage for the coroutine state is released by calling a non-array deallocation function (3.7.4.2). If destroy is called for a coroutine that is not suspended, the program has undefined behavior.

If we read this in conjunction with 8.4.4(3):

{
  P p;
  co_await p.initial_suspend(); // initial suspend point
  try { F } catch(...) { p .unhandled_exception(); }
final_suspend :
  co_await p.final_suspend(); // final suspend point
}

Perhaps this then implies that if the coroutine runs to completion by throwing an exception out of the coroutine (ie. in co_await initial_suspend(), co_await final_suspend() or unhandled_exception() calls) then the promise, parameter-copies and coroutine frame are destroyed during unwinding of the stack before the exception is propagated to the caller/resumer.

ie. that it is as if there was another big try/catch around the body of the coroutine body, as in:

{
  try
  {
    {
      P p;
      co_await p.initial_suspend();
      try { F } catch(...) { p.unhandled_exception(); }
final_suspend:
      co_await p.final_suspend();
    }
    <destroy-coroutine-frame>
  }
  catch (...)
  {
    <destroy-coroutine-frame>
    throw;
  }
}

Is my interpretation correct here, @GorNishanov ?

GorNishanov commented 7 years ago

For unhandled_exception, requiring it to be noexcept is a good suggestion. Though, I agree, it is somewhat underspecified what happens when exception leaks outside of the body of coroutine.

Here is what I would expect as the coroutine user:

Prior to the first suspend, when coroutine is executing during initial call to a coroutine, it behaves like a normal function call, if an exception propagates, everything is unrolled, freed, destroyed, etc.

If an exception leaks from a coroutine after it was resumed, we are in UB land. Hence, throwing from unhandled_exception, final_suspend and any of the calls to awaitable returned from final_suspend (ctor, dtor, await_ready, await_suspend) is not good.

I think that we can harden the specification by requesting std::terminate if an exception leaks from the resumed part of the coroutine. I think I would prefer std::terminate to trying to do some cleanup as you shown above..

Consider coroutines that have final suspend point and destroyed by a RAII class holding the coroutine-handle. If an exception leaks from the resume part, coroutine is not suspended, hence it is UB to destroy it. Cleaning it up

lewissbaker commented 7 years ago

I'm not sure it makes sense to arbitrarily restrict unhandled_exception() to be noexcept.

For coroutine-types that have an RAII class that owns the coroutine and that calls coroutine_handle<>::destroy() when it destructs, they would need to either wrap all calls to coroutine_handle<>::resume() so that they can detect when the coroutine frame has already been destroyed or would need to ensure that initial_suspend(), final_suspend() and unhandled_exception() are all noexcept.

It seems more consistent to me to have the coroutine frame automatically destroyed once the coroutine runs to completion in all code-paths regardless of whether the coroutine is executing in the initial call or in a call to coroutine_handle<>::resume().

The compiler should be able to avoid generating cleanup logic for coroutines if relevant promise methods are all noexcept.

ericniebler commented 7 years ago

It seems more consistent to me to have the coroutine frame automatically destroyed once the coroutine runs to completion in all code-paths regardless of whether the coroutine is executing in the initial call or in a call to coroutine_handle<>::resume().

Yes, this.