Propagate errors through val coroutine hierarchy.

[davits]

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.

The bug can be reproduced by the following example:

emscripten::val fetch(const std::string& url) {
  co_return co_await emscripten::val::global("fetch")(url);
}
emscripten::val json(const std::string& url) {
  auto response = co_await fetch(url);
  co_return co_await response.call<emscripten::val>("json");
}
EMSCRIPTEN_BINDINGS(module) {
  function("json", &json);
}

let p = Module.json('invalid.url'); // JS error is printed, but p remains pending
try {
  let p = await Module.json('invalid.url'); // JS error is printed
} catch(e) {
  console.log('Caught coro error!'); // this line is never printed
}

[davits]

I will add tests with separate commit...
If there is a maintainer for the coroutine part, I would like to discuss couple of small improvements I have in mind.

[brendandahl]

I will add tests with separate commit... If there is a maintainer for the coroutine part, I would like to discuss couple of small improvements I have in mind.

@RReverser added the coroutine part. I think he's been busy with other projects, but may have time.

[RReverser]

This PR looks reasonable, and I wanted this functionality myself in the past, but I wasn't (stil am not) sure if we should have coroutines / promises in Embind behave differently from synchronous function calls.

Basically, I believe we should have a central flag that tells Embind "hey, store JS exceptions and make them catch-able from C++" regardless of whether function you're calling is synchronous, Asyncify one or a coroutine one - see #11496 for original issue.

Having partial implementation where only one of those types of functions throws in C++ but not others might be surprising to users.

[davits]

I understand. I think I can propogate rejections through the promise hierarchy as well...
Side note: I just noticed that coroutine frames are leaking, couldn't find handle.destroy() call anywhere, will add to this PR.

The #11496 had been dragged for quite some time, without conclusion...
I see value in the ability to write C++ coroutines which can catch errors from the "fetch" API for the current project my company is working on. If I pursue addition of a new experimental flag, which switches val coroutine rejection propogation to exceptions, will it be merged ? (Note for the future: initial implementation works via exceptions.)

About the improvement:
I want to enhance val to awaiter transformation with promise_type::await_transform with an ability to define custom awaitable types via template trait types.
I use that approach in my small coroutine engine which we use internally, and with its help I was able to get seamless integration with emscripten::val coroutines, like so:

struct JSAwaitable {...};
template <>
struct await_ready_trait<emscripten::val> {
    static JSAwaitable await_transform(Executor*, emscripten::val&& awaitable) {
        return JSAwaitable(std::move(awaitable));
    }
};
...
emscripten::val fetch(const std::string& url) {
  auto response = co_await emscripten::val::global("fetch")(url);
  co_return co_await response.call<emscripten::val>("text");
}
Task<std::string> do_work() {
  auto text = co_await fetch("data.url");
  ...
}

After adding similar await_transform in the val::promise it will be possible to write/integrate custom awaitables for custom user coroutines. For my case I will be able to seamlessly integrate in opposite direction and do co_await do_work(); in the val coroutine.
Basically add these function to the val::promise_type:

  val&& await_transform(val&& v) {
    return std::move(v);
  }
  template <typename T>
  decltype(auto) await_transform(T&& obj) {
    using RawT = std::remove_cvref_t<T>;
    return await_ready_trait<RawT>::await_transform(std::forward<T>(obj));
  }

What do you think ?

[RReverser]

Side note: I just noticed that coroutine frames are leaking, couldn't find handle.destroy() call anywhere, will add to this PR.

Do you actually see the leaks or do you only suspect it because there is no destroy()?

Explicit destroy is necessary only in some situations, like when using generators where you drive the coroutine rather it driving itself like happens with promises. I believe the way I implemented it back then was to ensure that coroutine destroys itself automatically, but if that doesn't actually work, please raise a separate issue demonstrating the problem.

(I'll respond to the rest later, currently on the phone only)

[davits]

Sorry, my mistake: auto final_suspend() noexcept { return std::suspend_never{}; } suspend_never at the finish ensures automatic coroutine frame destruction.

[RReverser]

For my case I will be able to seamlessly integrate in opposite direction and do co_await do_work(); in the val coroutine.

For what it's worth, this is already supported - I needed custom awaitables too in some places, so added that a while back - just by subclassing the awaiter instead.

Here's the test showing an example and verifying it works:

emscripten/test/embind/test_val_coro.cpp

Lines 23 to 38 in 2e2ec08

	// Test that we can subclass and make custom awaitable types.
	template <typename T>
	class typed_promise: public val {
	public:
	typed_promise(val&& promise): val(std::move(promise)) {}
	auto operator co_await() const {
	struct typed_awaiter: public val::awaiter {
	T await_resume() {
	return val::awaiter::await_resume().template as<T>();
	}
	};
	return typed_awaiter(*this);
	}
	};

Would this way of overloading work for your usecase?

[davits]

Lets move the discussion about that into a separate PR (or another platform), to not clutter this one.

Getting back to the current PR. It is ready for review.
And can someone please help me identify the problem with the tests ? I didn't change any compile settings and am getting compile settings incompatibility failure for bind.o file. The thing is that tests passed with previous commit and I didn't even change anything in bind.cpp with my last commit.

[brendandahl]

As for the compiler errors, it seem something broke from an LLVM update and it appears to be fixed now. I've restarted the testers.

[davits]

OK, so it seems like that embind-rtti library is compiled without -pthread flag while the test itself (I am experimenting with the core0.test_pthread_create_embind_stack_check) is compiled with the -pthread flag and they are linked together...
I added the _emval_coro_reject() function to the bind.cpp, which calls the above mentioned reject() function at the end of the call hierarchy. And since reject is a JS function it goes through the Signature::get_method_caller() adding a thread_local storage thus requirement to be compiled with the -pthread flag.

Is it OK to require libembind and libembind-rtti libraries to be compiled with the same threadness as the application and how do I do so for the tests ?

EDIT:
Turned the libembind into a MTLibrary. The question whether it is acceptable remains.

[sbc100]

Turned the libembind into a MTLibrary. The question whether it is acceptable remains.

Seems like a reasonable solution yes. Its a pretty small library anyway.

[davits]

This is my first time contributing to the project, so I am not familiar with the process.
If I need to change something (title for example) to signal that the PR is ready for the review, please let me know.
Currently I am assuming that someone has to find some time to review it, but want to make sure :)

[sbc100]

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

[RReverser]

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.

[sbc100]

ping @aheejin

[sbc100]

ping @aheejin