Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement native unwind behaviour for coroutine cancellation #172

Merged
merged 5 commits into from
Aug 22, 2020

Conversation

lewissbaker
Copy link
Contributor

Coroutines now automatically unwind themselves if an awaited
operation completes with 'done'. Note that this 'unwind' process
is not catchable.

This signal will be transported up the stack until it hits a consumer
that does handle the 'done' signal. This typically means wrapping
the task<T> in a sender-algorithm that translates the done-signal
into some other signal that a coroutine can then handle.
eg. as an error/exception or as a value.

This means we don't have to unwrap std::optional values every
time we co_await something.

This relies on coroutine promise_types implementing a new
unhandled_done() method, similar to unhandled_exception(),
that handles the 'done' signal for that particular coroutine type.

The task type simply forwards the signal on to the awaiting
coroutine's promise.unhandled_done() method. This recursion
is terminated by the connect_awaitable() implementation which
implements unhandled_done() to forward on to the receiver's
set_done() implementation.

Copy link
Collaborator

@ericniebler ericniebler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rad.

include/unifex/await_transform.hpp Outdated Show resolved Hide resolved
include/unifex/connect_awaitable.hpp Outdated Show resolved Hide resolved

coro::coroutine_handle<> continuation_;
done_callback* doneCallback_ = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we ever consciously choose camelCase for variables? My preference is for snake_case. Obviously not a problem you should be solving here, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have always been writing camelCase for non-public variable names.
Public data-members that form part of the interface are still snake_case.
Although I've noticed others have been using snake_case variable names, so we're not consistent.

Copy link
Contributor

@kirkshoop kirkshoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me smile!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2020
@ericniebler
Copy link
Collaborator

ericniebler commented Aug 20, 2020

@GorNishanov should get a load of this.

include/unifex/task.hpp Outdated Show resolved Hide resolved
@ispeters
Copy link
Contributor

This is beautiful!

@ericniebler
Copy link
Collaborator

There are a lot of senders in unifex that don't have a sends_done static member. It would be work looking into why only MSVC is complaining, though. I'm guessing there is some unnecessary instantiation happening with MSVC due to return type computations happening too eagerly, or something. It might be unavoidable.

Base automatically changed from task-cancellation to master August 21, 2020 22:03
Lewis Baker added 3 commits August 21, 2020 17:03
Coroutines now unconditionall unwind themselves if an awaited
operation completes with 'done'.

This signal will be transported up the stack until it hits a consumer
that does handle the 'done' signal. This typically means wrapping
the task<T> in a sender-algorithm that translates the done-signal
into some other signal that a coroutine can then handle.
eg. as an error/exception or as a value.

This means we don't have to unwrap std::optional values every
time we co_await something.

This relies on coroutine promise_types implementing a new
'unhandled_done()' method, similar to 'unhandled_exception()',
that handles the 'done' signal for that particular coroutine type.

The 'task' type simply forwards the signal on to the awaiting
coroutine's promise.unhandled_done() method. This recursion
is terminated by the connect_awaitable() implementation which
implements unhandled_done() to forward on to the receiver's
set_done() implementation.
…meters.

The task<T> unhanded_done callbacks are now
namespace-scope functions that depend only
on the parent promise type, not on T.

Eliminate a conditional in unhandled_done() by
initialising the doneCallback member with a
default callback that terminates.
Implementation now only depends on the essential template
arguments. This should reduce code-bloat compared to the
lambda approach, which would have been instantiated for
each kind of Awaitable the receiver was connected to, even
though it didn't depend on the Awaitable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants