c-api: add wasi_config_set_stdout_pipe #5210

ShuP1 · 2022-11-05T12:46:37Z

As discussed in issue #4372 and respective issues about python and go bindings.

Can replace wasi_config_inherit_stdout in examples/wasi/main.c:79

wasi_config_t *wasi_config = wasi_config_new();
assert(wasi_config);
wasi_read_pipe_t *wasi_stdout_reader = NULL;
wasi_write_pipe_t *wasi_stdout_writer = NULL;
wasi_pipe_new(32, &wasi_stdout_reader, &wasi_stdout_writer);
assert(wasi_stdout_reader && wasi_stdout_writer);
wasi_config_set_stdout_pipe(wasi_config, wasi_stdout_writer);
error = wasmtime_context_set_wasi(context, wasi_config);
// run...
size_t expected = wasi_read_pipe_len(wasi_stdout_reader);
byte_t *buf = malloc(expected);
size_t filled = wasi_read_pipe_read(wasi_stdout_reader, buf, expected);
fprintf(stderr, "Pipe:\n%.*s\n", (int)filled, buf);
wasi_read_pipe_delete(wasi_stdout_reader);

See ShuP1@d818f6b

This PR replaces #5189
(cc @alexcrichton)

ShuP1 · 2022-11-05T12:51:42Z

Are the pipes safe to span threads?
Safe to move to another thread but may lock.

Or even maybe dead lock during concurrent use ?
This reaches the limit of my understand of rust async...

https://docs.rs/wasi-common/latest/src/wasi_common/pipe.rs.html#112
RwLock is locked in an async function who does not use await.
So no "context switch" ? and no risk for another thread to hung on this lock ?

ShuP1 · 2022-11-05T12:52:15Z

I'd avoid wasm_byte_vec_t as it's cumbersome to work with and instead use read/write-style buffer passing

Does this point also includes changing that ?

void wasi_config_set_stdin_bytes(wasi_config_t* config, wasm_byte_vec_t* binary);
// to
void wasi_config_set_stdin_bytes(wasi_config_t* config, const byte_t*, size_t);

github-actions · 2022-11-05T13:00:17Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

alexcrichton · 2022-11-07T15:42:16Z

crates/c-api/src/wasi.rs

+    }
+    #[inline]
+    fn flush(&mut self) -> io::Result<()> {
+        self.deque.flush()


I think this can be omitted since it doesn't do anything

std::io::Write::flush has not default body. IMO to force implementers to thinks about it.

Yes that makes sense but VecDeque::flush is a noop.

crates/c-api/include/wasi.h

alexcrichton · 2022-11-07T15:43:20Z

crates/c-api/src/wasi.rs

+pub unsafe extern "C" fn wasi_pipe_new(
+    limit: usize,
+    ret_read_pipe: *mut *mut wasi_read_pipe_t,
+    ret_write_pipe: *mut *mut wasi_write_pipe_t,


I think this can be &mut *mut wasi_write_pipe_t which should remove the need for this function to be unsafe.

Is it possible to "null check" a &mut since it's expected to be a valid reference ?

If not, want would append if the function is given a null pointer ? UB?

Or, is a function documentation enough to be confident than the function is used properly ?

My point is that the &mut layer is part of the contract of the function, it must be valid. What it points to could be invalid, hence the next layer of raw pointers.

alexcrichton · 2022-11-07T15:44:12Z

crates/c-api/include/wasi.h

+ * An instance using fd_read on an empty pipe with succeeded but with 0 bytes read.
+ * The effect depends on the implementation. For example, Rust's
+ * `std::io::Read::read` returns Ok(0), while `read_exact` returns Err


I'm not sure what this is intending to communicate? Could you clarify what this is explaining?

Wasi fd_read (like POSIX one), returns the number of byte read.
When the queue is empty, the returned value is zero.
Effects on programs may vary.
For example, a c program without checks will just ignore this.
While rust print macro unwraps std::io::Read::read_exact and so panic.

If you have a good way to explain that I'm taking it…

crates/c-api/include/wasi.h

alexcrichton · 2022-11-07T15:46:51Z

crates/c-api/src/wasi.rs

+impl io::Read for BoundedVecDeque<u8> {
+    #[inline]
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
+        io::Read::read(&mut self.deque, buf)
+    }
+}
+impl io::Write for BoundedVecDeque<u8> {
+    #[inline]
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        let amt = self.limit.saturating_sub(self.len());
+        let amt = std::cmp::min(amt, buf.len());
+        self.deque.extend(&buf[..amt]);
+        Ok(amt)
+    }


I'm not sure that this read/write impl are the best for WASI. As modeled today I think it would be best to implement synchronous I/O here but this is sort of a form of nonblocking I/O without the ability to be notified when bytes are ready.

Specifically Read will not block when the deque is empty, instead returning Ok(0) which many libraries actually interpret as EOF. Additionally Write will not block when it's full, instead returning Ok(0) which I think libraries take various viewpoints on.

I think it would be better for Read to block on writing and for writing to block on Read.

UNIX pipe returning 0 bytes read means not more data for now

Synchronous IO seems be more user friendly.
But there if not way to read all of stdin for example.
It would just be a deadlock after the last byte.

Furthermore, ReadPipe has not wait to give the hand back to the runtime. It would need to directly implement wasi_common::file::WasiFile::file::WasiFile trait

I don't think unix pipes return 0 when there's no more data, when the write pipe is still open then it blocks waiting for more data. I think the same thing should happen here, the reader blocks until the writer has written data or the writer has been dropped.

c-api: add wasi_read_pipe_t & wasi_write_pipe_t

6dce0dc

github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Nov 5, 2022

alexcrichton reviewed Nov 7, 2022

View reviewed changes

c-api: wasi.h review first pass

d99080e

jbourassa mentioned this pull request Dec 5, 2022

Allow stdin & stdout to write to String bytecodealliance/wasmtime-rb#54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c-api: add wasi_config_set_stdout_pipe #5210

c-api: add wasi_config_set_stdout_pipe #5210

ShuP1 commented Nov 5, 2022 •

edited

Loading

ShuP1 commented Nov 5, 2022

ShuP1 commented Nov 5, 2022

github-actions bot commented Nov 5, 2022

alexcrichton Nov 7, 2022

ShuP1 Nov 8, 2022

alexcrichton Nov 8, 2022

alexcrichton Nov 7, 2022

ShuP1 Nov 8, 2022

alexcrichton Nov 8, 2022

alexcrichton Nov 7, 2022

ShuP1 Nov 8, 2022 •

edited

Loading

alexcrichton Nov 7, 2022

ShuP1 Nov 8, 2022 •

edited

Loading

alexcrichton Nov 8, 2022

c-api: add wasi_config_set_stdout_pipe #5210

Are you sure you want to change the base?

c-api: add wasi_config_set_stdout_pipe #5210

Conversation

ShuP1 commented Nov 5, 2022 • edited Loading

ShuP1 commented Nov 5, 2022

ShuP1 commented Nov 5, 2022

github-actions bot commented Nov 5, 2022

Subscribe to Label Action

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

ShuP1 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShuP1 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShuP1 commented Nov 5, 2022 •

edited

Loading

ShuP1 Nov 8, 2022 •

edited

Loading

ShuP1 Nov 8, 2022 •

edited

Loading