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

Revert "Return error when exception was raised after main thread fini… #2524

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

yamt
Copy link
Collaborator

@yamt yamt commented Aug 31, 2023

…shes (#2169)"

This reverts commit 71d43f3.

for some reasons, the original commit assumes a trap should win over an exit even if the former is 1 second behind the latter. i don't see any reason it should. in that case, the expected behavior is that the exit likely wins and the other thread won't even reach the code to raise a trap.

…shes (bytecodealliance#2169)"

This reverts commit 71d43f3.

for some reasons, the original commit assumes a trap should win over
an exit even if the former is 1 second behind the latter. i don't
see any reason it should. in that case, the expected behavior is that
the exit likely wins and the other thread won't even reach the code to
raise a trap.
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@eloparco
Copy link
Contributor

eloparco commented Sep 4, 2023

for some reasons, the original commit assumes a trap should win over an exit

Is that because a return from main gets compiled to proc_exit() with wasi-sdk? If that's the case, it makes sense. When I wrote the test, I wasn't considering that.

@yamt
Copy link
Collaborator Author

yamt commented Sep 4, 2023

for some reasons, the original commit assumes a trap should win over an exit

Is that because a return from main gets compiled to proc_exit() with wasi-sdk? If that's the case, it makes sense. When I wrote the test, I wasn't considering that.

how a return from main should behave is defined by C standard.
you don't need to look at the specific implementation in wasi-sdk to write a C-level test.

@wenyongh
Copy link
Contributor

wenyongh commented Sep 4, 2023

@eloparco Is the PR good to you? Shall we merge it?

Copy link
Collaborator

@loganek loganek left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit 9e39043 into bytecodealliance:main Sep 4, 2023
368 checks passed
@eloparco
Copy link
Contributor

eloparco commented Sep 4, 2023

Is that because a return from main gets compiled to proc_exit() with wasi-sdk?

Actually no, that's not the case, that's why the test was returning 1.

The wasi-threads proposal doesn't state that the process ends if the main thread ends (as for phtreads). So in the test in the PR I would expect the main thread to end and the process to terminate after the spawned thread finishes. Since the thread raises a trap, I would expect it to be propagated.
Not sure if that can be considered an implementation detail.

@yamt
Copy link
Collaborator Author

yamt commented Sep 4, 2023

Is that because a return from main gets compiled to proc_exit() with wasi-sdk?

Actually no, that's not the case, that's why the test was returning 1.

The wasi-threads proposal doesn't state that the process ends if the main thread ends (as for phtreads). So in the test in the PR I would expect the main thread to end and the process to terminate after the spawned thread finishes. Since the thread raises a trap, I would expect it to be propagated. Not sure if that can be considered an implementation detail.

see WebAssembly/wasi-threads#21
the option b is what wasi-libc assumes.

@eloparco
Copy link
Contributor

eloparco commented Sep 4, 2023

So wasi-libc doesn't compile the return from main as proc_exit. The runtime decides whether or not to wait for all threads to finish. In WAMR implementation, we wait for them to finish.

In this scenario, shouldn’t the exception from the spawned thread be propagated and returned even if the main thread has already finished? That's what #2524 is reverting.

@eloparco
Copy link
Contributor

eloparco commented Sep 4, 2023

see WebAssembly/wasi-threads#21
the option b is what wasi-libc assumes.

Ok not sure that's what's currently implemented, last time I checked iwasm was waiting for all threads to finish. Worth adding an assert(false) at the end of the spawned thread execution in trap_after_main_thread_finishes.c to validate the behavior.

@yamt
Copy link
Collaborator Author

yamt commented Sep 4, 2023

see WebAssembly/wasi-threads#21
the option b is what wasi-libc assumes.

Ok not sure that's what's currently implemented, last time I checked iwasm was waiting for all threads to finish. Worth adding an assert(false) at the end of the spawned thread execution in trap_after_main_thread_finishes.c to validate the behavior.

see also #2529

we should test how it should be, not what it happens to be implemented.

@eloparco
Copy link
Contributor

eloparco commented Sep 4, 2023

we should test how it should be, not what it happens to be implemented.

But this is WAMR repository not the wasi-threads one. I think if we decide for a specific implementation we have to document it and test it, otherwise users wouldn't know what to expect.

@yamt
Copy link
Collaborator Author

yamt commented Sep 4, 2023

we should test how it should be, not what it happens to be implemented.

But this is WAMR repository not the wasi-threads one. I think if we decide for a specific implementation we have to document it and test it, otherwise users wouldn't know what to expect.

i'm not sure if i follow your logic.
do you mean wamr should behave differently?

@eloparco
Copy link
Contributor

eloparco commented Sep 4, 2023

i'm not sure if i follow your logic.
do you mean wamr should behave differently?

That whatever we implement we should update the test to check it.

for some reasons, the original commit assumes a trap should win over an exit even if the former is 1 second behind the latter.

The original commit was working as a. from WebAssembly/wasi-threads#21, so the main thread was exiting without stopping the spawned thread. When the spawned thread raises the exception afterwords, it gets propagated and returned to the host.

What I see from #2529 is that you want to implement it as b.

@yamt
Copy link
Collaborator Author

yamt commented Sep 4, 2023

for some reasons, the original commit assumes a trap should win over an exit even if the former is 1 second behind the latter.

The original commit was working as a. from WebAssembly/wasi-threads#21, so the main thread was exiting without stopping the spawned thread. When the spawned thread raises the exception afterwords, it gets propagated and returned to the host.

it's broken.
regardless options in WebAssembly/wasi-threads#21,
a return from main() should be an equivalent of exit() as in C standard.

yamt added a commit to yamt/wasm-micro-runtime that referenced this pull request Sep 20, 2023
On posix-like platforms, the rest of wasi-threads tests
should pass after the recent changes including the following PRs:

bytecodealliance#2516
bytecodealliance#2524
bytecodealliance#2529
yamt added a commit to yamt/wasm-micro-runtime that referenced this pull request Sep 21, 2023
On posix-like platforms, the rest of wasi-threads tests
should pass after the recent changes including the following PRs:

bytecodealliance#2516
bytecodealliance#2524
bytecodealliance#2529
yamt added a commit to yamt/wasm-micro-runtime that referenced this pull request Sep 22, 2023
On posix-like platforms, the rest of wasi-threads tests
should pass after the recent changes including the following PRs:

bytecodealliance#2516
bytecodealliance#2524
bytecodealliance#2529
yamt added a commit to yamt/wasm-micro-runtime that referenced this pull request Sep 23, 2023
On posix-like platforms, the rest of wasi-threads tests
should pass after the recent changes including the following PRs:

bytecodealliance#2516
bytecodealliance#2524
bytecodealliance#2529
wenyongh pushed a commit that referenced this pull request Sep 26, 2023
On posix-like platforms, the rest of wasi-threads tests
should pass after the recent changes including the following PRs:
#2516, #2524, #2529, #2571, #2576 and #2582.
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
On posix-like platforms, the rest of wasi-threads tests
should pass after the recent changes including the following PRs:
bytecodealliance#2516, bytecodealliance#2524, bytecodealliance#2529, bytecodealliance#2571, bytecodealliance#2576 and bytecodealliance#2582.
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

Successfully merging this pull request may close these issues.

None yet

4 participants