[Proxying] Add proxying functions that return promises by tlively · Pull Request #18825 · emscripten-core/emscripten

tlively · 2023-02-23T02:31:49Z

Add emscripten_proxy_promise and emscripten_proxy_promise_with_ctx that
return em_promise_t handles to promises that are fulfilled when the proxied
work is completed or rejected when the target thread dies before it can complete
the work.

Do not document these new APIs yet since they depend on the promise API in
emscripten/promise.h, which is not yet documented. Similarly, do not add a C++
wrapper yet because emscripten/promise.h does not yet have a C++ API.

tlively · 2023-02-23T02:32:02Z

Current dependencies on/for this PR:

main
- PR [Proxying][NFC] Move deferred cleanup logic to em_task_queue #18851
  - PR [Proxying] Send messages via in-memory mailbox queues #18852
    - PR [Proxying] Cancel proxied work when the worker thread dies #18741
      - PR [Proxying] Update proxying with callback to support cancellation #18776
        
        PR [Proxying] Add emscripten_proxy_callback_with_ctx #18810
        
        PR [Proxying] Add proxying functions that return promises #18825 👈
        
        PR [Proxying] Use Atomics.waitAsync to receive proxying notifications #18861
        
        PR Remove unnecessary EXIT_RUNTIME setting from proxying tests #18873

This comment was auto-generated by Graphite.

sbc100

Looks like this needs a rebase?

tlively · 2023-03-02T16:32:33Z

@sbc100 this is ready for review.

sbc100 · 2023-03-03T18:48:32Z

+    q, target_thread, func, arg, promise, &block->ctx, &block->promise_ctx);
+}
+
+__attribute__((warn_unused_result)) em_promise_t


I don't think you need to (or should) duplicate these attributes in the .c file.

sbc100 · 2023-03-03T18:57:17Z

+    promise_ctx promise_ctx;
+    task task;
+  };
+  struct block* block = malloc(sizeof(*block));


Where are these "blocks" freed? I don't see the block pointer escaping this function. Does it rely on ctx being free'd?

Yes, it relies on ctx being freed, which happens in all the places free_ctx is called.

Perhaps add a comment on the first eleemnt of the struct? e.g. // This struct is free'd via the ctx pointer so this must be the first element

sbc100 · 2023-03-03T19:03:23Z

+
+em_promise_result_t exit_thread(void** result, void* data, void* value) {
+  // TODO: exit the thread directly rather than on the next turn of the event
+  // loop.


Why is this a TODO?

Could we make this automatic (one day) and have the outstanding promises keep the thread alive and have the resolution of the final outstanding promise take the thread down?

Trying to exit directly with pthread_exit does not work because that ends up throwing an exception to try to unwind, which in turn ends up rejecting the promise rather than gracefully exiting the thread.

Outstanding promises (at least those created with emscripten_promise_then) do keep the thread alive until their callbacks are executed, so this almost works. The only thing missing is that we don't use callUserCallback in the promise callbacks, so nothing actually checks the runtimeKeepAlive and exits. I'll fix that in a separate PR.

Add `emscripten_proxy_promise` and `emscripten_proxy_promise_with_ctx` that return `em_promise_t` handles to promises that are fulfilled when the proxied work is completed or rejected when the target thread dies before it can complete the work. Do not document these new APIs yet since they depend on the promise API in emscripten/promise.h, which is not yet documented. Similarly, do not add a C++ wrapper yet because emscripten/promise.h does not yet have a C++ API.

…re#18825) Add `emscripten_proxy_promise` and `emscripten_proxy_promise_with_ctx` that return `em_promise_t` handles to promises that are fulfilled when the proxied work is completed or rejected when the target thread dies before it can complete the work. Do not document these new APIs yet since they depend on the promise API in emscripten/promise.h, which is not yet documented. Similarly, do not add a C++ wrapper yet because emscripten/promise.h does not yet have a C++ API.

tlively requested a review from sbc100 February 23, 2023 02:31

This was referenced Feb 23, 2023

[Proxying] Add emscripten_proxy_callback_with_ctx #18810

Merged

[Proxying] Update proxying with callback to support cancellation #18776

Merged

[Proxying] Cancel proxied work when the worker thread dies #18741

Merged

tlively force-pushed the proxying-callback-with-ctx branch from 1fe81f5 to 4baabe9 Compare February 25, 2023 05:27

This was referenced Feb 25, 2023

[Proxying] Send messages via in-memory mailbox queues #18852

Merged

[Proxying][NFC] Move deferred cleanup logic to em_task_queue #18851

Merged

tlively force-pushed the proxying-promise branch from d34407a to ef868fb Compare February 25, 2023 05:27

tlively force-pushed the proxying-callback-with-ctx branch from 4baabe9 to 6a5e4e7 Compare February 25, 2023 06:41

tlively force-pushed the proxying-promise branch from ef868fb to 7fe8414 Compare February 25, 2023 06:41

tlively force-pushed the proxying-callback-with-ctx branch from 6a5e4e7 to 5cb7113 Compare February 25, 2023 06:44

tlively force-pushed the proxying-promise branch from 7fe8414 to 961da1e Compare February 25, 2023 06:44

tlively force-pushed the proxying-callback-with-ctx branch from 5cb7113 to da1f95e Compare February 27, 2023 15:35

tlively force-pushed the proxying-promise branch from 961da1e to 37d81c9 Compare February 27, 2023 15:35

tlively mentioned this pull request Feb 27, 2023

[Proxying] Use Atomics.waitAsync to receive proxying notifications #18861

Merged

tlively force-pushed the proxying-callback-with-ctx branch from da1f95e to 289203e Compare February 28, 2023 15:39

tlively force-pushed the proxying-promise branch from 37d81c9 to 5573a31 Compare February 28, 2023 15:39

tlively mentioned this pull request Feb 28, 2023

Remove unnecessary EXIT_RUNTIME setting from proxying tests #18873

Merged

tlively force-pushed the proxying-callback-with-ctx branch from 289203e to 2ef3195 Compare March 1, 2023 01:10

tlively force-pushed the proxying-promise branch from 5573a31 to a2d0ef3 Compare March 1, 2023 01:10

tlively force-pushed the proxying-callback-with-ctx branch from 2ef3195 to 4ccf2ee Compare March 1, 2023 16:38

tlively force-pushed the proxying-promise branch from a2d0ef3 to 0fa4fc7 Compare March 1, 2023 16:38

tlively force-pushed the proxying-callback-with-ctx branch from 4ccf2ee to 1f0ecb4 Compare March 1, 2023 19:37

tlively force-pushed the proxying-promise branch from 0fa4fc7 to a8184bf Compare March 1, 2023 19:37

tlively force-pushed the proxying-callback-with-ctx branch from 1f0ecb4 to af490a7 Compare March 1, 2023 21:07

tlively force-pushed the proxying-promise branch from a8184bf to c3b5047 Compare March 1, 2023 21:07

tlively force-pushed the proxying-callback-with-ctx branch from af490a7 to 5415a72 Compare March 1, 2023 22:33

tlively force-pushed the proxying-promise branch from c3b5047 to c30ca68 Compare March 1, 2023 22:33

Base automatically changed from proxying-callback-with-ctx to main March 2, 2023 01:36

sbc100 reviewed Mar 2, 2023

View reviewed changes

Comment thread system/include/emscripten/proxying.h

tlively force-pushed the proxying-promise branch from c30ca68 to 2f1ddd5 Compare March 2, 2023 02:21

sbc100 reviewed Mar 3, 2023

View reviewed changes

Comment thread test/pthread/test_pthread_proxying.c

sbc100 reviewed Mar 3, 2023

View reviewed changes

Comment thread test/pthread/test_pthread_proxying.c

sbc100 reviewed Mar 3, 2023

View reviewed changes

Comment thread test/pthread/test_pthread_proxying_canceled_work.c

sbc100 approved these changes Mar 3, 2023

View reviewed changes

tlively enabled auto-merge (squash) March 4, 2023 00:27

tlively added 2 commits March 6, 2023 08:37

address comments

c3c38e7

tlively force-pushed the proxying-promise branch from 0bc125b to c3c38e7 Compare March 6, 2023 16:46

tlively merged commit 5a8a8dd into main Mar 6, 2023

tlively deleted the proxying-promise branch March 6, 2023 18:09

Navigation Menu

Uh oh!

[Proxying] Add proxying functions that return promises - #18825

[Proxying] Add proxying functions that return promises#18825
tlively merged 2 commits into
mainfrom
proxying-promise

tlively commented Feb 23, 2023

Uh oh!

tlively commented Feb 23, 2023 •

edited

Loading

Uh oh!

sbc100 left a comment

Uh oh!

Uh oh!

tlively commented Mar 2, 2023

Uh oh!

sbc100 Mar 3, 2023

Uh oh!

sbc100 Mar 3, 2023

Uh oh!

tlively Mar 3, 2023

Uh oh!

sbc100 Mar 3, 2023

Uh oh!

Uh oh!

Uh oh!

sbc100 Mar 3, 2023

Uh oh!

tlively Mar 4, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tlively commented Feb 23, 2023

Uh oh!

tlively commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tlively commented Mar 2, 2023

Uh oh!

sbc100 Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

tlively Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbc100 Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

tlively Mar 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tlively commented Feb 23, 2023 •

edited

Loading