Skip to content
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

handle a return from wasi _start function correctly #2529

Merged
merged 2 commits into from
Sep 8, 2023
Merged

handle a return from wasi _start function correctly #2529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
935f055
handle a return from wasi _start function correctly
yamt Aug 31, 2023
cd56892
add comments
yamt Sep 7, 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
29 changes: 28 additions & 1 deletion core/iwasm/common/wasm_application.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,34 @@ execute_main(WASMModuleInstanceCommon *module_inst, int32 argc, char *argv[])
the actual main function. Directly calling main function
may cause exception thrown. */
if ((func = wasm_runtime_lookup_wasi_start_function(module_inst))) {
return wasm_runtime_call_wasm(exec_env, func, 0, NULL);
const char *wasi_proc_exit_exception = "wasi proc exit";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it better if we use a global variable or macro to represent the string "wasi proc exit"？ It is a very import constant string now。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ideally we should stop using strings i guess.


ret = wasm_runtime_call_wasm(exec_env, func, 0, NULL);
#if WASM_ENABLE_THREAD_MGR != 0
if (ret) {
/* On a successful return from the `_start` function,
we terminate other threads by mimicing wasi:proc_exit(0).

Note:
- A return from the `main` function is an equivalent of
exit(). (C standard)
- When exit code is 0, wasi-libc's `_start` function just
returns w/o calling `proc_exit`.
- A process termination should terminate threads in
the process. */

wasm_runtime_set_exception(module_inst, wasi_proc_exit_exception);
/* exit_code is zero-initialized */
ret = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why need to set wasi_proc_exit_exception here? If ret is true, there should be no exception thrown and wasi_exit_code was already initialized as 0, seems we can do nothing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to propagate it to other threads.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the main thread will wait until all other threads exit in wasm_exec_env_destroy, it doesn't meet the "wasi proc exit" exception, seems no need to propagate "wasi proc exit" for other threads to exit early? Or other threads may not finish their jobs normally, also other threads may fail to throw exception if they continue to run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. a return from _start should behave as exit(0) because it's what wasi-libc expects.
  2. exit(0) should terminate other threads.

wasm_runtime_set_exception here is just a convenient way to terminate other threads.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I my concern is, suppose the sub thread is doing an important job, e.g. do some calculation, output important data to a file, can we terminate it from the main thread? Is it better to wait until the sub threads exit? In fact, there is API wasm_cluster_wait_for_all_except_self in thread manager.

Not sure whether there is reference about "a return from _start should behave as exit(0)" in wasi-libc?

@loganek What's your opinion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm not sure if i understand your concern.
the importance of the job is something apps should consider. not runtime.

Not sure whether there is reference about "a return from _start should behave as exit(0)" in wasi-libc?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very sure whether it is a good behavior that the main thread terminates all others thread instead of waiting for them. But if you want to do that, can we just call the API wasm_cluster_terminate_all_except_self of thread manager:

#if WASM_ENABLE_THREAD_MGR != 0
    if (ret) {
        WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env);
        wasm_cluster_terminate_all_except_self(cluster, exec_env);
    }
#endif

It also sets the terminate flag for other threads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i chose to use wasm_runtime_set_exception because:

  • what i want to do here is to mimic wasi proc_exit, which uses wasm_runtime_set_exception.
  • wasm_cluster_terminate_all_except_self seems unused (thus untested) and broken. (cluster->lock vs cluster_list_lock locking order issue.)
  • we don't need to join threads here

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, if to use wasm_runtime_set_exception, could you please add comments to mention somewhat like we want to mimic wasi proc_exit and to terminate other threads? It is not very easy to understand the code :)
And could you wrap the code with #if WASM_ENABLE_THREAD_MGR != 0 .. #endif, it is only for multi-threading?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, if to use wasm_runtime_set_exception, could you please add comments to mention somewhat like we want to mimic wasi proc_exit and to terminate other threads? It is not very easy to understand the code :) And could you wrap the code with #if WASM_ENABLE_THREAD_MGR != 0 .. #endif, it is only for multi-threading?

i prefer less #ifdef in general. but ok. done.

}
#endif
/* report wasm proc exit as a success */
WASMModuleInstance *inst = (WASMModuleInstance *)module_inst;
if (!ret && strstr(inst->cur_exception, wasi_proc_exit_exception)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems both L117 and L115 make if in L121 always true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's false if (ret == false and cur_exception == some other exception).

inst->cur_exception[0] = 0;
ret = true;
}
return ret;
}
#endif /* end of WASM_ENABLE_LIBC_WASI */

Expand Down
42 changes: 3 additions & 39 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 @@ -1938,33 +1938,6 @@ wasm_runtime_finalize_call_function(WASMExecEnv *exec_env,
}
#endif

static bool
clear_wasi_proc_exit_exception(WASMModuleInstanceCommon *module_inst_comm)
{
#if WASM_ENABLE_LIBC_WASI != 0
bool has_exception;
char exception[EXCEPTION_BUF_LEN];
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);

has_exception = wasm_copy_exception(module_inst, exception);
if (has_exception && !strcmp(exception, "Exception: wasi proc exit")) {
/* The "wasi proc exit" exception is thrown by native lib to
let wasm app exit, which is a normal behavior, we clear
the exception here. And just clear the exception of current
thread, don't call `wasm_set_exception(module_inst, NULL)`
which will clear the exception of all threads. */
module_inst->cur_exception[0] = '\0';
return true;
}
return false;
#else
return false;
#endif
}

bool
wasm_runtime_call_wasm(WASMExecEnv *exec_env,
WASMFunctionInstanceCommon *function, uint32 argc,
Expand Down Expand Up @@ -2005,15 +1978,10 @@ wasm_runtime_call_wasm(WASMExecEnv *exec_env,
param_argc, new_argv);
#endif
if (!ret) {
if (clear_wasi_proc_exit_exception(exec_env->module_inst)) {
ret = true;
}
else {
if (new_argv != argv) {
wasm_runtime_free(new_argv);
}
return false;
if (new_argv != argv) {
wasm_runtime_free(new_argv);
Copy link
Contributor

Choose a reason for hiding this comment

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

clear_wasi_proc_exit_exception is only applied after calling wasi _start function now, is it a general handling of wasi? Will developer call other functions in wasi mode and then wasm_proc_exit occurs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i guess it's a gray area.
proc_exit is supposed to terminate the "process".
the concept of "process" only makes sense in the context of a wasi command.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable, maybe we can apply the changes first, and modify if needed in the future.

}
return false;
}

#if WASM_ENABLE_REF_TYPES != 0
Expand Down Expand Up @@ -4594,10 +4562,6 @@ wasm_runtime_call_indirect(WASMExecEnv *exec_env, uint32 element_index,
ret = aot_call_indirect(exec_env, 0, element_index, argc, argv);
#endif

if (!ret && clear_wasi_proc_exit_exception(exec_env->module_inst)) {
ret = true;
}

return ret;
}

Expand Down