Skip to content

Add an API to terminate instance #2538

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 6 commits into from
Sep 13, 2023
Merged

Add an API to terminate instance #2538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fb063f6
wasm_runtime_set_exception: by stop distinguishing the calling instance
yamt Sep 8, 2023
a43afeb
add wasm_runtime_terminate
yamt Sep 8, 2023
4d41b26
iwasm: add --timeout option
yamt Sep 8, 2023
94e8ecc
comment
yamt Sep 8, 2023
c09464c
clear WASM_SUSPEND_FLAG_TERMINATE when clearing exception for now
yamt Sep 8, 2023
aff793c
iwasm: give a bit more details in the help message of --timeout
yamt Sep 11, 2023
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
27 changes: 24 additions & 3 deletions core/iwasm/common/wasm_runtime_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2342,8 +2342,8 @@ wasm_runtime_get_exec_env_singleton(WASMModuleInstanceCommon *module_inst_comm)
return module_inst->exec_env_singleton;
}

void
wasm_set_exception(WASMModuleInstance *module_inst, const char *exception)
static void
wasm_set_exception_local(WASMModuleInstance *module_inst, const char *exception)
{
exception_lock(module_inst);
if (exception) {
Expand All @@ -2354,13 +2354,22 @@ wasm_set_exception(WASMModuleInstance *module_inst, const char *exception)
module_inst->cur_exception[0] = '\0';
}
exception_unlock(module_inst);
}

void
wasm_set_exception(WASMModuleInstance *module_inst, const char *exception)
{
#if WASM_ENABLE_THREAD_MGR != 0
WASMExecEnv *exec_env =
wasm_clusters_search_exec_env((WASMModuleInstanceCommon *)module_inst);
if (exec_env) {
wasm_cluster_spread_exception(exec_env, exception);
wasm_cluster_set_exception(exec_env, exception);
}
else {
wasm_set_exception_local(module_inst, exception);
}
#else
wasm_set_exception_local(module_inst, exception);
#endif
}

Expand Down Expand Up @@ -2468,6 +2477,18 @@ wasm_runtime_clear_exception(WASMModuleInstanceCommon *module_inst_comm)
wasm_runtime_set_exception(module_inst_comm, NULL);
}

#if WASM_ENABLE_THREAD_MGR != 0
void
wasm_runtime_terminate(WASMModuleInstanceCommon *module_inst_comm)
{
WASMModuleInstance *module_inst = (WASMModuleInstance *)module_inst_comm;

bh_assert(module_inst_comm->module_type == Wasm_Module_Bytecode
|| module_inst_comm->module_type == Wasm_Module_AoT);
wasm_set_exception(module_inst, "terminated by user");
Copy link
Collaborator

@xujuntwt95329 xujuntwt95329 Sep 12, 2023

Choose a reason for hiding this comment

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

Seems our purpose is to terminate all threads, how about call wasm_cluster_terminate_all directly rather than setting an exception?

Copy link
Contributor Author

@yamt yamt Sep 12, 2023

Choose a reason for hiding this comment

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

wasm_cluster_terminate_all seems unused/untested and broken. (eg. cluster_list_lock vs cluster->lock locking order)
IMO it should be removed.

Copy link
Collaborator

@wenyongh wenyongh Sep 13, 2023

Choose a reason for hiding this comment

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

@yamt We are implementing the thread suspension/resume mechanism in PR #2320,
in which when a thread wants to suspend/terminate other threads, it will lock the whole thread list firstly (and also get the acknowledge of other threads, other threads will run into waiting status). Shall we change to call wasm_cluster_terminate_all after the PR is implemented and tested?

Copy link
Contributor Author

@yamt yamt Sep 13, 2023

Choose a reason for hiding this comment

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

@yamt We are implementing the thread suspension/resume mechanism in PR #2320, in which when a thread wants to suspend/terminate other threads, it will lock the whole thread list firstly (and also get the acknowledge of other threads, other threads will run into waiting status). Shall we change to call wasm_cluster_terminate_all after the PR is implemented and tested?

how it's implemented is not critical for me as far as it works.
if you want to replace it with something else later, it's fine.

however, i want to provide a way for the terminated thread (the embedder app calling one of our execute APIs) to notice the fact it was terminated by wasm_runtime_terminate.
a nice property of a trap here is that the existing embedders are likely checking it already.
i'm not sure how wasm_cluster_terminate_all can work in that regard.

also, the current wasm_cluster_terminate_all seems to join all the threads by itself.
i'm not sure how it's supposed to work for the usage like iwasm, where wasm module is executed by the main thread.

Copy link
Collaborator

@wenyongh wenyongh Sep 13, 2023

Choose a reason for hiding this comment

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

Yes, a trap may be a good option for the embedders to check, maybe we can add a flag to indicate whether to throw the exception or not in wasm_runtime_terminate and wasm_cluster_terminate_all.

The current wasm_cluster_terminate_all joins all the threads which may include the main thread that initializes the runtime (if embedder calls the API in a another thread created by himself), it means that wasm_cluster_terminate_all waits until the main thread exits (and runtime is destroyed). This is not what embedder expects, I think we can change to wait until the cluster is destroyed in the cluster set. A potential issue of this PR is that wasm_cluster_terminate_all doesn't wait until all threads end their jobs, I am not sure whether it will cause unexpected behavior, it should be better if wasm_cluster_terminate_all can figure it out.

}
#endif

void
wasm_runtime_set_custom_data_internal(
WASMModuleInstanceCommon *module_inst_comm, void *custom_data)
Expand Down
4 changes: 4 additions & 0 deletions core/iwasm/common/wasm_runtime_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,10 @@ wasm_runtime_get_exception(WASMModuleInstanceCommon *module);
WASM_RUNTIME_API_EXTERN void
wasm_runtime_clear_exception(WASMModuleInstanceCommon *module_inst);

/* See wasm_export.h for description */
WASM_RUNTIME_API_EXTERN void
wasm_runtime_terminate(WASMModuleInstanceCommon *module);

/* Internal API */
void
wasm_runtime_set_custom_data_internal(WASMModuleInstanceCommon *module_inst,
Expand Down
21 changes: 21 additions & 0 deletions core/iwasm/include/wasm_export.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,27 @@ wasm_runtime_set_exception(wasm_module_inst_t module_inst,
WASM_RUNTIME_API_EXTERN void
wasm_runtime_clear_exception(wasm_module_inst_t module_inst);

/**
* Terminate the WASM module instance.
*
* This function causes the module instance fail as if it raised a trap.
*
* This is intended to be used in situations like:
*
* - A thread is executing the WASM module instance
* (eg. it's in the middle of `wasm_application_execute_main`)
*
* - Another thread has a copy of `wasm_module_inst_t` of
* the module instance and wants to terminate it asynchronously.
*
* This function is provided only when WAMR is built with threading enabled.
* (`WASM_ENABLE_THREAD_MGR=1`)
*
* @param module_inst the WASM module instance
*/
WASM_RUNTIME_API_EXTERN void
wasm_runtime_terminate(wasm_module_inst_t module_inst);

/**
* Set custom data to WASM module instance.
* Note:
Expand Down
16 changes: 14 additions & 2 deletions core/iwasm/libraries/thread-mgr/thread_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1061,6 +1061,15 @@ set_thread_cancel_flags(WASMExecEnv *exec_env)
os_mutex_unlock(&exec_env->wait_lock);
}

static void
clear_thread_cancel_flags(WASMExecEnv *exec_env)
{
os_mutex_lock(&exec_env->wait_lock);
WASM_SUSPEND_FLAGS_FETCH_AND(exec_env->suspend_flags,
~WASM_SUSPEND_FLAG_TERMINATE);
os_mutex_unlock(&exec_env->wait_lock);
}

int32
wasm_cluster_cancel_thread(WASMExecEnv *exec_env)
{
Expand Down Expand Up @@ -1266,18 +1275,21 @@ set_exception_visitor(void *node, void *user_data)
if (data->exception != NULL) {
set_thread_cancel_flags(exec_env);
}
else {
clear_thread_cancel_flags(exec_env);
}
}
}

void
wasm_cluster_spread_exception(WASMExecEnv *exec_env, const char *exception)
wasm_cluster_set_exception(WASMExecEnv *exec_env, const char *exception)
{
const bool has_exception = exception != NULL;
WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env);
bh_assert(cluster);

struct spread_exception_data data;
data.skip = exec_env;
data.skip = NULL;
Copy link
Collaborator

@wenyongh wenyongh Sep 11, 2023

Choose a reason for hiding this comment

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

If data.skip is set to NULL, seems no need to check if (exec_env != data->skip) in set_exception_visitor as exec_env is always not NULL? And also no need to use spread_exception_data struct, how about just passing exception to user_data argument of set_exception_visitor?

Copy link
Contributor Author

@yamt yamt Sep 11, 2023

Choose a reason for hiding this comment

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

yes, it can be simplified if we want.

data.exception = exception;

os_mutex_lock(&cluster->lock);
Expand Down
2 changes: 1 addition & 1 deletion core/iwasm/libraries/thread-mgr/thread_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ WASMExecEnv *
wasm_clusters_search_exec_env(WASMModuleInstanceCommon *module_inst);

void
wasm_cluster_spread_exception(WASMExecEnv *exec_env, const char *exception);
wasm_cluster_set_exception(WASMExecEnv *exec_env, const char *exception);

WASMExecEnv *
wasm_cluster_spawn_exec_env(WASMExecEnv *exec_env);
Expand Down
72 changes: 72 additions & 0 deletions product-mini/platforms/posix/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ print_help()
#if WASM_ENABLE_LIB_PTHREAD != 0 || WASM_ENABLE_LIB_WASI_THREADS != 0
printf(" --max-threads=n Set maximum thread number per cluster, default is 4\n");
#endif
#if WASM_ENABLE_THREAD_MGR != 0
printf(" --timeout=ms Set the maximum execution time in ms.\n");
printf(" If it expires, the runtime aborts the execution\n");
printf(" with a trap.\n");
#endif
#if WASM_ENABLE_DEBUG_INTERP != 0
printf(" -g=ip:port Set the debug sever address, default is debug disabled\n");
printf(" if port is 0, then a random port will be used\n");
Expand Down Expand Up @@ -488,6 +493,37 @@ dump_pgo_prof_data(wasm_module_inst_t module_inst, const char *path)
}
#endif

#if WASM_ENABLE_THREAD_MGR != 0
struct timeout_arg {
uint32 timeout_ms;
wasm_module_inst_t inst;
_Atomic bool cancel;
};

void *
timeout_thread(void *vp)
{
const struct timeout_arg *arg = vp;
uint32 left = arg->timeout_ms;
while (!arg->cancel) {
uint32 ms;
if (left >= 100) {
ms = 100;
}
else {
ms = left;
}
os_usleep((uint64)ms * 1000);
left -= ms;
if (left == 0) {
wasm_runtime_terminate(arg->inst);
break;
}
}
return NULL;
}
#endif

int
main(int argc, char *argv[])
{
Expand Down Expand Up @@ -546,6 +582,9 @@ main(int argc, char *argv[])
#if WASM_ENABLE_STATIC_PGO != 0
const char *gen_prof_file = NULL;
#endif
#if WASM_ENABLE_THREAD_MGR != 0
int timeout_ms = -1;
#endif

/* Process options. */
for (argc--, argv++; argc > 0 && argv[0][0] == '-'; argc--, argv++) {
Expand Down Expand Up @@ -742,6 +781,13 @@ main(int argc, char *argv[])
wasm_runtime_set_max_thread_num(atoi(argv[0] + 14));
}
#endif
#if WASM_ENABLE_THREAD_MGR != 0
else if (!strncmp(argv[0], "--timeout=", 10)) {
if (argv[0][10] == '\0')
return print_help();
timeout_ms = atoi(argv[0] + 10);
}
#endif
#if WASM_ENABLE_DEBUG_INTERP != 0
else if (!strncmp(argv[0], "-g=", 3)) {
char *port_str = strchr(argv[0] + 3, ':');
Expand Down Expand Up @@ -902,6 +948,22 @@ main(int argc, char *argv[])
}
#endif

#if WASM_ENABLE_THREAD_MGR != 0
struct timeout_arg timeout_arg;
korp_tid timeout_tid;
if (timeout_ms >= 0) {
timeout_arg.timeout_ms = timeout_ms;
timeout_arg.inst = wasm_module_inst;
timeout_arg.cancel = false;
ret = os_thread_create(&timeout_tid, timeout_thread, &timeout_arg,
APP_THREAD_STACK_SIZE_DEFAULT);
if (ret != 0) {
printf("Failed to start timeout\n");
goto fail5;
}
}
#endif

ret = 0;
if (is_repl_mode) {
app_instance_repl(wasm_module_inst);
Expand Down Expand Up @@ -932,6 +994,16 @@ main(int argc, char *argv[])
dump_pgo_prof_data(wasm_module_inst, gen_prof_file);
#endif

#if WASM_ENABLE_THREAD_MGR != 0
if (timeout_ms >= 0) {
timeout_arg.cancel = true;
os_thread_join(timeout_tid, NULL);
}
#endif

#if WASM_ENABLE_THREAD_MGR != 0
fail5:
#endif
#if WASM_ENABLE_DEBUG_INTERP != 0
fail4:
#endif
Expand Down