Skip to content

Propagate errors through val coroutine hierarchy. #23653

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

Merged
merged 12 commits into from
Jun 3, 2025
Merged
Next Next commit
Propagate errors through val call hierarchy.
Fix bug when failure or exception happening in a >1 level of the val
coroutine call hierarchy is not being propagated. As a result only
deep most level coroutine promise is rejected, other val promises
including top most one remain in a perpetual pending state.
  • Loading branch information
davits committed Feb 25, 2025
commit 7ec6306bf30d19dc37aab7314f8e2f7527cfef95
40 changes: 20 additions & 20 deletions src/lib/libemval.js
Original file line number Diff line number Diff line change
Expand Up @@ -467,33 +467,33 @@ var LibraryEmVal = {
return result.done ? 0 : Emval.toHandle(result.value);
},

_emval_coro_suspend__deps: ['$Emval', '_emval_coro_resume'],
_emval_coro_suspend: async (promiseHandle, awaiterPtr) => {
var result = await Emval.toValue(promiseHandle);
__emval_coro_resume(awaiterPtr, Emval.toHandle(result));
_emval_coro_suspend__deps: ['$Emval', '_emval_coro_resume', '_emval_coro_reject'],
_emval_coro_suspend: (promiseHandle, awaiterPtr) => {
Emval.toValue(promiseHandle)
.then(result => __emval_coro_resume(awaiterPtr, Emval.toHandle(result)))
.catch(error => __emval_coro_reject(awaiterPtr, Emval.toHandle(error)));
},

_emval_coro_make_promise__deps: ['$Emval', '__cxa_rethrow'],
_emval_coro_make_promise__deps: ['$Emval'],
_emval_coro_make_promise: (resolveHandlePtr, rejectHandlePtr) => {
return Emval.toHandle(new Promise((resolve, reject) => {
const rejectWithCurrentException = () => {
try {
// Use __cxa_rethrow which already has mechanism for generating
// user-friendly error message and stacktrace from C++ exception
// if EXCEPTION_STACK_TRACES is enabled and numeric exception
// with metadata optimised out otherwise.
___cxa_rethrow();
} catch (e) {
// But catch it so that it rejects the promise instead of throwing
// in an unpredictable place during async execution.
reject(e);
}
};

{{{ makeSetValue('resolveHandlePtr', '0', 'Emval.toHandle(resolve)', '*') }}};
{{{ makeSetValue('rejectHandlePtr', '0', 'Emval.toHandle(rejectWithCurrentException)', '*') }}};
{{{ makeSetValue('rejectHandlePtr', '0', 'Emval.toHandle(reject)', '*') }}};
}));
},

_emval_coro_exception_to_error__deps: ['$Emval', '__cxa_rethrow'],
_emval_coro_exception_to_error: () => {
try {
// Use __cxa_rethrow which already has mechanism for generating
// user-friendly error message and stacktrace from C++ exception
// if EXCEPTION_STACK_TRACES is enabled and numeric exception
// with metadata optimised out otherwise.
___cxa_rethrow();
} catch (e) {
return Emval.toHandle(e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aheejin does this look reasonable to you? Or is the maybe a better way to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we wait for @aheejin's review or merge as-is? This piece of code was before this PR (I added it a while back because it was the only way to make it work that I found at the time), so I'm leaning towards let's merge it anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping @aheejin

},
};

addToLibrary(LibraryEmVal);
1 change: 1 addition & 0 deletions src/lib/libsigs.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ sigs = {
_emval_await__sig: 'pp',
_emval_call__sig: 'dpppp',
_emval_call_method__sig: 'dppppp',
_emval_coro_exception_to_error__sig: 'p',
_emval_coro_make_promise__sig: 'ppp',
_emval_coro_suspend__sig: 'vpp',
_emval_decref__sig: 'vp',
Expand Down
32 changes: 26 additions & 6 deletions system/include/emscripten/val.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#include <variant>
#endif


namespace emscripten {

class val;
Expand Down Expand Up @@ -118,6 +117,7 @@ EM_VAL _emval_iter_next(EM_VAL iterator);

#if __cplusplus >= 202002L
void _emval_coro_suspend(EM_VAL promise, void* coro_ptr);
EM_VAL _emval_coro_exception_to_error();
EM_VAL _emval_coro_make_promise(EM_VAL *resolve, EM_VAL *reject);
#endif

Expand Down Expand Up @@ -712,11 +712,13 @@ class val::awaiter {
// - initially created with promise
// - waiting with a given coroutine handle
// - completed with a result
std::variant<val, std::coroutine_handle<val::promise_type>, val> state;
// - rejected with an error
std::variant<val, std::coroutine_handle<val::promise_type>, val, val> state;

constexpr static std::size_t STATE_PROMISE = 0;
constexpr static std::size_t STATE_CORO = 1;
constexpr static std::size_t STATE_RESULT = 2;
constexpr static std::size_t STATE_ERROR = 3;

public:
awaiter(const val& promise)
Expand All @@ -743,9 +745,20 @@ class val::awaiter {
coro.resume();
}

void reject_with(val&& error) {
auto coro = std::move(std::get<STATE_CORO>(state));
state.emplace<STATE_ERROR>(std::move(error));
coro.resume();
}

// `await_resume` finalizes the awaiter and should return the result
// of the `co_await ...` expression - in our case, the stored value.
val await_resume() { return std::move(std::get<STATE_RESULT>(state)); }
val await_resume() {
if (state.index() == STATE_ERROR) {
throw std::get<STATE_ERROR>(state);
}
return std::move(std::get<STATE_RESULT>(state));
}
};

inline val::awaiter val::operator co_await() const {
Expand All @@ -756,7 +769,7 @@ inline val::awaiter val::operator co_await() const {
// that compiler uses to drive the coroutine itself
// (`T::promise_type` is used for any coroutine with declared return type `T`).
class val::promise_type {
val promise, resolve, reject_with_current_exception;
val promise, resolve, reject;

public:
// Create a `new Promise` and store it alongside the `resolve` and `reject`
Expand All @@ -766,7 +779,7 @@ class val::promise_type {
EM_VAL reject_handle;
promise = val(internal::_emval_coro_make_promise(&resolve_handle, &reject_handle));
resolve = val(resolve_handle);
reject_with_current_exception = val(reject_handle);
reject = val(reject_handle);
}

// Return the stored promise as the actual return value of the coroutine.
Expand All @@ -779,7 +792,14 @@ class val::promise_type {
// On an unhandled exception, reject the stored promise instead of throwing
// it asynchronously where it can't be handled.
void unhandled_exception() {
reject_with_current_exception();
try {
std::rethrow_exception(std::current_exception());
} catch (const val& error) {
reject(error);
} catch (...) {
val error = val(internal::_emval_coro_exception_to_error());
reject(error);
}
}

// Resolve the stored promise on `co_return value`.
Expand Down
4 changes: 4 additions & 0 deletions system/lib/embind/bind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ void _emval_coro_resume(val::awaiter* awaiter, EM_VAL result) {
awaiter->resume_with(val::take_ownership(result));
}

void _emval_coro_reject(val::awaiter* awaiter, EM_VAL error) {
awaiter->reject_with(val::take_ownership(error));
}

}

namespace {
Expand Down
1 change: 1 addition & 0 deletions tools/emscripten.py
Original file line number Diff line number Diff line change
Expand Up @@ -1093,6 +1093,7 @@ def create_pointer_conversion_wrappers(metadata):
'emscripten_proxy_finish': '_p',
'emscripten_proxy_execute_queue': '_p',
'_emval_coro_resume': '_pp',
'_emval_coro_reject': '_pp',
'emscripten_main_runtime_thread_id': 'p',
'_emscripten_set_offscreencanvas_size_on_thread': '_pp__',
'fileno': '_p',
Expand Down