lewissbaker / cppcoro

A library of C++ coroutine abstractions for the coroutines TS
MIT License
3.43k stars 472 forks source link

Deprecate and remove 'eager' task classes `task` and `shared_task` #29

Closed lewissbaker closed 7 years ago

lewissbaker commented 7 years ago

Once we have a way to synchronously block waiting for a task in #27 and when_all in #10 there shouldn't be any need for the eagerly started task types any more and we can limit use to just the lazy task types (ie. lazy_task and shared_lazy_task)

One of the big motivations for getting rid of eagerly-started tasks is that it is difficult to write exception-safe code with use of eagerly-started tasks that aren't immediately co_awaited.

We don't want to leave dangling tasks/computation as this can lead to ignored/uncaught exceptions or unsynchronised access to shared resources.

For example, consider the case where we want to execute two tasks concurrently and wait for them both to finish before continuing, using their results:

task<A> do1();
task<B> do2();

task<> do1and2concurrently_unsafe()
{
  // The calls to do1() and do2() start the tasks eagerly but are potentially executed
  // in any order and either one of them could fail after the other has already started
  // a concurrent computation.
  auto [a, b] = cppcoro::when_all(do1(), do2());

  // use a and b ...
}

To implement this in an exception-safe way we'd need to modify the function as follows:

task<> do1and2concurrently_safe()
{
  // This implicitly starts executing the task and it executes concurrently with the
  // rest of the logic below until we co_await the task.
  task<A> t1 = do1();

  // Now we need to start the do2() task but since this could fail we need
  // to start it inside a try/catch so we can wait for t1 to finish before it
  // goes out of scope (we don't want to leave any dangling computation).
  task<B> t2;
  std::exception_ptr ex;
  try
  {
    // This call might throw, since it may need to allocate a coroutine frame
    // which could fail with std::bad_alloc.
    t2 = do2();
  }
  catch (...)
  {
    // Don't want to leave t1 still executing but we can't co_await inside catch-block
    // So we capture current exception and do co_await outside catch block.
    ex = std::current_exception();
  }

  if (ex)
  {
    // Wait until t1 completes before rethrowing the exception.
    co_await t1.when_ready();
    std::rethrow_exception(ex);
  }

  // Now that we have both t1 and t2 started successfully we can use when_all() to get the results.
  auto [a, b] = when_all(std::move(t1), std::move(t2));

  // use a, b ...  
}

Compared with lazy_task version which is both concise and exception-safe:

lazy_task<> do1();
lazy_task<> do2();

lazy_task<> do1and2concurrently()
{
  // The calls to do1() and do2() can still execute in any order but all they
  // do is allocate coroutine frames, they don't start any computation.
  // If the second call fails then the normal stack-unwinding will ensure the
  // first coroutine frame is destroyed. It doesn't need to worry about waiting
  // for the first task to complete. Either they both start or none of them do.
  auto [a, b] = cppcoro::when_all(do1(), do2());

  // use a and b ...
}

With a lazy_task, the task is either being co_awaited by some other coroutine or it is not executing (it has either not yet started, or has completed executing) and so the lazy_task is always safe to destruct and will free the coroutine frame it owns.

A side-benefit of using lazy_task everywhere is that it can be implemented without the need for std::atomic operations to synchronise coroutine completion and awaiter. This can have some potential benefits for performance by avoiding use of atomic operations for basic sequential flow of execution.

lewissbaker commented 7 years ago

Started the process of removing use of task<T> from tests in b78bc3ed7b0c2c13fb92483a6b34e31589e22286.

There are still the where_all_tests.cpp and where_all_ready_tests.cpp to go but I've been running into some crashes when updating these that I need to investigate.

lewissbaker commented 7 years ago

Commit 722fe1b24a3d0cb5fbafa17ee167e2a024f59bf7 removes use of eager tasks from where_all_ready_tests.cpp.

Only where_all_tests.cpp to go.

lewissbaker commented 7 years ago

Commit 421de76f189c1f38a621898eaec77dc3700b9c47 removes last of eager task usage from tests (except where testing eager tasks themselves).

lewissbaker commented 7 years ago

Commit 64b0a049f9e07eef7082b1d4de31686975c3cdc8 removes the task<T> and shared_task<T> classes and updates the README.

All that remains now is renaming lazy_task -> task and shared_lazy_task -> shared_task.

lewissbaker commented 7 years ago

Commit bf9eb2c58110a650cc4128bef03c8d9d87be62f7 has implemented the rename.

Closing.