Add some concepts related to exit(3) #17

yamt · 2022-12-15T10:06:12Z

Introduce process and its exit status

loganek · 2022-12-15T10:38:35Z

I think we probably want to reach a conclusion here before moving forward with the changes in the proposal?

yamt · 2022-12-15T11:55:55Z

I think we probably want to reach a conclusion here before moving forward with the changes in the proposal?

sure. let me mark this a draft for now.
depending on the conclusion, i will remove wasi_thread_exit part.

sbc100 · 2022-12-15T17:04:53Z

README.md

+* Threads created by a thread in a thread group using `wasi_thread_spawn`
+  is added to the thread group.
+
+* When a thread is terminated, it's removed from the thread group.


Do we even need a concept of thread group? Why not just say that wasi_thread_spawn creates new threads and wasi_thread_exit terminates the current thread?

The thread group concept seems like something fairly specific to linux and/or the world outside of the process.

If we do want to give a name the container of threads why not use the commonly used term "process"?

i just wanted a name for what wasi proc_exit terminates. the name "process" is fine for me too.

i renamed it to process.

sbc100 · 2022-12-15T17:05:41Z

README.md

+### Changes to WASI `proc_exit`
+
+With this proposal, the `proc_exit` function takes extra responsibility
+to terminate all threads in the thread group, not only the calling one.


Again just not just drop the in the thread group part of this phrase.

sbc100 · 2022-12-15T17:07:41Z

README.md

+When a thread caused a trap, it terminates all threads in the thread group
+similarly to `proc_exit`.
+
+### Thread group exit status


How about Process exit status ? This terminology makes sense to me and aligns with the proc_exit name (the proc here stands for process)

WebAssembly/wasi-threads#7 WebAssembly/wasi-threads#17

abrown

Even if we end up deciding to add wasi_thread_exit in #7, I think I have some concerns about this:

I think talking of "processes" and "thread groups" could be confusing: I see that @sbc100 has asked for changes to the terminology and I don't want to nitpick that, but since Wasm/WASI does not really have these concepts and people already have definitions for them in mind, I would prefer to not use them in the proposal — e.g., we could speak instead of "running module" and "all the threads" instead.
I think this change, as written, risks over-specifying things at this stage: e.g., I'm not too sure we want to terminate all threads if one of them traps (I assume via Wasm unreachable). And why concern ourselves (or not) about which thread determines the wasi_proc_exit exit code? I would prefer these kinds of thing to be left open-ended until we have a more clear idea if they must be specified.

yamt · 2022-12-19T06:58:44Z

Even if we end up deciding to add wasi_thread_exit in #7, I think I have some concerns about this:

* I think talking of "processes" and "thread groups" could be confusing: I see that @sbc100 has asked for changes to the terminology and I don't want to nitpick that, but since Wasm/WASI does not really have these concepts and people already have definitions for them in mind, I would prefer to not use them in the proposal — e.g., we could speak instead of "running module" and "all the threads" instead.

i want to introduce a term because i feel "all the threads" is a bit vague.

as @sbc100 pointed out, we already have the term in the name of "wasi_proc_exit".

* I think this change, as written, risks over-specifying things at this stage: e.g., I'm not too sure we want to terminate all threads if one of them traps (I assume via Wasm `unreachable`). And why concern ourselves (or not) about which thread determines the `wasi_proc_exit` exit code? I would prefer these kinds of thing to be left open-ended until we have a more clear idea if they must be specified.

everything in this PR was something i actually had to decide while implementing wasi-threads in toywasm.
are you suggesting to make some of these "implementation-defined"?

The current wasi-threads has no thread-exit functionality. Thus it isn't straightforward to implement pthread_exit without leaking thread context. This commit simply disables pthread_exit for now. Also, instead of abusing `wasi_proc_exit` for thread exit, make let `wasi_thread_start` return. Note: `wasi_proc_exit` is supposed to terminate all threads in the "process", not only the calling thread. Note: Depending on the conclusion of the discussion about `wasi_thread_exit`, we might revisit this change later. References: WebAssembly/wasi-threads#7 WebAssembly/wasi-threads#17

The current wasi-threads has no thread-exit functionality. Thus it isn't straightforward to implement pthread_exit without leaking thread context. This commit simply disables pthread_exit for now. Also, instead of abusing `wasi_proc_exit` for thread exit, make `wasi_thread_start` return. Note: `wasi_proc_exit` is supposed to terminate all threads in the "process", not only the calling thread. Note: Depending on the conclusion of the discussion about `wasi_thread_exit`, we might revisit this change later. References: WebAssembly/wasi-threads#7 WebAssembly/wasi-threads#17

yamt · 2023-01-04T03:25:52Z

i removed thread_exit as it still seems controversial.

* Introduce wasi_thread_exit * Introduce thread group and its exit status

It's a more commen name and matches "proc" in `proc_exit`.

yamt · 2023-01-04T12:20:57Z

i resolved conflicts after #16

sbc100 · 2023-01-04T12:48:33Z

README.md

+of the process.
+It's non deterministic which one is chosen.
+
+If the process gets empty without involving `proc_exit` or a trap,


"gets empty" sounds a little odd to me. How about "If the last remaining thread in the process returns from its entry point without trapping or calling proc_exit it is treated as if proc_exit was called with exit code 0".

it isn't only about the last thread.
eg. if the second last thread terminated itself with a trap and the last thread returned from wasi_thread_start, i still want to make the trap represent the process exit status.

also, i want to avoid mentioning the entry point in this sentence because it can involve another topic: #21

how about:

If all the threads in the process have been terminated without calling `proc_exit` or raising a trap, it's treated as if the last thread called `proc_exit` with exit code 0.

Sure thats sounds fine. Perhaps we can define "normal termination" and "early termination" and say "If all the threads in the process terminate normally"?

Sure thats sounds fine.

done

Perhaps we can define "normal termination" and "early termination" and say "If all the threads in the process terminate normally"?

i feel it confusing as calling exit() is quite normal.

I think its reasonable to refer to "exit()" as early and/or abnormal termination.

loganek · 2023-01-06T17:40:05Z

README.md

+  the main thread.
+
+* Threads created by a thread in a process using `wasi_thread_spawn`
+  is added to the process.


Suggested change

is added to the process.

are added to the process.

loganek · 2023-01-06T17:41:07Z

README.md

+With this proposal, the `proc_exit` function takes extra responsibility
+to terminate all threads in the process, not only the calling one.
+
+Any of threads in the process can call `proc_exit`.


Suggested change

Any of threads in the process can call `proc_exit`.

Any of the threads in the process can call `proc_exit`.

loganek · 2023-01-06T17:45:34Z

README.md

+
+If all the threads in the process have been terminated without calling
+`proc_exit` or raising a trap, it's treated as if the last thread called
+`proc_exit` with exit code 0.


Should we not mention proc_exit here and just say that exit code is 0? It might get a bit confusing if e.g. in the future proc_exit is extended with additional functionality - in that case, this sentence can be interpreted as that additional functionality should also be executed along with returning 0 as exit code (whereas I think it shouldn't).

i can't imagine any functionalities which proc_exit might gain but on the other hand should not be a part of this. do you have examples?

even if we avoid mentioning proc_exit here, when you are adding an extra functionality to proc_exit, you still need to consider if the change should apply to this sentence or not.

do you have examples?

No, it was only hypothetical question.

you still need to consider if the change should apply to this sentence or not.

Yes

loganek · 2023-01-09T15:52:11Z

README.md

+
+### Process exit status
+
+If one or more threads call WASI `proc_exit` or raise a trap,


Does it include calling it proc_exit from the main thread?

As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17

sbc100 · 2023-01-11T11:47:55Z

README.md

+### Changes to WASI `proc_exit`
+
+With this proposal, the `proc_exit` function takes extra responsibility
+to terminate all threads in the process, not only the calling one.


How about simply "When proc_exit is called the entire process is terminated, including all running threads". I'm not we need the "takes extra responsibility" wording.

It seems fairly clear from its name that proc_exit would do this, so I don't really see it as "extra responsibility", but maybe thats just my reading of it.

without wasi-threads, a runtime would simply terminate the calling thread.

when adding wasi-threads support to a runtime, you need to make it terminate other thread too. it would need a significant implementation effort. i feel it's appropriate to call it extra.

Sure. But don't think that is clear enough to simply say: "When proc_exit is called the entire process is terminated, including all running threads".

(BTW, the way I see it proc_exit brings down the entire process (that is that the proc part means), even without wasi-threads. It just happens that there was only ever one thread previously.)

maybe. i wanted to somehow emphasize that this is something a runtime needs to implement.

sbc100 · 2023-01-11T11:49:19Z

README.md

+
+### Traps
+
+When a thread caused a trap, it terminates all threads in the process


s/caused/causes/ ?

How about "When a trap occurs in any thread, the entire process is terminated."?

s/caused/causes/ ?

probably. i'm not good at english as you may noticed.

How about "When a trap occurs in any thread, the entire process is terminated."?

done.

sbc100 · 2023-01-11T11:53:45Z

README.md

+of the process.
+It's non deterministic which one is chosen.
+
+If all the threads in the process have been terminated without calling


How about, "If that last running thread in a process terminates without calling proc_exit or trapping it is treated as if ... "

it's not only about the last running thread.

consider a process with two threads.

a thread terminated itself by calling proc_exit(50)

another thread, which is the last running thread, terminates itself by returning from wasi_thread_spawn before the runtime terminates it for proc_exit.

the exit code of the process should be 50.

i think your wording can be interpreted as the exit code of the process can be 0.

My my understanding, in your example above proc_exit(50) causes all threads to be terminated, so its not possible (2) to occor.

Another way of putting it "proc_exitdoes not complete until all threads are terminated, so not thread can exist afterproc_exit` is complete.

My my understanding, in your example above proc_exit(50) causes all threads to be terminated, so its not possible (2) to occor.

if things work in a lockstep, maybe.
but we are talking about threads...

Another way of putting it "proc_exitdoes not complete until all threads are terminated, so not thread can exist afterproc_exit` is complete.

in that case, i guess you need to say the thread which called proc_exit is terminated after all other threads are terminated. i feel it's more complicated than the current sentence.

I think it would odd if we allowed any threads to outlive the thread that calls proc_exit.. the whole point of that system call is to bring down all the threads so logically nothing else can happen on any thread once that syscall completes.

I don't want to hold up this PR, even though I don't love the way this sentence is phrased. I guess we can land this as it stands and try to improve it later.

Yeah, let me know if #27 addresses some of the things you bring up here...

sbc100

lgtm % nits

yamt · 2023-01-18T06:23:41Z

is there any blocker on this?

abrown

I'm going to merge this as-is, despite the nits @sbc100 brought up remaining unaddressed. The spec does need to say something about how proc_exit shuts down all the threads and this starts that process--it's progress! I will immediately open up a follow-up PR to see if I can resolve some of the finer points noted here by others. Thanks @yamt!

Recent changes (e.g., WebAssembly#17) introduced new concepts necessary for the specification. This change migrates this new text to the WASI proposal template style by: - linking each section in the document index - using paragraph style more extensively - clarifying and compacting several sentences This change is not meant to introduce anything new; it should solely be a style and clarity edit.

Recent changes (e.g., #17) introduced new concepts necessary for the specification. This change migrates this new text to the WASI proposal template style by: - linking each section in the document index - using paragraph style more extensively - clarifying and compacting several sentences

As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17

This commit includes a set of changes that add initial support for `wasi-threads` to Wasmtime: * feat: remove mutability from the WasiCtx Table This patch adds interior mutability to the WasiCtx Table and the Table elements. Major pain points: * `File` only needs `RwLock<cap_std::fs::File>` to implement `File::set_fdflags()` on Windows, because of [1] * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be hold across an `.await`, The `async` from `async fn num_ready_bytes(&self)` had to be removed * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be dereferenced in `pollable`, the signature of `fn pollable(&self) -> Option<rustix::fd::BorrowedFd>` changed to `fn pollable(&self) -> Option<Arc<dyn AsFd + '_>>` [1] https://github.com/bytecodealliance/system-interface/blob/da238e324e752033f315f09c082ad9ce35d42696/src/fs/fd_flags.rs#L210-L217 * wasi-threads: add an initial implementation This change is a first step toward implementing `wasi-threads` in Wasmtime. We may find that it has some missing pieces, but the core functionality is there: when `wasi::thread_spawn` is called by a running WebAssembly module, a function named `wasi_thread_start` is found in the module's exports and called in a new instance. The shared memory of the original instance is reused in the new instance. This new WASI proposal is in its early stages and details are still being hashed out in the [spec] and [wasi-libc] repositories. Due to its experimental state, the `wasi-threads` functionality is hidden behind both a compile-time and runtime flag: one must build with `--features wasi-threads` but also run the Wasmtime CLI with `--wasm-features threads` and `--wasi-modules experimental-wasi-threads`. One can experiment with `wasi-threads` by running: ```console $ cargo run --features wasi-threads -- \ --wasm-features threads --wasi-modules experimental-wasi-threads \ <a threads-enabled module> ``` Threads-enabled Wasm modules are not yet easy to build. Hopefully this is resolved soon, but in the meantime see the use of `THREAD_MODEL=posix` in the [wasi-libc] repository for some clues on what is necessary. Wiggle complicates things by requiring the Wasm memory to be exported with a certain name and `wasi-threads` also expects that memory to be imported; this build-time obstacle can be overcome with the `--import-memory --export-memory` flags only available in the latest Clang tree. Due to all of this, the included tests are written directly in WAT--run these with: ```console $ cargo test --features wasi-threads -p wasmtime-cli -- cli_tests ``` [spec]: https://github.com/WebAssembly/wasi-threads [wasi-libc]: https://github.com/WebAssembly/wasi-libc This change does not protect the WASI implementations themselves from concurrent access. This is already complete in previous commits or left for future commits in certain cases (e.g., wasi-nn). * wasi-threads: factor out process exit logic As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17 * review: work around `fd_fdstat_set_flags` In order to make progress with wasi-threads, this change temporarily works around limitations induced by `wasi-common`'s `fd_fdstat_set_flags` to allow `&mut self` use in the implementation. Eventual resolution is tracked in #5643. This change makes several related helper functions (e.g., `set_fdflags`) take `&mut self` as well. * test: use `wait`/`notify` to improve `threads.wat` test Previously, the test simply executed in a loop for some hardcoded number of iterations. This changes uses `wait` and `notify` and atomic operations to keep track of when the spawned threads are done and join on the main thread appropriately. * various fixes and tweaks due to the PR review --------- Signed-off-by: Harald Hoyer <[email protected]> Co-authored-by: Harald Hoyer <[email protected]> Co-authored-by: Alex Crichton <[email protected]>

yamt marked this pull request as draft December 15, 2022 11:56

sbc100 reviewed Dec 15, 2022

View reviewed changes

yamt added a commit to yamt/wasi-libc that referenced this pull request Dec 16, 2022

Use wasi thread_exit instead of proc_exit where appropriate

9a1530e

WebAssembly/wasi-threads#7 WebAssembly/wasi-threads#17

yamt mentioned this pull request Dec 16, 2022

Use wasi thread_exit instead of proc_exit where appropriate WebAssembly/wasi-libc#364

Draft

abrown reviewed Dec 16, 2022

View reviewed changes

yamt mentioned this pull request Dec 19, 2022

Disable pthread_exit for now WebAssembly/wasi-libc#366

Merged

yamt marked this pull request as ready for review January 4, 2023 03:25

yamt added 3 commits January 4, 2023 21:19

Add some concepts related to exit(3)

2b7fb32

* Introduce wasi_thread_exit * Introduce thread group and its exit status

Rename thread group to process

ccc5811

It's a more commen name and matches "proc" in `proc_exit`.

Remove thread_exit for now

ec8172d

yamt force-pushed the exit branch from 8fbf828 to ec8172d Compare January 4, 2023 12:20

sbc100 reviewed Jan 4, 2023

View reviewed changes

try to make a sentence less odd

dc1ac30

eloparco mentioned this pull request Jan 6, 2023

Handle thread exit bytecodealliance/wasm-micro-runtime#1869

Merged

loganek reviewed Jan 6, 2023

View reviewed changes

loganek reviewed Jan 9, 2023

View reviewed changes

english improvements

f74d69d

sbc100 reviewed Jan 11, 2023

View reviewed changes

sbc100 approved these changes Jan 11, 2023

View reviewed changes

english improvement

c732034

tonibofarull approved these changes Jan 18, 2023

View reviewed changes

abrown approved these changes Jan 19, 2023

View reviewed changes

abrown merged commit 4a9776c into WebAssembly:main Jan 19, 2023

abrown mentioned this pull request Jan 19, 2023

Edit README for clarity #27

Merged

	Any of threads in the process can call `proc_exit`.
	Any of the threads in the process can call `proc_exit`.


		### Process exit status

		If one or more threads call WASI `proc_exit` or raise a trap,


		### Traps

		When a thread caused a trap, it terminates all threads in the process

Add some concepts related to exit(3) #17

Add some concepts related to exit(3) #17

Conversation

yamt commented Dec 15, 2022 • edited Loading

loganek commented Dec 15, 2022

yamt commented Dec 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrown left a comment

Choose a reason for hiding this comment

yamt commented Dec 19, 2022

yamt commented Jan 4, 2023

yamt commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganek Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

yamt commented Jan 18, 2023

abrown left a comment

Choose a reason for hiding this comment

yamt commented Dec 15, 2022 •

edited

Loading

loganek Jan 9, 2023 •

edited

Loading

sbc100 Jan 11, 2023 •

edited

Loading