Some wasm-wasi-component related fixes #1181
Conversation
|
I'm still wrapping my head around the bindings (and nxt_application.c), but I'm not fully certain of the approach to exiting. For one, it seems like we should call exit from outside of the closure passed to handle_result. I think we might short-circuit handle_result's error handling otherwise. Second, if we're directly exiting, the start function signature should change to return nothing instead of an nxt_int_t... but I also haven't fully convinced myself that calling exit directly from our application code is the right thing here. |
|
Rebase with master |
I've moved the exit(2) to outside the closure (assuming I know what
That function has a signature of typedef nxt_int_t (*nxt_process_start_t)(nxt_task_t *task,
nxt_process_data_t *data);We can signal errors in this function (i.e return NXT_ERROR) before we nxt_unit_run(unit_ctx);which is the main event loop. So it's right and I guess it was generated from bindgen or something.
That's how the module API works. In C we basically do the following ...
nxt_unit_run(unit_ctx);
nxt_unit_done(unit_ctx);
/* do any clean up here */
exit(EXIT_SUCCESS);We sit in nxt_unit_run() until we're told otherwise, e.g if we're told If we don't exit(2) Unit doesn't know we're properly terminated and we |
|
Moved the exit(2) outside the closure |
|
It might be easier to talk through this live -- let me know if you want to hop on Zoom -- but I'll take a crack at this here. Calling One consequence of exit is that the function never returns. Which means our signature of Lastly, instead of unconditionally exiting with |
Well I don't know about this specific case, in the wasm module my start static nxt_int_t
nxt_wasm_start(nxt_task_t *task, nxt_process_data_t *data)
{
nxt_int_t ret;
nxt_unit_ctx_t *unit_ctx;
nxt_unit_init_t wasm_init;
nxt_common_app_conf_t *conf;
conf = data->app;
ret = nxt_unit_default_init(task, &wasm_init, conf);
if (nxt_slow_path(ret != NXT_OK)) {
nxt_alert(task, "nxt_unit_default_init() failed");
return ret;
}
wasm_init.callbacks.request_handler = nxt_wasm_request_handler;
unit_ctx = nxt_unit_init(&wasm_init);
if (nxt_slow_path(unit_ctx == NULL)) {
return NXT_ERROR;
}
NXT_WASM_DO_HOOK(NXT_WASM_FH_MODULE_INIT);
nxt_unit_run(unit_ctx);
nxt_unit_done(unit_ctx);
NXT_WASM_DO_HOOK(NXT_WASM_FH_MODULE_END);
if (nxt_wasm_ctx.dirs != NULL) {
char **p;
for (p = nxt_wasm_ctx.dirs; *p != NULL; p++) {
nxt_free(*p);
}
nxt_free(nxt_wasm_ctx.dirs);
}
nxt_wops->destroy(&nxt_wasm_ctx);
exit(EXIT_SUCCESS);
}The only actually required bit there after the Maybe the rust code could do something similar or maybe it does its
As I've mentioned, it isn't incorrect. That is the function signature
I'm not sure about that, if anything you might use the result from Application process termination is handled by From that I don't see that the exit(3) status actually makes much For example here's how the PHP language module's start function ends static nxt_int_t
nxt_php_start(nxt_task_t *task, nxt_process_data_t *data)
{
...
nxt_unit_run(nxt_php_unit_ctx);
nxt_unit_done(nxt_php_unit_ctx);
exit(0);
return NXT_OK;
}Yes, pointless return... |
|
Use the return value of nxt_unit_run() as the exit(3) status |
|
Okay, so we can early return an nxt_int_t, but it's otherwise fine to run to completion and exit? Before this patch, After this patch, With your first version of this patch, What's the intended behavior? (Edit: Updated to reflect the revision you just pushed) |
Yes, once we return from
Which is precisely where the issue lies, these processes were not OTOH if you
Yes. we've been told to terminate, there's nothing expecting a return If all goes well, this function doesn't return...
Maybe that's my lack of rust knowledge, but I don't think that's true.
To exit(3) the application process when we're told. |
Oh, right, with the That hasn't changed, but if we get as far as calling |
|
Bingo :) (I missed the bail!()'s on my first read, too) |
|
Fix a build issue on arm64. nxt_unit_run() returns an int not an |
|
This looks different. Is it part of the patch or is it resulting from another commit? |
Yes, this is from other recent work. You can view the verbose make output with (or by passing |
|
Do we still want to call |
|
...or even |
|
It should be returning -1 on error. I don't know what If I |
|
I can tell I'm really selling you on Rust here ;) Looking just at the call to But we're not capturing that value at all. Which means even if we hit a I think we want would be closer to: diff --git a/src/wasm-wasi-component/src/lib.rs b/src/wasm-wasi-component/src/lib.rs
index 3ee40c4f..1b5171a2 100644
--- a/src/wasm-wasi-component/src/lib.rs
+++ b/src/wasm-wasi-component/src/lib.rs
@@ -101,7 +101,9 @@ unsafe extern "C" fn start(
task: *mut bindings::nxt_task_t,
data: *mut bindings::nxt_process_data_t,
) -> bindings::nxt_int_t {
- handle_result(task, || {
+ let mut rc: i32 = 0;
+
+ let result = handle_result(task, || {
let config = GLOBAL_CONFIG.get().unwrap();
let state = GlobalState::new(&config)
.context("failed to create initial state")?;
@@ -123,11 +125,19 @@ unsafe extern "C" fn start(
bail!("nxt_unit_init() failed");
}
- bindings::nxt_unit_run(unit_ctx);
+ rc = bindings::nxt_unit_run(unit_ctx);
bindings::nxt_unit_done(unit_ctx);
Ok(())
- })
+ });
+
+ if (result != bindings::NXT_OK as bindings::nxt_int_t) {
+ return result;
+ } else if (rc != 0) {
+ return rc;
+ }
+
+ exit(0);
}
unsafe fn handle_result(...but this could be totally wrong in several ways (haven't actually checked to see if it'd compile, and it's ugly enough that there must be a better way to do it. Lastly, it's not clear if we should return or exit with the value of nxt_unit_run when it fails.) |
AFAIK and looking at other modules once you've called nxt_unit_run() the Side note: You can call nxt_unit_run() from multiple threads, in which void *(*start_routine)(void *);and you do the exit(3) from the main thread. After some testing... If I simply As opposed to if I call exit(3) Testing your patch by forcing a bail!() in the closure, we end up in an So it seems to me
|
|
Okay, so it sounds like smeantically we want something more like if (result != bindings::NXT_OK as bindings::nxt_int_t) {
return result;
}
exit(rc)But that our handling of |
|
Unfortunately that still results in coredump city... Just for clarity this is what I tested diff --git a/src/wasm-wasi-component/src/lib.rs b/src/wasm-wasi-component/src/lib.rs
index ed35024a..0df88ff5 100644
--- a/src/wasm-wasi-component/src/lib.rs
+++ b/src/wasm-wasi-component/src/lib.rs
@@ -104,7 +104,7 @@ unsafe extern "C" fn start(
) -> bindings::nxt_int_t {
let mut rc: i32 = 0;
- handle_result(task, || {
+ let result = handle_result(task, || {
let config = GLOBAL_CONFIG.get().unwrap();
let state = GlobalState::new(&config)
.context("failed to create initial state")?;
@@ -122,6 +122,7 @@ unsafe extern "C" fn start(
wasm_init.callbacks.request_handler = Some(request_handler);
let unit_ctx = bindings::nxt_unit_init(&mut wasm_init);
+ bail!("nxt_unit_init() failed");
if unit_ctx.is_null() {
bail!("nxt_unit_init() failed");
}
@@ -132,6 +133,12 @@ unsafe extern "C" fn start(
Ok(())
});
+ if result != bindings::NXT_OK as bindings::nxt_int_t {
+ println!("Bailed returning ({})", result);
+ return result;
+ }
+
+ println!("Calling exit()");
exit(rc);
}
Resulting in the following on the console and a bunch of coredumps... and to clarify, it's the application process To double check I tried doing something similar in the original wasm diff --git a/src/wasm/nxt_wasm.c b/src/wasm/nxt_wasm.c
index 92ed57ab..6dce70ec 100644
--- a/src/wasm/nxt_wasm.c
+++ b/src/wasm/nxt_wasm.c
@@ -213,6 +213,9 @@ nxt_wasm_start(nxt_task_t *task, nxt_process_data_t *data)
wasm_init.callbacks.request_handler = nxt_wasm_request_handler;
+ printf("%s: Bailing out...\n", __func__);
+ return NXT_ERROR;
+
unit_ctx = nxt_unit_init(&wasm_init);
if (nxt_slow_path(unit_ctx == NULL)) {
return NXT_ERROR;Everything works as expected. |
|
Do you get coredump city with current master, with no changes other than forcing the bail? |
|
Yes I do... |
|
Okay, so that part's never worked. Merging this PR is strictly better than the status quo (fixes restarts), but failure cases are still broken. Do you want to tackle those in this PR, or in a followup? |
|
I just realised I did the So I think the bails are OK as long as you don't do it after a I will cook this up as a separate patch with you as the author as this (The application process probably shouldn't be core dumping, but that's This is the patch of yours I'm proposing... diff --git a/src/wasm-wasi-component/src/lib.rs b/src/wasm-wasi-component/src/lib.rs
index ed35024a..b0552e81 100644
--- a/src/wasm-wasi-component/src/lib.rs
+++ b/src/wasm-wasi-component/src/lib.rs
@@ -104,7 +104,7 @@ unsafe extern "C" fn start(
) -> bindings::nxt_int_t {
let mut rc: i32 = 0;
- handle_result(task, || {
+ let result = handle_result(task, || {
let config = GLOBAL_CONFIG.get().unwrap();
let state = GlobalState::new(&config)
.context("failed to create initial state")?;
@@ -132,6 +132,10 @@ unsafe extern "C" fn start(
Ok(())
});
+ if result != bindings::NXT_OK as bindings::nxt_int_t {
+ return result;
+ }
+
exit(rc);
}
|
|
Sounds like a plan |
|
I decided to just fold that patch into the main one as it is really Permission to add your |
|
Probably helps if I actually push the changes! |
Makes sense; go for it 👍 |
|
Add Dan's tags |
Have cargo run if for example src/wasm-wasi-component/src/lib.rs is changed, or any of the other files that should perhaps trigger a rebuild. Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Liam reported a problem when trying to restart wasm-wasi-component based applications using the /control/applications/APPLICATION_NAME/restart endpoint. The application would become unresponsive. What was happening was the old application process(es) weren't exit(3)ing and so while we were starting new application processes, the old ones were still hanging around in a non-functioning state. When we are terminating an application it must call exit(3). So that's what we do. We use the return value of nxt_unit_run() as the exit status. Due to exit(3)ing we also need to now explicitly handle the return on error case. Reported-by: Liam Crilly <liam@nginx.com> Fixes: 20ada4b ("Wasm-wc: Core of initial Wasm component model language module support") Closes: nginx#1179 Tested-by: Liam Crilly <liam@nginx.com> Tested-by: Danielle De Leo <d.deleo@f5.com> Co-developed-by: Dan Callahan <d.callahan@f5.com> Signed-off-by: Dan Callahan <d.callahan@f5.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
|
Rebase with master |
This PR comprises two fixes
src/wasm-wasi-component/src/lib.rs)Missing dependencies in the Makefile
Terminating applications must call
exit(2)