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 emscripten_promise_race in promise.h #19154

Merged
merged 2 commits into from
Apr 12, 2023
Merged

Implement emscripten_promise_race in promise.h #19154

merged 2 commits into from
Apr 12, 2023

Conversation

tlively
Copy link
Member

@tlively tlively commented Apr 10, 2023

Implement emscripten_promise_race in promise.h

This function propagates the result of its first input promise to settle,
whether it is fulfilled or rejected.

update library sig file

@tlively
Copy link
Member Author

tlively commented Apr 10, 2023

@tlively tlively force-pushed the promise-race branch 2 times, most recently from 8a0f9f7 to 1814020 Compare April 11, 2023 21:48
Base automatically changed from promise-any to main April 12, 2023 03:31
tlively added 2 commits April 11, 2023 20:41
This function propagates the result of its first input promise to settle,
whether it is fulfilled or rejected.
@tlively
Copy link
Member Author

tlively commented Apr 12, 2023

@sbc100 this should be ready to go.

emscripten_console_log("test_race");
assert(data == (void*)7);

em_promise_t in[2] = {emscripten_promise_create(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that it matters but I think you can drop the "2" here and below and have it inferred. Might make the code less readable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's true. I kind of like having the explicit visual marker where I can check that the declared size matches the size I pass to the API function, but I don't think it matters too much.

em_promise_t assert_stack =
emscripten_promise_then(test6, check_stack, fail, NULL);
emscripten_promise_then(test7, check_stack, fail, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this kind of test that has many phases/sub-tests, would make sense written in gtest? I added one test recently that uses gtest, but perhaps it would be worth looking into using it more? Once downside is that its maybe a little heavy weight and relies heavily on C++ (so is not so good for testing low level C libraries perhaps?)>

Copy link
Member Author

Choose a reason for hiding this comment

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

gtest is great, but another problem for these tests in particular is that I don't think gtest would play well with the asynchrony happening here. The actual test checks for all of these tests aren't being run until well after main returns.

@tlively tlively merged commit 47cb9c6 into main Apr 12, 2023
@tlively tlively deleted the promise-race branch April 12, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants