Minimal viable implementation of poll_oneoff for Windows

[marmistrz]

Again, I dislike the implementation, but it's the best what we can do without touching the implementation of other syscalls and a starting point for the discussion.
I'll be adding more comments and explanations in the coming days. I'll add a follow-up reply when I'm done with it.

The main blocker that forces us to busy-wait on stdin getting ready for input is the fact that the current implementation of fd_read (and similarly fd_write, but this is not significant) uses BufRead to read from stdin, which does internal buffering. The details have been described in a comment and will be omitted from this summary.

Still, libstd doing buffering on our behalf may be superfluous if not wasteful. Note that our fd_read is the rough counterpart of read from POSIX, which doesn't do any buffering and will most likely not be used directly. Most probably, the relevant standard library will do its buffering itself, be it stdio, iostream or Rust libstd, whichever is used by the wasm binary. Therefore, there should be no technical problem dropping buffering from wasi-common and it may even be desirable.

Even more, this may be a leftover bug, judging by a closed issue: #255. The use of BufRead is implied by this line:

wasmtime/crates/wasi-common/src/hostcalls_impl/fs.rs

Line 162 in 85cfdd0

Descriptor::Stdin => io::stdin().lock().read_vectored(&mut iovs),

Still, this brings another issue: if we rely on this behavior, we must not ever use BufRead on stdin throughout wasi-common, neither can it be used in any of the dependent crates. I don't think it's easy to be checked.

Possible solutions:

1. Leave the existing code as it is and do busy-loops.
2. Modify fd_read (and fd_write) not to make use of BufRead

---

I tried using WaitForSingleObject to detect new stdin events, but this function doesn't work for me as expected. (see wait.cpp.gz for the exact code I tested).

1. The timeout doesn't actually occur. With a timeout so low as 10ms I should be flooded with the messages saying Something else happened. On the contrary, I get absolutely no message until I actually write a line to the terminal
2. When I close the stdin using ctrl+D, WaitForSingleObject return immediately with WAIT_OBJECT_0, but clearly there's no data for getline to read. Moreover, the handle still seems valid: GetHandleInformation succeeds even after the stdin has been closed, so I don't know if we have a valid way of detecting such situation.

Please note that my tests were done over an SSH terminal from MSYS2 on Windows 10. I compiled the test program using g++ from mingw.

There are also other questions related to WaitForSingleObject. How will it react to "empty" key presses, i.e. those which don't produce any character, for instance arrow up/arrow down. I'd need to test it. We could possible ignore this issue, as select & poll on Linux may in some cases report spurious readiness notifications.

[marmistrz]

@kubkon @peterhuene Please wait a while with reviewing this change until I add some notes and state some ideas/concerns, so that we can meaningfully discuss it. I probably wasn't clear enough about that.

[peterhuene]

Sorry, just saw your comment now! I'll hold off on additional review feedback.

[marmistrz]

@kubkon @peterhuene please share your feedback guys! Thanks!

[peterhuene]

@marmistrz sorry this completely slipped off my radar with the holiday last week.

I'll review and provide feedback shortly.

[peterhuene]

@marmistrz this looks great, thanks for implementing this rather tricky API! I have some questions regarding the approach that I think we should address.

[kubkon]

All in all, this looks good to me especially since this is our first pass at implementing poll_oneoff on Windows.

While here, not sure how relevant it is now, but it seems that stdin is always buffered by Rust. See here for more details on the subject. This PR does mention that we should be able to use stdin_raw to access the raw, unbuffered stdin handle, but this method is kept private in libstd. @alexcrichton would you perhaps be able to chip in here?

Also, FWIW, it might be worth pointing out that the busy loop might disappear soon-ish when we start considering rewriting the syscalls in an async fashion. Although, I'm not sure we will ever want to support only the async version especially if determinism is a requirement for the client code.

[marmistrz]

This is ready for another round of reviews. The poll_oneoff test has also been enabled on Windows.

[peterhuene]

Looking good! I just have one nit-picky name question and a more important clarification question. Once answered, I think this is good to go.

[marmistrz]

I made sure that we don't wait on events if there are any available immediately (if -> else if). I also made sure that we don't do extra syscalls if we're requested to return immediately.

[marmistrz]

I rebased and got rid of the loop. Now we're just calling thread::park once.

It seems the tests are failing because of the snapshot stuff. Should I try to somehow port these changes into the snapshot, or what's the preferred way of coping with that?

[marmistrz]

I have just pushed a new minimal viable implementation of poll_oneoff for Windows. The following features are out of scope of this change (it's already gotten much too complicated) and will be addressed in subsequent PRs

• actual polling of pipes under Windows. There's PeekNamedPipe on Windows that can be used to achieve it.
• correct handling of empty poll. Current implementation on Unix depends on the fact that empty poll immediately succeeds and it's difficult to address this issue without changing the Unix impl, so we align with Unix for the time being.

I'm blocked by #760 for now. I need to either have the top-level error type implement Clone and I'll be happy with any of means of achieving it proposed in #760.

With the current test configuration it's impossible to test the real stdin implementation on CI, because in runtime.rs the stdin is swapped for a dummy pipe. This was tested manually today, it's enough to comment out this line:

wasmtime/crates/test-programs/tests/wasm_tests/runtime.rs

Line 36 in b2bfb98

builder = builder.stdin(reader_to_file(reader));

[sunfishcode]

Concerning the empty poll case, we've now merged WebAssembly/WASI#193 in the WASI spec, so while that isn't in a snapshot yet, it means we don't need to worry much about this case here.

[peterhuene]

This is looking good!

While I'm not thrilled to have to use a thread to implement this on Windows, you've called out the limitations quite clearly and I think we can live with them.

Just one more requested change regarding the EINVAL return and then 👍.

[peterhuene]

👍 Huge thanks for sticking through this difficult PR!

[marmistrz]

Is there anything that prevents this PR from being merged? It's ready as far as I'm concerned.

[peterhuene]

I agree it is ready. Merging now. Thanks again!