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

fd_write should be unbuffered #255

Closed
kanaka opened this issue Aug 6, 2019 · 5 comments · Fixed by CraneStation/wasi-common#55
Closed

fd_write should be unbuffered #255

kanaka opened this issue Aug 6, 2019 · 5 comments · Fixed by CraneStation/wasi-common#55
Assignees
Labels
wasi:api Issues pertaining to the WASI API, not necessarily specific to Wasmtime.

Comments

@kanaka
Copy link
Contributor

kanaka commented Aug 6, 2019

fd_write should be unbuffered to match the behavior of writev. Buffering can be implemented in the application if desired, however, fd_write being buffered means there is no way to do unbuffered writes (there is no WASI flush API call and there shouldn't be at this level). This is using the dev release of wasmtime from today (95bcc63). This is a regression from a a few months ago. Note that recent builds of wasmer and lucet have unbuffered fd_write.

Here is a simple test case that should print a prompt, read a string and echo it:

(module
  (import "wasi_unstable" "fd_write" (func $fd_write (param i32 i32 i32 i32) (result i32)))
  (import "wasi_unstable" "fd_read" (func $fd_read (param i32 i32 i32 i32) (result i32)))

  (memory 10)
  (export "memory" (memory 0))

  (func $main (local i32)
    (i32.store offset=0 (i32.const 4) (i32.const 12))  ;; iovec ptr
    (i32.store offset=4 (i32.const 4) (i32.const 2))   ;; iovec len
    (i32.store offset=0 (i32.const 12) (i32.const 62)) ;; string '> '
    (i32.store offset=1 (i32.const 12) (i32.const 32))
    (i32.store offset=2 (i32.const 12) (i32.const 0))
    ;; print prompt
    (drop (call $fd_write (i32.const 1) (i32.const 4) (i32.const 1) (i32.const 256)))

    (i32.store offset=0 (i32.const 16) (i32.const 24))  ;; iovec ptr
    (i32.store offset=4 (i32.const 16) (i32.const 200)) ;; iovec len
    ;; read
    (drop (call $fd_read (i32.const 0) (i32.const 16) (i32.const 1) (i32.const 256)))
    ;; echo
    (drop (call $fd_write (i32.const 1) (i32.const 16) (i32.const 1) (i32.const 256)))
  )

  ;;(start $main)
  (export "_start" $main)
)

And here is the behavior:

$ wasm-as wasitests/echo2.wat -o wasitests/echo2.wasm
$ wasmtime wasitests/echo2.wasm
typed by the user
> typed by the user

The correct behavior should be:

$ wasmtime wasitests/echo2.wasm
> typed by the user
typed by the user
@kubkon
Copy link
Member

kubkon commented Aug 6, 2019

Hi! Thanks for filing the issue. We've recently changed the implementation of fd_write and fd_read and this must have sneaked it. I'll have a look and let you know when I have a draft of a fix :-)

@kubkon kubkon self-assigned this Aug 6, 2019
@kubkon kubkon added the wasi:api Issues pertaining to the WASI API, not necessarily specific to Wasmtime. label Aug 6, 2019
sunfishcode pushed a commit to CraneStation/wasi-common that referenced this issue Aug 7, 2019
@kubkon
Copy link
Member

kubkon commented Aug 7, 2019

@kanaka it should be fixed now. Could you double check against the latest upstream and confirm that it works fine for you?

@kanaka
Copy link
Contributor Author

kanaka commented Aug 7, 2019

I rebuilt wasmtime and didn't get the fix. Then realized it's in a module. I tried updating the version of wasi-common in Cargo.toml and got the following link time errors:

...
/mal/wasmtime/target/release/deps/libwasi_common-24c49d090c9bb950.rlib(wasi_common-24c49d090c9bb950.wasi_common.2pbfosmq-cgu.6.rcgu.o):wasi_common.2pbfosmq-cgu.6:(.text.wasi_common_random_get+0x0): first defined here
/mal/wasmtime/target/release/deps/libwasi_common-8b3e6efcaec3722c.rlib(wasi_common-8b3e6efcaec3722c.wasi_common.3sry3gv5-cgu.15.rcgu.o): In function `wasi_common_sched_yield': wasi_common.3sry3gv5-cgu.15:(.text.wasi_common_sched_yield+0x0): multiple definition of `wasi_common_sched_yield'
/mal/wasmtime/target/release/deps/libwasi_common-24c49d090c9bb950.rlib(wasi_common-24c49d090c9bb950.wasi_common.2pbfosmq-cgu.6.rcgu.o):wasi_common.2pbfosmq-cgu.6:(.text.wasi_common_sched_yield+0x0): first defined here

Can you give me a hint about how I should build this?

@kubkon
Copy link
Member

kubkon commented Aug 8, 2019

Ahhh, apologies for that! It seems that wasi-common was not updated to the latest rev in upstream. I've bumped the rev and so it should be fine now if you build upstream directly.

In terms of the error, it looks to me like you've updated the version of wasi-common in only one crate being wasmtime-wasi but it also needs to be updated in the Wasmtime crate's root as well Cargo.toml. :-)

@kanaka
Copy link
Contributor Author

kanaka commented Aug 8, 2019

I pulled down the nightly CI build and can confirm that it is now fixed. And you're right, I did only update the wasi-common in one place without realizing there are multiple levels that depend on it. Thanks for the fix!

grishasobol pushed a commit to grishasobol/wasmtime that referenced this issue Nov 29, 2021
* Minor fixes to prevent warnings related to Rust 2021 in the tests
* Remove unnecessary references
* Silence clippy

Co-authored-by: Sergei Shulepov <sergei@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi:api Issues pertaining to the WASI API, not necessarily specific to Wasmtime.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants