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

__stdio_exit.c: use normal file locking for now #363

Closed
wants to merge 1 commit into from

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Dec 13, 2022

Unlike many of platforms with threads/lwps, wasi doesn't have separate api for thread exit and process exit as of writing this. It only has a single wasi_proc_exit, which is currently used for both of process exit and thread exit by wasi-libc. Becasue of that, on wasi, exit(3) only terminates the calling thread.

On the other hand, the stdio exit logic uses FFINALLOCK macro, which leaves the file locked. While it's fine on platforms where exit somehow forcibly terminates other threads soon, it can make other threads block on these locks forever on wasi.

Until the situation is cleaned up, this commit makes stdio exit use normal file locking instead of the "final" variant. This change allows other threads to continue running. Hopefully those threads will exit by themselves soon.

cf. WebAssembly/wasi-threads#7

Unlike many of platforms with threads/lwps, wasi doesn't
have separate api for thread exit and process exit as of writing
this. It only has a single `wasi_proc_exit`, which is currently
used for both of process exit and thread exit by wasi-libc.
Becasue of that, on wasi, exit(3) only terminates the calling thread.

On the other hand, the stdio exit logic uses FFINALLOCK macro,
which leaves the file locked. While it's fine on platforms where
exit somehow forcibly terminates other threads soon, it can make
other threads block on these locks forever on wasi.

Until the situation is cleaned up, this commit makes stdio exit use
normal file locking instead of the "final" variant. This change
allows other threads to continue running. Hopefully those threads
will exit by themselves soon.

cf. WebAssembly/wasi-threads#7
@sbc100
Copy link
Member

sbc100 commented Dec 13, 2022

I think wasi_proc_exit should always exit the entire process and not be used to implement pthread_exit. I think the discussion in #7 is about whether we need to add a new API to implement pthread_exit, or not implement it at all.

IIUC, the semantics of wasi_proc_exit should be that the whole process exits, regardless of the how the discussion in #7 goes.

If threads continue after exit then other bad things can happen, not just the use of closed files. We run other destruction stuff on exit too:

void __wasm_call_dtors(void)
{
__funcs_on_exit();
__stdio_exit();
}
_Noreturn void exit(int code)
{
__wasm_call_dtors();
_Exit(code);
}
#endif

@yamt
Copy link
Contributor Author

yamt commented Dec 13, 2022

I think wasi_proc_exit should always exit the entire process and not be used to implement pthread_exit.

i agree it's what the name wasi_proc_exit sounds.

but it's less clear to me what wasi_proc_exit should do for web worker style threading w/o wasi-threads.
should it somehow kill the entire agent cluster?

also, "kill other thread" is a somehow new concept to implement.

I think the discussion in #7 is about whether we need to add a new API to implement pthread_exit, or not implement it at all.

although i feel it's widely used api, it's a valid option.

IIUC, the semantics of wasi_proc_exit should be that the whole process exits, regardless of the how the discussion in #7 goes.

is it explicitly stated in some spec?

If threads continue after exit then other bad things can happen, not just the use of closed files. We run other destruction stuff on exit too:

sure. even if those threads do nothing particularly bad, running threads itself is a bad thing.

@sbc100
Copy link
Member

sbc100 commented Dec 13, 2022

I think wasi_proc_exit should always exit the entire process and not be used to implement pthread_exit.

i agree it's what the name wasi_proc_exit sounds.

but it's less clear to me what wasi_proc_exit should do for web worker style threading w/o wasi-threads. should it somehow kill the entire agent cluster?

also, "kill other thread" is a somehow new concept to implement.

In emscripten this is implemented by sending and message back to the main thread that can then terminate all the workers. i.e. there is a main thread that manages all the pthread workers, and it is in charge of creating new ones and destroying all of them on program exit.

I think the discussion in #7 is about whether we need to add a new API to implement pthread_exit, or not implement it at all.

although i feel it's widely used api, it's a valid option.

IIUC, the semantics of wasi_proc_exit should be that the whole process exits, regardless of the how the discussion in #7 goes.

is it explicitly stated in some spec?

wasi_proc_exit was created before wasi-threads, so I doubt the spec for wasi_proc_exit makes any claim about this currently. I guess it would be up to wasi-threads proposal to specify this.

If threads continue after exit then other bad things can happen, not just the use of closed files. We run other destruction stuff on exit too:

sure. even if those threads do nothing particularly bad, running threads itself is a bad thing.

@yamt
Copy link
Contributor Author

yamt commented Dec 14, 2022

I think wasi_proc_exit should always exit the entire process and not be used to implement pthread_exit.

i agree it's what the name wasi_proc_exit sounds.
but it's less clear to me what wasi_proc_exit should do for web worker style threading w/o wasi-threads. should it somehow kill the entire agent cluster?
also, "kill other thread" is a somehow new concept to implement.

In emscripten this is implemented by sending and message back to the main thread that can then terminate all the workers. i.e. there is a main thread that manages all the pthread workers, and it is in charge of creating new ones and destroying all of them on program exit.

ok. i didn't know that. thank you.

I think the discussion in #7 is about whether we need to add a new API to implement pthread_exit, or not implement it at all.

although i feel it's widely used api, it's a valid option.

IIUC, the semantics of wasi_proc_exit should be that the whole process exits, regardless of the how the discussion in #7 goes.

is it explicitly stated in some spec?

wasi_proc_exit was created before wasi-threads, so I doubt the spec for wasi_proc_exit makes any claim about this currently. I guess it would be up to wasi-threads proposal to specify this.

ok

If threads continue after exit then other bad things can happen, not just the use of closed files. We run other destruction stuff on exit too:

sure. even if those threads do nothing particularly bad, running threads itself is a bad thing.

@yamt
Copy link
Contributor Author

yamt commented Dec 19, 2022

i implemented in the "proc_exit terminates all threads" semantics in an engine and confirmed this change is not necessary.

@yamt yamt closed this Dec 19, 2022
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.

2 participants