[Proxying] Cancel proxied work when the worker thread dies by tlively · Pull Request #18741 · emscripten-core/emscripten

tlively · 2023-02-14T01:48:59Z

Update the synchronous proxying APIs to return 0 when attempting to proxy to a
thread that has already exited or has been canceled or exits or is canceled
before the proxied work is completed.

To implement cancellation, add an optional cancel function pointer to the
tasks queued in em_task_queues, which are the internal thread-specific queues
used in the implementation of em_proxying_queue. This cancel function
pointer is not controllable directly by users of the API, but is rather used
internally to ensure that synchronous waiters are notified when the thread they
are waiting on dies. This mechanism will scale to other forms of cancellation
notification in the future. For example, it could be used to reject a promise
associated with the proxied work.

When a thread dies, it runs the cancellation handlers for each task in its
mailbox. Those tasks each carry a pointer to some task queue owned by a proxying
queue, so their cancellation handlers in turn run the cancellation handlers for
all the tasks in those pointed-to queues.

Task queue cancellation handlers do not handle the case of proxied work that has
been started but not finished when the worker thread dies because that work has
already been removed from the task queues. To cancel in-progress work as well,
update the implementation of synchronous proxying to place em_proxying_ctx
objects associated with active work in a thread-local doubly linked list that is
traversed by a registered thread specific data destructor. The destructor takes
care of canceling the active work.

Implements the design sketched out in #18631.

tlively · 2023-02-14T01:49:14Z

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.

tlively · 2023-02-14T05:23:23Z

The failing wasm2js test should be fixed by WebAssembly/binaryen#5489.

sbc100

Do for each task we proxy we actually now create two tasks? (i.e. the user's work in the queue they specify, and then a special notification task which lives in the mailbox of the target thread?)

tlively · 2023-02-17T00:30:24Z

Do for each task we proxy we actually now create two tasks? (i.e. the user's work in the queue they specify, and then a special notification task which lives in the mailbox of the target thread?)

In the worst case, yes. However, if there is already a pending notification task in the mailbox of the target thread, then we don't create another one of those.

sbc100

I can't say I've read an understood the entire change, especially the different tests cases. But I think this is great direction and the test coverage looks thorough.

tlively · 2023-02-17T00:38:32Z

Is test_pthread_dlopen_many flaky before this change or because of this change?

sbc100 · 2023-02-17T00:38:53Z

I think the test failure on wasm64 is new flake that I recently added.

sbc100 · 2023-02-17T00:50:01Z

Is test_pthread_dlopen_many flaky before this change or because of this change?

yes, but I think/hope its only flaky on wasm64

tlively · 2023-02-17T03:26:10Z

That same test was failing asan tests with a detected memory leak earlier in this PR, so it's not just a wasm64 flake: https://app.circleci.com/pipelines/github/emscripten-core/emscripten/25923/workflows/3092d7a9-0f07-465c-8972-c2b364ac6d5d/jobs/604201

sbc100 · 2023-03-01T18:53:55Z

Is this change good to go now?

tlively · 2023-03-01T19:13:36Z

Yes. Since it got smaller since you last approved it, I was just going to land it as soon as the tests pass.

tlively · 2023-03-01T19:14:15Z

Is it safe to land despite the test_minimal_runtime_code_size_random_printf_wasm failure?

sbc100 · 2023-03-01T19:18:52Z

I just pushed a rebase of the expectations. If you rebase again you should be good.

Update the synchronous proxying APIs to return 0 when attempting to proxy to a thread that has already exited or has been canceled or exits or is canceled before the proxied work is completed. To implement cancellation, add an optional `cancel` function pointer to the tasks queued in `em_task_queue`s, which are the internal thread-specific queues used in the implementation of `em_proxying_queue`. This `cancel` function pointer is not controllable directly by users of the API, but is rather used internally to ensure that synchronous waiters are notified when the thread they are waiting on dies. This mechanism will scale to other forms of cancellation notification in the future. For example, it could be used to reject a promise associated with the proxied work. When a thread dies, it runs the cancellation handlers for each task in its mailbox. Those tasks each carry a pointer to some task queue owned by a proxying queue, so their cancellation handlers in turn run the cancellation handlers for all the tasks in those pointed-to queues. Task queue cancellation handlers do not handle the case of proxied work that has been started but not finished when the worker thread dies because that work has already been removed from the task queues. To cancel in-progress work as well, update the implementation of synchronous proxying to place `em_proxying_ctx` objects associated with active work in a thread-local doubly linked list that is traversed by a registered thread specific data destructor. The destructor takes care of canceling the active work. Implements the design sketched out in #18631.

…n-core#18741) Update the synchronous proxying APIs to return 0 when attempting to proxy to a thread that has already exited or has been canceled or exits or is canceled before the proxied work is completed. To implement cancellation, add an optional `cancel` function pointer to the tasks queued in `em_task_queue`s, which are the internal thread-specific queues used in the implementation of `em_proxying_queue`. This `cancel` function pointer is not controllable directly by users of the API, but is rather used internally to ensure that synchronous waiters are notified when the thread they are waiting on dies. This mechanism will scale to other forms of cancellation notification in the future. For example, it could be used to reject a promise associated with the proxied work. When a thread dies, it runs the cancellation handlers for each task in its mailbox. Those tasks each carry a pointer to some task queue owned by a proxying queue, so their cancellation handlers in turn run the cancellation handlers for all the tasks in those pointed-to queues. Task queue cancellation handlers do not handle the case of proxied work that has been started but not finished when the worker thread dies because that work has already been removed from the task queues. To cancel in-progress work as well, update the implementation of synchronous proxying to place `em_proxying_ctx` objects associated with active work in a thread-local doubly linked list that is traversed by a registered thread specific data destructor. The destructor takes care of canceling the active work. Implements the design sketched out in emscripten-core#18631.

tlively requested review from kripken and sbc100 February 14, 2023 01:49

tlively mentioned this pull request Feb 16, 2023

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

Merged

tlively force-pushed the proxying-shutdown branch from 741aafa to b9fb8c0 Compare February 16, 2023 22:26

sbc100 reviewed Feb 16, 2023

View reviewed changes

Comment thread system/lib/pthread/proxying.c

Comment thread test/pthread/test_pthread_proxying.c

Comment thread test/test_core.py

sbc100 approved these changes Feb 17, 2023

View reviewed changes

tlively force-pushed the proxying-shutdown branch from 54d2274 to d13a814 Compare February 17, 2023 06:02

This was referenced Feb 21, 2023

[Proxying] Add emscripten_proxy_callback_with_ctx #18810

Merged

[Proxying] Add proxying functions that return promises #18825

Merged

tlively changed the base branch from main to proxying-mailbox February 25, 2023 05:27

tlively force-pushed the proxying-shutdown branch from d13a814 to 1301f0e 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-mailbox branch from 579f55d to 73023ce Compare February 25, 2023 06:41

tlively force-pushed the proxying-shutdown branch from 1301f0e to c7918e5 Compare February 25, 2023 06:41

tlively force-pushed the proxying-mailbox branch from 73023ce to 6ba6e6d Compare February 25, 2023 06:43

tlively force-pushed the proxying-shutdown branch 2 times, most recently from 5223090 to e0abb19 Compare February 27, 2023 15:35

This was referenced Feb 27, 2023

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

Merged

Remove unnecessary EXIT_RUNTIME setting from proxying tests #18873

Merged

tlively force-pushed the proxying-mailbox branch from 2f288d3 to bc9b5ad Compare March 1, 2023 01:09

tlively force-pushed the proxying-shutdown branch from e0abb19 to 09a3df8 Compare March 1, 2023 01:09

Base automatically changed from proxying-mailbox to main March 1, 2023 16:30

tlively force-pushed the proxying-shutdown branch from 09a3df8 to 8b4eb90 Compare March 1, 2023 16:38

tlively force-pushed the proxying-shutdown branch from 8b4eb90 to ea71ab3 Compare March 1, 2023 19:37

tlively merged commit 168dafb into main Mar 1, 2023

tlively deleted the proxying-shutdown branch March 1, 2023 21:04

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tlively commented Feb 14, 2023

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tlively commented Feb 17, 2023

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

tlively commented Feb 17, 2023

Uh oh!

sbc100 commented Feb 17, 2023

Uh oh!

sbc100 commented Feb 17, 2023

Uh oh!

tlively commented Feb 17, 2023

Uh oh!

sbc100 commented Mar 1, 2023

Uh oh!

tlively commented Mar 1, 2023

Uh oh!

tlively commented Mar 1, 2023

Uh oh!

sbc100 commented Mar 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tlively commented Feb 14, 2023 •

edited

Loading

tlively commented Feb 14, 2023 •

edited

Loading