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

Implement wasi::poll_oneoff on Windows #570

Closed
saghul opened this issue Jul 19, 2019 · 21 comments
Closed

Implement wasi::poll_oneoff on Windows #570

saghul opened this issue Jul 19, 2019 · 21 comments
Assignees
Labels
🎉 enhancement New feature! 📦 lib-wasi About wasmer-wasi 🖼️ platform-windows This issue happens on Windows priority-medium Medium priority issue
Milestone

Comments

@saghul
Copy link

saghul commented Jul 19, 2019

Motivation

It's part of the WASI API, but not yet implemented. Also, wasmtime does implement it ;-)

@saghul saghul added the 🎉 enhancement New feature! label Jul 19, 2019
@MarkMcCaskey
Copy link
Contributor

We're missing a few pieces of WASI now that we can implement and I'll be prioritizing that in the next couple of weeks, but there are a few pieces that aren't really defined yet.

Sockets and poll_oneoff haven't been designed yet. Development of the pieces of WASI (by the WASI subgroup) hasn't been a priority yet because we're still defining the groundwork on which we can work independently on the various pieces. WASI is getting close to being ready to start development on these pieces though.

I can't give you any kind of timeline, but we'll implement it as soon as it's defined (or during its definition so we can give feedback)

@saghul
Copy link
Author

saghul commented Jul 19, 2019

I'm very new to WASI (and WASM for that matter) so excuse my ignorance :-) At this point, I'm trying to run a JS interpreter which uses select / poll for timers. I know sockets are not there yet, but I was under the impression that poll_oneoff was already part of WASI: https://github.com/WebAssembly/WASI/blob/b81f586ecdca184764e91587a18d59b1d85242c9/design/WASI-core.md#__wasi_poll_oneoff

AFAIS the WASI C SDK does implement poll(2) using __wasi_poll_oneoff: https://github.com/CraneStation/wasi-libc/blob/320054e84f8f2440def3b1c8700cedb8fd697bf8/libc-bottom-half/cloudlibc/src/libc/poll/poll.c and FWIW wasmtime does seem to provide an implementation for it.

@MarkMcCaskey
Copy link
Contributor

Oh, I see! Thanks for correcting me! I misread your post as "wasmtime doesn't implement it" and assumed this was something that hadn't been designed yet.

Sure, then this will be on my list of things to add in the coming week or two!

@MarkMcCaskey
Copy link
Contributor

Okay! I'm actively working on this now, do you happen to have a Wasm binary I can use to test this? If not, don't worry about it, I'll make one.

I won't be able to finish it this week, but it'll hopefully be finished early next week!

@saghul
Copy link
Author

saghul commented Aug 9, 2019

qjs.wasm.zip

There you go, cheers!

@MarkMcCaskey
Copy link
Contributor

Thanks! I got the repl working quite quickly by returning 0 bytes found/success. But I've been pretty stumped on how to implement this properly in a robust and cross-platform way.

Especially given that our new public API means that files can be backed by arbitrary Rust code. So we'll need some async functions in our WasiFile trait. I haven't been keeping up to date with Rust's new async progress though and ran into a ton of issues when trying things out earlier today.

I'll have to learn more about ioctl/poll and Rust async to properly do this I think

@saghul
Copy link
Author

saghul commented Aug 13, 2019

I might be talking out of my ass here, but anyway... :-) Maybe using select(2) or poll(2) would do, https://github.com/rust-lang/libc seems to expose them.

@MarkMcCaskey
Copy link
Contributor

@saghul Yeah, I think that might work, but we'll have to write a different version for Windows/NT and other operating systems. I think something like Tokio might be the solution we want.

I also got the impression that I can't just use poll, I have to use ioctl to get the data and buffer it myself or something.

Thanks! I think there's probably a path toward getting a subset of this properly working quickly, just need to work on it some more

@saghul
Copy link
Author

saghul commented Aug 14, 2019

@MarkMcCaskey Yes, that won't work on Windows. libc doesn't seem to wrap WSAPoll, but it's broken anyway.

Tokio seems to big of a dependency IMHO, just to get level-triggered I/O. mio's Poll seems to be closer to what you need: https://tokio-rs.github.io/mio/doc/mio/struct.Poll.html

As for ioctl, I don't think you'll need to use it directly. Hopefully mio has helpers to set fds non-blocking. As for getting the buffer, I assume you mean FIONREAD. In libuv we found it was not worth it, so you just read until you get EAGAIN.

FTR: wasmtime seems to implement this in C and the wrapping it in Rust. I can't see how it can work on Windows though. At any rate, I applaud you treating Windows as a first class citizen :-)

@MarkMcCaskey
Copy link
Contributor

@saghul Thanks! That article was extremely useful!

Hmm, it says OSX is even worse... that's concerning. I'll have to look into that too

Yeah, we want to avoid platform specific code if we can -- so far our WASI implementation has been fully portable except for some assumptions with files earlier on and accessing clocks.

Anyways, for now I ended up just implementing it for Unix using poll and ioctl and a bit of a nasty hack in the WasiFile trait to return an Option of the host system's file descriptor, which most tormentors won't be able to do, and the code that calls this just ignores anything that can't 🤷‍♂

I think it's the best I can do right now, but I'll keep thinking about it. I'm noticing some weird behavior where returning __EAGAIN as the error code doesn't seem to matter, the program tries to read immediately, but otherwise it looks like it's more or less good for files.

that's a good point about mio vs tokio, though we might eventually need something like tokio anyways, at least conditionally, for the "reactor" use case of WASI which acts like an async event-loop.

@saghul
Copy link
Author

saghul commented Aug 14, 2019

we might eventually need something like tokio anyways, at least conditionally, for the "reactor" use case of WASI which acts like an async event-loop.

Ah, true that!

bors bot added a commit that referenced this issue Aug 15, 2019
671: Add wasi::poll_oneoff for Unix r=MarkMcCaskey a=MarkMcCaskey

Part of #570

Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
@syrusakbary
Copy link
Member

This is now implemented in master (since wasmer 0.7.0). Closing the issue :)

@saghul
Copy link
Author

saghul commented Dec 11, 2019

Not quite:

wasmer --version
wasmer 0.11.0

wasmer run build/qjs.wasm
QuickJS - Type "\h" for help
qjs >
qjs > os.setTimeout(() => { console.log('hello'); }, 100);
os.setTimeout(() => { console.log('hello'); }, 100);
{  }
qjs > thread 'main' panicked at 'not yet implemented: Clock eventtypes in wasi::poll_oneoff', lib/wasi/src/syscalls/mod.rs:2354:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
Error: error: "not yet implemented: Clock eventtypes in wasi::poll_oneoff"

Can you please reopen?

@syrusakbary syrusakbary reopened this Dec 11, 2019
@syrusakbary
Copy link
Member

Good catch! Thanks

@Hywan
Copy link
Contributor

Hywan commented Jul 13, 2021

It now works :-). Closing the issue.

@Hywan Hywan added the 📦 lib-wasi About wasmer-wasi label Jul 13, 2021
@Hywan Hywan closed this as completed Jul 13, 2021
@syrusakbary
Copy link
Member

@Hywan it only works in Linux, not in Windows yet.
Can we check if there is a Windows related issue so we follow-up there?

@syrusakbary syrusakbary reopened this Jul 13, 2021
@Hywan Hywan changed the title Implement wasi::poll_oneoff Implement wasi::poll_oneoff on Windows Jul 15, 2021
@syrusakbary
Copy link
Member

syrusakbary commented Nov 23, 2021

The PR #2689 fixes this (superseded)

The PR #2887 fixes this

@syrusakbary syrusakbary added this to the v3.0 milestone Apr 27, 2022
@canewsin
Copy link

canewsin commented Nov 4, 2022

Is this issue solved? seems like mentioned PRs are merged.

@ptitSeb
Copy link
Contributor

ptitSeb commented Nov 16, 2022

I checked, and no, it's not really fixed. qjs.wasm load but freeze there, you cannot enter anything. I'll debug that.

@ptitSeb ptitSeb self-assigned this Nov 16, 2022
@ptitSeb
Copy link
Contributor

ptitSeb commented Nov 16, 2022

A rewrite / refactor of WASI is planed for the 3.2, so Assigning this ticket to this milestone.

@Michael-F-Bryan
Copy link
Contributor

This seems to be implemented already so I'm going to close the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! 📦 lib-wasi About wasmer-wasi 🖼️ platform-windows This issue happens on Windows priority-medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

7 participants