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.

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

WasiExitException thrown every time proc_exit is called #533

Closed
bhelx opened this issue Sep 19, 2024 · 7 comments
Closed

WasiExitException thrown every time proc_exit is called #533

bhelx opened this issue Sep 19, 2024 · 7 comments

Comments

@bhelx
Copy link
Contributor

bhelx commented Sep 19, 2024

Reported in Zulip by Stefan


Hello from Go

Sept. 19, 2024 1:44:15 PM com.dylibso.chicory.log.SystemLogger log
INFORMATION: proc_exit: [0@i32]

An error occurred.
com.dylibso.chicory.runtime.exceptions.WASMMachineException: com.dylibso.chicory.wasi.WasiExitException: Process exit code: 0

It appears this exception is being thrown even when the program exits successfully: https://github.com/dylibso/chicory/blame/b69237737c6f05f74d4c3fafbb1e32a350953e9a/wasi/src/main/java/com/dylibso/chicory/wasi/WasiPreview1.java#L1043

I think there are two options to fix:

  1. only throw the exception when the status-code is non-zero
  2. don't throw the exception at all and propagate the status code through some other means

Though I think it depends on what was the intention behind the exception so perhaps @electrum can provide some insight.

@andreaTP
Copy link
Collaborator

It appears this exception is being thrown even when the program exits successfully

I'm convinced that the current implementation is about "the best" we can offer in terms of consistency.
I think, it is pretty useful to be able to handle the exception "from the outside".

Regarding your options:

  1. I think that doing different things depending on the status code is more opinionated than the current approach, still you can do it by plugging in a custom implementation of the proc_exit Host Function
  2. I have no idea how to design this, also, I would not change a lot the design to support an optional, and easily overridable function

FWIW, additional context is that the only compiler(I'm aware of) that makes use of this Wasi function is the "standard" Go compiler.

@bhelx
Copy link
Contributor Author

bhelx commented Sep 19, 2024

My assumption was that proc_exit is a proxy for the exit syscall, so it's not necessarily compiler specific as any c or rust program could call exit. though i may be wrong there. If the answer is to leave it the way it is, that's fine I think. maybe we just document that it may throw this exception and you need to catch it. Or put it in the example in the Readme

@andreaTP
Copy link
Collaborator

it's not necessarily compiler specific

It shouldn't, but, apparently, it's not widely used in practice.
Happy to document it, should it be in a "standard Go" specific section/FAQ list/whatever?

Fun fact, looking closer at this, if you don't throw the Exception nor explicitly stop the runtime while running "standard Go" the runtime is going to fail trying to perform a "sleep"(and we don't have the wasi function implemented for it yet).

@bhelx
Copy link
Contributor Author

bhelx commented Sep 19, 2024

Fun fact, looking closer at this, if you don't throw the Exception nor explicitly stop the runtime while running "standard Go" the runtime is going to fail trying to perform a "sleep"(and we don't have the wasi function implemented for it yet).

I assume this is because you actually need to explicitly stop the program, which the exception is implicitly doing. There is code after the exit but it should never be executed because calling the syscall on an OS would halt the program. FWIW it does seem that any time you call exit it's turning that into proc_exit. Verified with:

#include <stdlib.h>
#include <stdio.h>

int main() {
    printf("This program will now exit.\n");
    exit(0);
    printf("This line will not be printed.\n");
    return 0;
}

@andreaTP
Copy link
Collaborator

Thanks for looking closer at it!

According to those further findings, I think that "always throwing an exception"(current implementation) is the most idiomatic way to implement the proc_exit wasi function, as it requires the user to handle the situation accordingly.

@electrum
Copy link
Collaborator

As you've shown, the proc_exit call must not return, so it must throw an exception. In order for Chicory to not throw an exception to the user Java code, we would need to add special handling somewhere in Chicory that knows about WASI (or is pluggable somehow), which doesn't seem reasonable.

@andreaTP
Copy link
Collaborator

andreaTP commented Oct 4, 2024

I think this one can be closed, feel free to open a new one or re-open if necessary.

@andreaTP andreaTP closed this as completed Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants