Skip to content

Implement emscripten_promise_all_settled in promise.h #19152

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 5 commits into from
Apr 11, 2023
Merged

Conversation

tlively
Copy link
Member

@tlively tlively commented Apr 10, 2023

This is like emscripten_promise_all, but always fulfills and separately
reports whether each input promise is fulfilled or rejected.

This is like `emscripten_promise_all`, but always fulfills and separately
reports whether each input promise is fulfilled or rejected.
@tlively tlively requested review from kripken and sbc100 April 10, 2023 18:35
@tlively
Copy link
Member Author

tlively commented Apr 10, 2023

promise: Promise.allSettled(promises).then((results) => {
if (resultBuf) {
for (var i = 0; i < size; i++) {
let baseOffset = i * {{{ C_STRUCTS.em_settled_result_t.__size__ }}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we using let yet in our library code. Can you stick to var?

@tlively
Copy link
Member Author

tlively commented Apr 10, 2023

Any ideas for how to work around this closure error?

building:ERROR: Closure compiler completed with warnings and -Werror=closure enabled, aborting!

building:ERROR: /tmp/emscripten_temp_3rigp9cj/main.js:19315:16: WARNING - [JSC_TYPE_MISMATCH] assignment
found   : (*|undefined)
required: number
  19315|                 HEAPU32[(((resultBuf)+(valueOffset))>>2)] = results[i].reason;
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 error(s), 1 warning(s), 72.6% typed

@tlively
Copy link
Member Author

tlively commented Apr 10, 2023

I just decided to suppress it for now. LMK if there's a better solution.

@tlively
Copy link
Member Author

tlively commented Apr 10, 2023

Yikes, it looks like some build modes remove the newline after the suppression comment, causing a new error.

Closure compiler completed with warnings and -Werror=closure enabled, aborting!

building:ERROR: /tmp/emscripten_temp_sykc9y08/main.jso1.js:11734:36: WARNING - [JSC_MISPLACED_SUPPRESS] @suppress annotation not allowed here. See https://github.com/google/closure-compiler/wiki/@suppress-annotations
  11734|       /** @suppress {checkTypes} */ SAFE_HEAP_STORE((((resultBuf) + (valueOffset)) >> 2) * 4, results[i].reason, 4);
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 error(s), 1 warning(s), 68.6% typed

@sbc100, any ideas for workarounds?

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

TIL about allSettled...

@sbc100
Copy link
Collaborator

sbc100 commented Apr 11, 2023

Any ideas for how to work around this closure error?

building:ERROR: Closure compiler completed with warnings and -Werror=closure enabled, aborting!

building:ERROR: /tmp/emscripten_temp_3rigp9cj/main.js:19315:16: WARNING - [JSC_TYPE_MISMATCH] assignment
found   : (*|undefined)
required: number
  19315|                 HEAPU32[(((resultBuf)+(valueOffset))>>2)] = results[i].reason;
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 error(s), 1 warning(s), 72.6% typed

I've not seen that before from the makeSetValue macros. Its basically saying it can't determine that results[i].reason is actually a number. Perhaps that type of the objects in the results array needs a type annotation? Let me see..

{{{ makeSetValue('resultBuf', 'resultOffset', 'reject', 'i32') }}};
// Closure can't type `reason` in some contexts
/** @suppress {checkTypes} */
{{{ makeSetValue('resultBuf', 'valueOffset', 'results[i].reason', '*') }}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

So these promises only ever get rejected or fullfilled with Numbers? What happens if one of them is rejected to fulfilled with something that is not a number?

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, they're always fulfilled or rejected with pointers because the fulfill values and reject reasons are always represented as void* in the C API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can do something like this: var reason = /** @type {number} */ results[i].reason;

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a good way to generalize that for wasm64?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We use number for pointer in wasm64 and wasm32 so it should just work.

@tlively tlively merged commit dfbf6a8 into main Apr 11, 2023
@tlively tlively deleted the promise-all-settled branch April 11, 2023 14:52
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.

3 participants