Skip to content

Handle thread exit #1869

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.

Sign up for GitHub

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
wenyongh merged 1 commit into from
Jan 23, 2023
Merged

Handle thread exit #1869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion samples/wasi-threads/wasm-apps/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@ function (compile_sample SOURCE_FILE)
endfunction ()

compile_sample(no_pthread.c wasi_thread_start.S)
compile_sample(exception_propagation.c wasi_thread_start.S)
compile_sample(thread_termination.c wasi_thread_start.S)
39 changes: 32 additions & 7 deletions ...threads/wasm-apps/exception_propagation.c → ...si-threads/wasm-apps/thread_termination.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

#include "wasi_thread_start.h"

#define TEST_TERMINATION_BY_TRAP 0 // Otherwise test `proc_exit` termination
#define TEST_TERMINATION_IN_MAIN_THREAD 1

#define TIMEOUT_SECONDS 10
#define NUM_THREADS 3
static sem_t sem;
Expand All @@ -27,7 +30,7 @@ typedef struct {
void
run_long_task()
{
// Busy waiting to be interruptible by exception
// Busy waiting to be interruptible by trap or `proc_exit`
for (int i = 0; i < TIMEOUT_SECONDS; i++)
Copy link
Contributor Author

@eloparco eloparco Jan 11, 2023

Choose a reason for hiding this comment

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

@wenyongh btw I notice something while testing: if I replace this for loop with a __builtin_wasm_memory_atomic_wait32(0, 0, -1); or even while(1); the thread doesn't get interrupted even if the exception is propagated (in the first case it gets stuck here https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/shared/platform/common/posix/posix_thread.c#L192). Is that expected? I'll investigate to understand what's happening

Copy link
Contributor

@yamt yamt Jan 11, 2023

Choose a reason for hiding this comment

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

it's what i expect to happen with the current implementation.

to terminate busy looping threads, something like wasm_cluster_thread_send_signal would work.
i guess we eventually need to implement something (eg. send a signal on posix-like platforms) to terminate the blocking threads.

Copy link
Collaborator

@wenyongh wenyongh Jan 11, 2023

Choose a reason for hiding this comment

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

@wenyongh btw I notice something while testing: if I replace this for loop with a __builtin_wasm_memory_atomic_wait32(0, 0, -1); or even while(1); the thread doesn't get interrupted even if the exception is propagated (in the first case it gets stuck here https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/shared/platform/common/posix/posix_thread.c#L192). Is that expected? I'll investigate to understand what's happening

Normally runtime should terminate all threads when exception was thrown, I remember it is the expected behavior of Java VM. Seems it is an issue of the main branch, I try to fix it with the patch below for interpreter. And another issue of main branch may be that the main thread doesn't spread exception to other thread when exception was thrown, like the change of this PR in wasm_runtime_call_wasm. I will discuss with @xujuntwt95329 for how to fix them.

diff --git a/core/iwasm/interpreter/wasm_interp_fast.c b/core/iwasm/interpreter/wasm_interp_fast.c
index 3109b0c8..36863bf2 100644
--- a/core/iwasm/interpreter/wasm_interp_fast.c
+++ b/core/iwasm/interpreter/wasm_interp_fast.c
@@ -1054,6 +1054,8 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst,
 #if WASM_ENABLE_THREAD_MGR != 0
 #define CHECK_SUSPEND_FLAGS()                           \
     do {                                                \
+        if (wasm_get_exception(module))                 \
+            goto got_exception;                         \
         if (exec_env->suspend_flags.flags != 0) {       \
             if (exec_env->suspend_flags.flags & 0x01) { \

Copy link
Collaborator

@wenyongh wenyongh Jan 11, 2023

Choose a reason for hiding this comment

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

@eloparco I discussed the issues with @xujuntwt95329, he agreed that they are issues and will submit patch to fix them on main branch. Could you wait until we fix them and merge branch main into dev/wasi_threads, so as to sync up the code and avoid conflicts in the future?

Copy link
Contributor Author

@eloparco eloparco Jan 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

as proposed by @yamt, I tried adding a wasm_cluster_send_signal_all(cluster, WAMR_SIG_TERM); after this line that spreads the exception

wasm-micro-runtime/core/iwasm/libraries/thread-mgr/thread_manager.c

Line 995 in 903f521

traverse_list(&cluster->exec_env_list, set_exception_visitor, exec_env);
and it solves in case of a while(1); but not for other cases like sleep(TIMEOUT_SECONDS); or __builtin_wasm_memory_atomic_wait32(0, 0, -1);

Copy link
Collaborator

@wenyongh wenyongh Jan 20, 2023

Choose a reason for hiding this comment

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

@loganek Thanks for the review, I changed the wasm_runtime_deinstantiate_internal in thread_manager_start_routine. But for the wasm_cluster_add_exec_env, not sure why ret can be removed.
I pushed the PR, see: c714189

Copy link
Contributor

@loganek loganek Jan 20, 2023

Choose a reason for hiding this comment

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

But for the wasm_cluster_add_exec_env, not sure why ret can be removed.

It can be replaced with return statements (we no longer have a lock to unlock). As I said, it's not a big deal though. The rest looks ok for me.

Copy link
Contributor

@loganek loganek Jan 20, 2023

Choose a reason for hiding this comment

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

I agree with @yamt that we may need some more complex mechanism to terminate the threads when they are blocked in kernel.

@xujuntwt95329 / @wenyongh does anybody on your side actively working on that? If not, we'll take on that work.

Copy link
Collaborator

@wenyongh wenyongh Jan 20, 2023

Choose a reason for hiding this comment

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

@loganek not yet

Copy link
Contributor

@loganek loganek Jan 20, 2023

Choose a reason for hiding this comment

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

ok thanks, we'll look into that then.

sleep(1);
}
Expand All @@ -39,18 +42,22 @@ __wasi_thread_start_C(int thread_id, int *start_arg)

if (data->throw_exception) {
// Wait for all other threads (including main thread) to be ready
printf("Waiting before throwing exception\n");
printf("Waiting before terminating\n");
for (int i = 0; i < NUM_THREADS; i++)
sem_wait(&sem);

printf("Throwing exception\n");
printf("Force termination\n");
#if TEST_TERMINATION_BY_TRAP == 1
__builtin_trap();
#else
__wasi_proc_exit(1);
#endif
}
else {
printf("Thread running\n");

sem_post(&sem);
run_long_task(); // Wait to be interrupted by exception
run_long_task(); // Wait to be interrupted
assert(false && "Unreachable");
}
}
Expand All @@ -74,19 +81,26 @@ main(int argc, char **argv)
}
}

// Create a thread that throws an exception
// Create a thread that forces termination through trap or `proc_exit`
#if TEST_TERMINATION_IN_MAIN_THREAD == 1
data[0].throw_exception = false;
#else
data[0].throw_exception = true;
#endif
thread_id = __wasi_thread_spawn(&data[0]);
if (thread_id < 0) {
printf("Failed to create thread: %d\n", thread_id);
return EXIT_FAILURE;
}

// Create two additional threads to test exception propagation
data[1].throw_exception = false;
thread_id = __wasi_thread_spawn(&data[1]);
if (thread_id < 0) {
printf("Failed to create thread: %d\n", thread_id);
return EXIT_FAILURE;
}
data[2].throw_exception = false;
thread_id = __wasi_thread_spawn(&data[2]);
if (thread_id < 0) {
printf("Failed to create thread: %d\n", thread_id);
Expand All @@ -96,8 +110,19 @@ main(int argc, char **argv)
printf("Main thread running\n");

sem_post(&sem);
run_long_task(); // Wait to be interrupted by exception
assert(false && "Unreachable");

#if TEST_TERMINATION_IN_MAIN_THREAD == 1

printf("Force termination (main thread)\n");
#if TEST_TERMINATION_BY_TRAP == 1
__builtin_trap();
#else /* TEST_TERMINATION_BY_TRAP */
__wasi_proc_exit(1);
#endif /* TEST_TERMINATION_BY_TRAP */

#else /* TEST_TERMINATION_IN_MAIN_THREAD */
run_long_task(); // Wait to be interrupted
assert(false && "Unreachable");
#endif /* TEST_TERMINATION_IN_MAIN_THREAD */
return EXIT_SUCCESS;
}