Implement async termination of blocking thread #2516

yamt · 2023-08-30T08:31:16Z

send a signal whose handler is no-op to a blocking thread to wake up the blocking syscall
with either EINTR equivalent or partial success.

unlike the approach taken in the dev/interrupt_block_insn branch (that is, signal + longjmp similarly to OS_ENABLE_HW_BOUND_CHECK),
this PR does not use longjmp because:

longjmp from signal handler doesn't work on nuttx (signal handler is called within "kernel" apache/nuttx#10326)
i'm afraid that the singal+longjmp approach is too difficult for average programmers who might implement host functions to deal with. especially when the longjmp can rewind a lot of functions, some of which are potentially user-supplied.

todo/known issues/limitations/open questions:

~~make the initialization independent from OS_ENABLE_HW_BOUND_CHECK~~
~~provide an api (or at least a build-time config) to specifiy a signal to use, instead of hardcoding SIGUSR1.~~
adapt operations other than fd_read
- ~~fd_close~~
- ~~fd_pread~~
- ~~fd_pwrite~~
- ~~fd_read~~
- ~~fd_write~~
- ~~path_open~~
  - i guess it can block on eg. fifo. i'm not sure if i want to fix it in this PR.
- poll_oneoff
  - leave this for now as it's already fixed with a different approach: modified poll_oneoff to make it interruptible #1951
- ~~sock_accept~~
- ~~sock_close (?)~~ just ENOSYS
- ~~sock_connect~~
- ~~sock_recv~~ just a wrapper of sock_recv_from
- ~~sock_recv_from~~
- ~~sock_send~~ just a wrapper of sock_send_to
- ~~sock_send_to~~
- ~~sock_shutdown~~ usually not block
- ~~sock_addr_resolve~~
  - this one is not a simple syscall. see the comment in blocking_op.c
- memory.atomic.wait32/64
  - leave this for now as it's already fixed with a different approach: Fix atomic.wait, get wasi_ctx exit code and thread mgr issues #2024
what to do for platforms other than posix?
what to do for other wasi implementation? (uvwasi)
~~document/comment~~
test
- the relevant tests in wasi-threads repo passed on macOS and nuttx/esp32-devkitc
~~add an api to terminate an instance~~ Add an API to terminate instance #2538
~~add a sample~~ Add an API to terminate instance #2538
for simple blocking system call like readv, it's easy to wrap it with wasm_runtime_begin_blocking_op/wasm_runtime_end_blocking_op. but how about something more complex, like a call which possibly blocks deep inside an external library?

relevant issue: #1910

yamt · 2023-08-31T23:02:42Z

the last push passes all test cases in wasi-threads repo on macOS.

yamt · 2023-09-13T12:43:55Z

the ci failure looks unrelated

  Cannot initiate the connection to ppa.launchpadcontent.net:443 (2620:2d:4000:1::3e). - connect (101: Network is unreachable) Could not connect to ppa.launchpadcontent.net:443 (185.125.190.52), connection timed out [IP: 185.125.190.52 443]

core/iwasm/include/wasm_export.h

core/shared/platform/common/posix/posix_blocking_op.c

wenyongh · 2023-09-18T04:01:43Z

@eloparco Could you help review the PR? It implements feature like dev/interrupt_block_insn in different manner.

basically a port from https://github.com/yamt/garbage/blob/master/c/sig2/test.c this ported version has not been tested yet though.

for now, only update fd_read. it should be easy to apply to other methods as the necessary change is mostly mechanical.

I was assuming pthread_sigmask was an alias of sigprocmask. It's true for platforms I'm familiar with. However, after reading the descriptions in the relevant standards, I changed my mind.

eloparco · 2023-09-19T09:55:14Z

@eloparco Could you help review the PR? It implements feature like dev/interrupt_block_insn in different manner.

Sure, I'll take a look in the next couple of days.

core/iwasm/common/wasm_blocking_op.c

wenyongh

LGTM

eloparco · 2023-09-19T23:59:02Z

Took a look. This doesn't cover user-defined functions that could have been achieved with longjmp I think. But, as you said, the longjmp approach is probably too complicated.

We should be able to add these tests now, probably in a separate PR.

yamt · 2023-09-20T00:25:50Z

Took a look. This doesn't cover user-defined functions that could have been achieved with longjmp I think. But, as you said, the longjmp approach is probably too complicated.

user-defined functions should take care of it by itself, either using the api introduced by this PR or other approaches like #1951. i guess in many cases it's simpler than preparing for signal+longjmp.

We should be able to add these tests now, probably in a separate PR.

yes. they all pass with this PR: #2516 (comment)

wenyongh · 2023-09-20T08:43:58Z

Took a look. This doesn't cover user-defined functions that could have been achieved with longjmp I think. But, as you said, the longjmp approach is probably too complicated.

We should be able to add these tests now, probably in a separate PR.

@eloparco so the PR is good to you, right? Shall we merge the PR?

eloparco · 2023-09-20T08:53:03Z

@eloparco so the PR is good to you, right? Shall we merge the PR?

LGTM

On posix-like platforms, the rest of wasi-threads tests should pass after the recent changes including the following PRs: bytecodealliance#2516 bytecodealliance#2524 bytecodealliance#2529

Implement async termination of blocking thread (bytecodealliance#2516)

On posix-like platforms, the rest of wasi-threads tests should pass after the recent changes including the following PRs: bytecodealliance#2516 bytecodealliance#2524 bytecodealliance#2529

On posix-like platforms, the rest of wasi-threads tests should pass after the recent changes including the following PRs: #2516, #2524, #2529, #2571, #2576 and #2582.

Send a signal whose handler is no-op to a blocking thread to wake up the blocking syscall with either EINTR equivalent or partial success. Unlike the approach taken in the `dev/interrupt_block_insn` branch (that is, signal + longjmp similarly to `OS_ENABLE_HW_BOUND_CHECK`), this PR does not use longjmp because: * longjmp from signal handler doesn't work on nuttx refer to apache/nuttx#10326 * the singal+longjmp approach may be too difficult for average programmers who might implement host functions to deal with See also bytecodealliance#1910

On posix-like platforms, the rest of wasi-threads tests should pass after the recent changes including the following PRs: bytecodealliance#2516, bytecodealliance#2524, bytecodealliance#2529, bytecodealliance#2571, bytecodealliance#2576 and bytecodealliance#2582.

yamt mentioned this pull request Aug 30, 2023

Implement os_usleep for posix #2517

Merged

yamt changed the title ~~Implement termination of blocking thread~~ Implement async termination of blocking thread Aug 30, 2023

yamt force-pushed the blocking-op branch 3 times, most recently from 38f769a to 455061a Compare August 31, 2023 22:59

yamt force-pushed the blocking-op branch 2 times, most recently from 70a66cb to 8b14c2f Compare September 4, 2023 05:30

yamt mentioned this pull request Sep 4, 2023

handle a return from wasi _start function correctly #2529

Merged

yamt force-pushed the blocking-op branch 4 times, most recently from 348a15d to 1b28087 Compare September 7, 2023 08:41

yamt mentioned this pull request Sep 8, 2023

Add an API to terminate instance #2538

Merged

yamt force-pushed the blocking-op branch 2 times, most recently from c3a7e6f to 4877ae8 Compare September 13, 2023 11:45

yamt marked this pull request as ready for review September 13, 2023 11:46

wenyongh reviewed Sep 18, 2023

View reviewed changes

core/iwasm/include/wasm_export.h Outdated Show resolved Hide resolved

core/iwasm/include/wasm_export.h Outdated Show resolved Hide resolved

core/iwasm/include/wasm_export.h Outdated Show resolved Hide resolved

core/shared/platform/common/posix/posix_blocking_op.c Show resolved Hide resolved

yamt added 10 commits September 19, 2023 11:37

some logic to terminate blocking op

53f1655

basically a port from https://github.com/yamt/garbage/blob/master/c/sig2/test.c this ported version has not been tested yet though.

avoid sleeping with a mutex held

5ea4c03

comment

6fbe85c

fix an inverted condition

63bdb21

wasm_export.h: add wasm_runtime_{begin|end}_blocking_op

1bfc737

make set_thread_cancel_flags interrupt blocking op

ec28fa2

wasi: mark blocking ops explicitly

acfd426

for now, only update fd_read. it should be easy to apply to other methods as the necessary change is mostly mechanical.

separate blocking op initialization from thread signal init

3114f90

nuttx: update the file list

0dbcee3

wasm_blocking_op.c: this stuff doesn't make sense w/o threads

f8c8070

yamt added 7 commits September 19, 2023 11:37

comment

a770e37

add os_set_signal_number_for_blocking_op

6643822

comment

33d23f0

posix_blocking_op.c: use pthread_sigmask instead of sigprocmask

143a7d6

I was assuming pthread_sigmask was an alias of sigprocmask. It's true for platforms I'm familiar with. However, after reading the descriptions in the relevant standards, I changed my mind.

add a comment about getaddrinfo

fd13caf

comment

5ac3cb5

comments

0daf3d7

yamt force-pushed the blocking-op branch from 8345b24 to 0daf3d7 Compare September 19, 2023 02:37

yamt added 3 commits September 19, 2023 11:38

comment

b5399cd

add a build-time config WAMR_DISABLE_WAKEUP_BLOCKING_OP

ecf798a

add messages

3be54a0

wenyongh reviewed Sep 19, 2023

View reviewed changes

core/iwasm/common/wasm_blocking_op.c Show resolved Hide resolved

add a missing unlock

778d2b2

wenyongh reviewed Sep 19, 2023

View reviewed changes

wenyongh merged commit 444b159 into bytecodealliance:main Sep 20, 2023
368 checks passed

yamt mentioned this pull request Sep 20, 2023

test_wamr.sh: bump wasi-testsuite version #2568

Merged

eloparco mentioned this pull request Sep 20, 2023

proc_exit and traps do not stop thread executing blocking instructions #1910

Closed

wenyongh added a commit to wenyongh/wasm-micro-runtime that referenced this pull request Sep 21, 2023

Merge pull request #815 from bytecodealliance/main

c4279bf

Implement async termination of blocking thread (bytecodealliance#2516)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement async termination of blocking thread #2516

Implement async termination of blocking thread #2516

yamt commented Aug 30, 2023 •

edited

Loading

yamt commented Aug 31, 2023

yamt commented Sep 13, 2023

wenyongh commented Sep 18, 2023

eloparco commented Sep 19, 2023

wenyongh left a comment

eloparco commented Sep 19, 2023

yamt commented Sep 20, 2023

wenyongh commented Sep 20, 2023

eloparco commented Sep 20, 2023

Implement async termination of blocking thread #2516

Implement async termination of blocking thread #2516

Conversation

yamt commented Aug 30, 2023 • edited Loading

yamt commented Aug 31, 2023

yamt commented Sep 13, 2023

wenyongh commented Sep 18, 2023

eloparco commented Sep 19, 2023

wenyongh left a comment

Choose a reason for hiding this comment

eloparco commented Sep 19, 2023

yamt commented Sep 20, 2023

wenyongh commented Sep 20, 2023

eloparco commented Sep 20, 2023

yamt commented Aug 30, 2023 •

edited

Loading