Add an API to terminate instance #2538
Conversation
this allows it to be called by other threads.
IMO, it's evil to clear exceptions. However, with the current API, it's necessary for repl at least.
|
|
||
| struct spread_exception_data data; | ||
| data.skip = exec_env; | ||
| data.skip = NULL; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yes, it can be simplified if we want.
|
|
||
| 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"); |
There was a problem hiding this comment.
Seems our purpose is to terminate all threads, how about call wasm_cluster_terminate_all directly rather than setting an exception?
There was a problem hiding this comment.
wasm_cluster_terminate_all seems unused/untested and broken. (eg. cluster_list_lock vs cluster->lock locking order)
IMO it should be removed.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
Add API wasm_runtime_terminate to terminate a module instance by setting "terminated by user" exception to the module instance. And update the product-mini of posix platforms. Note: this doesn't work for some situations like blocking system calls.
3 participants
Note: this doesn't work for some situations like blocking system calls.
Most of them will be fixed (at least for posix-like platforms) by #2516