Skip to content

Commit

Permalink
Fix co_nothrow throwing when co_scope_exit is inside
Browse files Browse the repository at this point in the history
Summary: `folly::coro::co_nothrow` is supposed to never throw an exception, but if it wraps a `Task` with `folly::coro::co_scope_exit` inside, then it actually rethrows the exception, which seems like a clear bug.

Reviewed By: iahs

Differential Revision: D53438250

fbshipit-source-id: d32fe1d3385c58142035fe7bedb5a2d1e6583918
  • Loading branch information
denvned authored and facebook-github-bot committed Feb 6, 2024
1 parent 01326c8 commit 5a6ba76
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 7 deletions.
20 changes: 15 additions & 5 deletions folly/experimental/coro/ScopeExit.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include <folly/tracing/AsyncStack.h>

#include <folly/ExceptionWrapper.h>
#include <folly/Executor.h>
#include <folly/ScopeGuard.h>
#include <folly/experimental/coro/Coroutine.h>
Expand Down Expand Up @@ -100,27 +101,35 @@ class ScopeExitTaskPromiseBase {
promise.next_.promise().setContext(
promise.continuation_,
promise.parentAsyncFrame_,
promise.executor_.get_alias());
promise.executor_.get_alias(),
std::move(promise.error_));
return promise.next_;
}

/// If we reached this point, then this ScopeExitTask is the final one to
/// be executed on the parent task, and we can now pop the parent's async
/// frame before calling the original parent's continuation.
folly::popAsyncStackFrameCallee(*promise.parentAsyncFrame_);
return promise.continuation_;
if (promise.error_) {
auto [handle, frame] =
promise.continuation_.getErrorHandle(promise.error_);
return handle.getHandle();
}
return promise.continuation_.getHandle();
}

[[noreturn]] void await_resume() noexcept { folly::assume_unreachable(); }
};

void setContext(
coroutine_handle<> continuation,
ExtendedCoroutineHandle continuation,
folly::AsyncStackFrame* asyncFrame,
folly::Executor::KeepAlive<> executor) {
folly::Executor::KeepAlive<> executor,
folly::exception_wrapper error = {}) {
continuation_ = continuation;
parentAsyncFrame_ = asyncFrame;
executor_ = std::move(executor);
error_ = std::move(error);
}

suspend_always initial_suspend() noexcept { return {}; }
Expand Down Expand Up @@ -151,9 +160,10 @@ class ScopeExitTaskPromiseBase {
template <typename... Args>
friend class ScopeExitTask;

coroutine_handle<> continuation_;
ExtendedCoroutineHandle continuation_;
folly::AsyncStackFrame* parentAsyncFrame_;
folly::Executor::KeepAlive<> executor_;
folly::exception_wrapper error_;
coroutine_handle<ScopeExitTaskPromiseBase> next_;
};

Expand Down
6 changes: 4 additions & 2 deletions folly/experimental/coro/Task.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,11 @@ class TaskPromiseBase {
// a virtual wrapper over coroutine_handle that handles the pop for us.
if (promise.scopeExit_) {
promise.scopeExit_.promise().setContext(
promise.continuation_.getHandle(),
promise.continuation_,
&promise.asyncFrame_,
promise.executor_.get_alias());
promise.executor_.get_alias(),
promise.result_.hasException() ? promise.result_.exception()
: exception_wrapper{});
return promise.scopeExit_;
}

Expand Down
20 changes: 20 additions & 0 deletions folly/experimental/coro/test/ScopeExitTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,26 @@ TEST_F(ScopeExitTest, OneExitActionThroughNoThrow) {
EXPECT_EQ(count, 1);
}

TEST_F(ScopeExitTest, ExitActionInsideNoThrow) {
EXPECT_THROW(
folly::coro::blockingWait([this]() -> Task<> {
try {
co_await co_nothrow([this]() -> Task<> {
co_await co_scope_exit([this]() -> Task<> {
++count;
co_return;
});
throw std::runtime_error("Something bad happened!");
co_return;
}());
} catch (...) {
ADD_FAILURE();
}
}()),
std::runtime_error);
EXPECT_EQ(count, 1);
}

TEST_F(AsyncGeneratorScopeExitTest, PartiallyConsumed) {
auto makeGenerator = [&]() -> folly::coro::CleanableAsyncGenerator<int> {
co_await co_scope_exit([&]() -> folly::coro::Task<> {
Expand Down

0 comments on commit 5a6ba76

Please sign in to comment.