From 4695c95374e1f53c5d00ca4c89561046e36bdb58 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Sun, 10 Nov 2019 16:41:16 +0100 Subject: [PATCH 01/29] WIP implementation of poll_oneoff on Windows --- crates/test-programs/build.rs | 1 - .../src/sys/windows/hostcalls_impl/misc.rs | 152 +++++++++++++++++- 2 files changed, 150 insertions(+), 3 deletions(-) diff --git a/crates/test-programs/build.rs b/crates/test-programs/build.rs index 0f223b1c71..c7c9460623 100644 --- a/crates/test-programs/build.rs +++ b/crates/test-programs/build.rs @@ -176,7 +176,6 @@ mod wasi_tests { "dangling_symlink" => true, "symlink_loop" => true, "truncation_rights" => true, - "poll_oneoff" => true, "path_link" => true, "dangling_fd" => true, _ => false, diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index d781409093..eaf662fd8a 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -76,12 +76,160 @@ pub(crate) fn clock_time_get(clock_id: wasi::__wasi_clockid_t) -> Result bool { + use std::io::Read; + std::io::stdin().bytes().peekable().peek().is_some() +} + pub(crate) fn poll_oneoff( timeout: Option, fd_events: Vec, events: &mut Vec, -) -> Result> { - unimplemented!("poll_oneoff") +) -> Result<()> { + use crate::fdentry::Descriptor; + if fd_events.is_empty() && timeout.is_none() { + return Ok(()); + } + + // Currently WASI file support is only (a) regular files (b) directories (c) symlinks on Windows, + // which are always ready to write on Unix. + // + // We need to consider stdin/stdout/stderr separately. + // We treat stdout/stderr as always ready to write. I'm not sure if it's correct + // on Windows but I have not find any way of checking if a write to stdout would block. + // Therefore, we only poll the stdin. + let mut stdin_events = vec![]; + let mut immediate_events = vec![]; + + for event in fd_events { + match event.descriptor { + Descriptor::Stdin if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ => { + stdin_events.push(event) + } + _ => immediate_events.push(event), + } + } + + // we have at least one immediate event, so we don't need to care about stdin + if immediate_events.len() > 0 { + for event in immediate_events { + let size = match event.descriptor { + Descriptor::OsHandle(os_handle) + if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ => + { + os_handle + .metadata() + .expect("FIXME return a proper error") + .len() + } + Descriptor::Stdin => panic!("Descriptor::Stdin should have been filtered out"), + // On Unix, ioctl(FIONREAD) will return 0 for stdout/stderr. Emulate the same behavior on Windows. + // + // Besides, the spec is unclear what nbytes should actually be for __WASI_EVENTTYPE_FD_WRITE and + // the implementation on Unix just returns 0 here, so it's probably fine to do the same on Windows for now. + // cf. https://github.com/WebAssembly/WASI/issues/148 + _ => 0, + }; + + events.push(wasi::__wasi_event_t { + userdata: event.userdata, + r#type: event.r#type, + error: wasi::__WASI_ERRNO_SUCCESS, + u: wasi::__wasi_event_u_t { + fd_readwrite: wasi::__wasi_event_fd_readwrite_t { + nbytes: size, + flags: 0, + }, + }, + }) + } + } else { + assert_ne!(stdin_events.len(), 0, "stdin_events should not be empty"); + + // We are busy-polling the stdin with delay, unfortunately. + // + // We'd like to do the following: + // (1) wait in a non-blocking way for data to be available in stdin, with timeout + // (2) find out, how many bytes are there available to be read. + // For one, using `WaitForSingleObject` on the stdin handle could possibly be one way of + // achieving (1). + // I don't know of any way to achieve (2). + // + // While both of these are not as trivial on Windows as they are on Linux, there's a much + // more fundamental issue preventing us from achieving such behavior with the current + // implementation of wasi-common. + // + // Precisely, in `fd_read` we are using `io::stdin` via the `BufRead` trait, which does + // buffering on the libstd side. This means that even if there's still some unread data + // in stdin, the Windows system calls may return false negatives, indicating that stdin is empty. + // Therefore, avoiding the busy-poll here would require us to ditch libstd for the interaction + // with stdin altogether. + // + // However, polling stdin is a relatively infrequent use case, so this hopefully won't be + // a major issue. + let poll_interval = Duration::from_millis(10); + let poll_start = Instant::now(); + let timeout_duration = timeout + .map(|t| t.delay.try_into().map(Duration::from_nanos)) + .transpose()?; + let timeout = loop { + if let Some(timeout_duration) = timeout_duration { + if poll_start.elapsed() >= timeout_duration { + break timeout; + } + } + if stdin_nonempty() { + break None; + } + std::thread::sleep(poll_interval); + }; + + // TODO try refactoring pushing to events + match timeout { + // timeout occurred + Some(timeout) => { + for event in stdin_events { + events.push(wasi::__wasi_event_t { + userdata: timeout.userdata, + r#type: wasi::__WASI_EVENTTYPE_CLOCK, + error: wasi::__WASI_ERRNO_SUCCESS, + u: wasi::__wasi_event_u_t { + fd_readwrite: wasi::__wasi_event_fd_readwrite_t { + nbytes: 0, + flags: 0, + }, + }, + }); + } + } + // stdin is ready for reading + None => { + for event in stdin_events { + assert_eq!( + event.r#type, + wasi::__WASI_EVENTTYPE_FD_READ, + "stdin was expected to be polled for reading" + ); + events.push(wasi::__wasi_event_t { + userdata: event.userdata, + r#type: event.r#type, + error: wasi::__WASI_ERRNO_SUCCESS, + // Another limitation is that `std::io::BufRead` doesn't allow us + // to find out the number bytes available in the buffer, + // so we return the only universally correct lower bound. + u: wasi::__wasi_event_u_t { + fd_readwrite: wasi::__wasi_event_fd_readwrite_t { + nbytes: 1, + flags: 0, + }, + }, + }); + } + } + } + } + + Ok(()) } fn get_monotonic_time() -> Duration { From 98e84ae48774bd9f1e2d33fe8e7b8879cd4cfaa0 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Sun, 8 Dec 2019 20:37:14 +0100 Subject: [PATCH 02/29] Refactor poll_oneoff and return stdin if immediately readable. --- crates/wasi-common/src/error.rs | 16 ++ crates/wasi-common/src/lib.rs | 4 +- .../src/sys/windows/hostcalls_impl/misc.rs | 141 ++++++++++-------- 3 files changed, 95 insertions(+), 66 deletions(-) diff --git a/crates/wasi-common/src/error.rs b/crates/wasi-common/src/error.rs index 302d5d76bd..821ba0a476 100644 --- a/crates/wasi-common/src/error.rs +++ b/crates/wasi-common/src/error.rs @@ -158,6 +158,7 @@ impl Error { Self::Winx(err) => crate::sys::host_impl::errno_from_win(*err), } } + pub const ESUCCESS: Self = Error::Wasi(WasiError::ESUCCESS); pub const E2BIG: Self = Error::Wasi(WasiError::E2BIG); pub const EACCES: Self = Error::Wasi(WasiError::EACCES); @@ -246,3 +247,18 @@ fn errno_from_ioerror(e: &std::io::Error) -> wasi::__wasi_errno_t { } } } + +pub(crate) type Result = std::result::Result; + +pub(crate) trait WasiErrno { + fn as_wasi_errno(&self) -> wasi::__wasi_errno_t; +} + +impl WasiErrno for Result { + fn as_wasi_errno(&self) -> wasi::__wasi_errno_t { + self.as_ref() + .err() + .unwrap_or(&Error::ESUCCESS) + .as_wasi_errno() + } +} diff --git a/crates/wasi-common/src/lib.rs b/crates/wasi-common/src/lib.rs index 3834f253d0..bc4fcab6e7 100644 --- a/crates/wasi-common/src/lib.rs +++ b/crates/wasi-common/src/lib.rs @@ -41,5 +41,5 @@ pub mod wasi32; pub use ctx::{WasiCtx, WasiCtxBuilder}; pub use sys::preopen_dir; -pub type Error = error::Error; -pub(crate) type Result = std::result::Result; +pub use error::Error; +pub(crate) use error::Result; diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index eaf662fd8a..c6f70fb03a 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -81,12 +81,41 @@ fn stdin_nonempty() -> bool { std::io::stdin().bytes().peekable().peek().is_some() } +fn make_read_event(event: &FdEventData, nbytes: Result) -> wasi::__wasi_event_t { + use crate::error::WasiErrno; + let error = nbytes.as_wasi_errno(); + let nbytes = nbytes.unwrap_or_default(); + wasi::__wasi_event_t { + userdata: event.userdata, + r#type: event.r#type, + error, + u: wasi::__wasi_event_u_t { + fd_readwrite: wasi::__wasi_event_fd_readwrite_t { nbytes, flags: 0 }, + }, + } +} + +fn make_timeout_event(event: &FdEventData, timeout: &ClockEventData) -> wasi::__wasi_event_t { + wasi::__wasi_event_t { + userdata: timeout.userdata, + r#type: wasi::__WASI_EVENTTYPE_CLOCK, + error: wasi::__WASI_ERRNO_SUCCESS, + u: wasi::__wasi_event_u_t { + fd_readwrite: wasi::__wasi_event_fd_readwrite_t { + nbytes: 0, + flags: 0, + }, + }, + } +} + pub(crate) fn poll_oneoff( timeout: Option, fd_events: Vec, events: &mut Vec, ) -> Result<()> { use crate::fdentry::Descriptor; + use std::fs::Metadata; if fd_events.is_empty() && timeout.is_none() { return Ok(()); } @@ -100,52 +129,51 @@ pub(crate) fn poll_oneoff( // Therefore, we only poll the stdin. let mut stdin_events = vec![]; let mut immediate_events = vec![]; + let mut stdin_ready = None; for event in fd_events { match event.descriptor { Descriptor::Stdin if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ => { - stdin_events.push(event) + // Cache the non-emptiness for better performance. + let immediate = stdin_ready.get_or_insert_with(stdin_nonempty); + if *immediate { + immediate_events.push(event) + } else { + stdin_events.push(event) + } } _ => immediate_events.push(event), } } - // we have at least one immediate event, so we don't need to care about stdin - if immediate_events.len() > 0 { - for event in immediate_events { + // Process all the events that do not require waiting. + if !immediate_events.is_empty() { + for mut event in immediate_events { let size = match event.descriptor { - Descriptor::OsHandle(os_handle) - if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ => - { - os_handle - .metadata() - .expect("FIXME return a proper error") - .len() + Descriptor::OsHandle(os_handle) => { + if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ { + os_handle.metadata().map(|m| m.len()).map_err(Into::into) + } else { + // The spec is unclear what nbytes should actually be for __WASI_EVENTTYPE_FD_WRITE and + // the implementation on Unix just returns 0 here, so it's probably fine + // to do the same on Windows for now. + // cf. https://github.com/WebAssembly/WASI/issues/148 + Ok(0) + } } - Descriptor::Stdin => panic!("Descriptor::Stdin should have been filtered out"), + // We return the only universally correct lower bound, see the comment later in the function. + Descriptor::Stdin => Ok(1), // On Unix, ioctl(FIONREAD) will return 0 for stdout/stderr. Emulate the same behavior on Windows. - // - // Besides, the spec is unclear what nbytes should actually be for __WASI_EVENTTYPE_FD_WRITE and - // the implementation on Unix just returns 0 here, so it's probably fine to do the same on Windows for now. - // cf. https://github.com/WebAssembly/WASI/issues/148 - _ => 0, + Descriptor::Stdout | Descriptor::Stderr => Ok(0), }; - events.push(wasi::__wasi_event_t { - userdata: event.userdata, - r#type: event.r#type, - error: wasi::__WASI_ERRNO_SUCCESS, - u: wasi::__wasi_event_u_t { - fd_readwrite: wasi::__wasi_event_fd_readwrite_t { - nbytes: size, - flags: 0, - }, - }, - }) + let new_event = make_read_event(&event, size); + events.push(new_event) } - } else { - assert_ne!(stdin_events.len(), 0, "stdin_events should not be empty"); + } + // There are some stdin poll requests and there's no data available immediately + if !stdin_events.is_empty() { // We are busy-polling the stdin with delay, unfortunately. // // We'd like to do the following: @@ -172,58 +200,43 @@ pub(crate) fn poll_oneoff( let timeout_duration = timeout .map(|t| t.delay.try_into().map(Duration::from_nanos)) .transpose()?; - let timeout = loop { + + let timeout_occurred: Option = loop { + // Even though we assume that stdin is not ready, it's better to check it + // sooner than later, as we're going to wait anyway if it's the case. + if stdin_nonempty() { + break None; + } if let Some(timeout_duration) = timeout_duration { if poll_start.elapsed() >= timeout_duration { break timeout; } } - if stdin_nonempty() { - break None; - } std::thread::sleep(poll_interval); }; - // TODO try refactoring pushing to events - match timeout { - // timeout occurred - Some(timeout) => { + match timeout_occurred { + Some(timeout_info) => { for event in stdin_events { - events.push(wasi::__wasi_event_t { - userdata: timeout.userdata, - r#type: wasi::__WASI_EVENTTYPE_CLOCK, - error: wasi::__WASI_ERRNO_SUCCESS, - u: wasi::__wasi_event_u_t { - fd_readwrite: wasi::__wasi_event_fd_readwrite_t { - nbytes: 0, - flags: 0, - }, - }, - }); + let new_event = make_timeout_event(&event, &timeout_info); + events.push(new_event); } } - // stdin is ready for reading None => { + // stdin became ready for reading for event in stdin_events { assert_eq!( event.r#type, wasi::__WASI_EVENTTYPE_FD_READ, "stdin was expected to be polled for reading" ); - events.push(wasi::__wasi_event_t { - userdata: event.userdata, - r#type: event.r#type, - error: wasi::__WASI_ERRNO_SUCCESS, - // Another limitation is that `std::io::BufRead` doesn't allow us - // to find out the number bytes available in the buffer, - // so we return the only universally correct lower bound. - u: wasi::__wasi_event_u_t { - fd_readwrite: wasi::__wasi_event_fd_readwrite_t { - nbytes: 1, - flags: 0, - }, - }, - }); + + // Another limitation is that `std::io::BufRead` doesn't allow us + // to find out the number bytes available in the buffer, + // so we return the only universally correct lower bound, + // which is 1 byte. + let new_event = make_read_event(&event, Ok(1)); + events.push(new_event); } } } From 40ec01a1e869e79fe59d8c88c956a4f394b8ccc8 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Mon, 9 Dec 2019 19:44:05 +0100 Subject: [PATCH 03/29] Fix poll_oneoff behavior when fd_events are empty --- .../src/sys/windows/hostcalls_impl/misc.rs | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index c6f70fb03a..3276045a4b 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -116,8 +116,26 @@ pub(crate) fn poll_oneoff( ) -> Result<()> { use crate::fdentry::Descriptor; use std::fs::Metadata; - if fd_events.is_empty() && timeout.is_none() { - return Ok(()); + use std::thread; + + let timeout_duration = timeout + .map(|t| t.delay.try_into().map(Duration::from_nanos)) + .transpose()?; + + // With no events to listen, poll_oneoff just becomes a sleep. + if fd_events.is_empty() { + match timeout_duration { + Some(t) => { + thread::sleep(t); + return Ok(()); + } + None => { + // The thread is not guanteed to remain parked forever, so we need to loop + loop { + thread::park(); + } + } + } } // Currently WASI file support is only (a) regular files (b) directories (c) symlinks on Windows, @@ -197,9 +215,6 @@ pub(crate) fn poll_oneoff( // a major issue. let poll_interval = Duration::from_millis(10); let poll_start = Instant::now(); - let timeout_duration = timeout - .map(|t| t.delay.try_into().map(Duration::from_nanos)) - .transpose()?; let timeout_occurred: Option = loop { // Even though we assume that stdin is not ready, it's better to check it @@ -212,7 +227,7 @@ pub(crate) fn poll_oneoff( break timeout; } } - std::thread::sleep(poll_interval); + thread::sleep(poll_interval); }; match timeout_occurred { From 7cb8137fae716f78abad14706d9cb464628a45d8 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Wed, 11 Dec 2019 19:24:25 +0100 Subject: [PATCH 04/29] Avoid issuing syscalls if we're requested to return immediately --- .../src/sys/windows/hostcalls_impl/misc.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 3276045a4b..4df00f4ea0 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -188,10 +188,9 @@ pub(crate) fn poll_oneoff( let new_event = make_read_event(&event, size); events.push(new_event) } - } + } else if !stdin_events.is_empty() { + // There are some stdin poll requests and there's no data available immediately - // There are some stdin poll requests and there's no data available immediately - if !stdin_events.is_empty() { // We are busy-polling the stdin with delay, unfortunately. // // We'd like to do the following: @@ -213,6 +212,15 @@ pub(crate) fn poll_oneoff( // // However, polling stdin is a relatively infrequent use case, so this hopefully won't be // a major issue. + let timeout_duration = timeout + .map(|t| t.delay.try_into().map(Duration::from_nanos)) + .transpose()?; + + // avoid issuing more syscalls if we're requested to return immediately + if timeout_duration == Some(Duration::from_nanos(0)) { + return Ok(()); + } + let poll_interval = Duration::from_millis(10); let poll_start = Instant::now(); From a8e9b1a0d5960a201fb4f6b7c3c3d0fccd0aeb5f Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Wed, 11 Dec 2019 19:34:17 +0100 Subject: [PATCH 05/29] Comment on infinite sleep --- crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 4df00f4ea0..b45e10e086 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -130,6 +130,7 @@ pub(crate) fn poll_oneoff( return Ok(()); } None => { + // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep // The thread is not guanteed to remain parked forever, so we need to loop loop { thread::park(); From 5cd3e9904f5ba05dde9992a08fdf333554a0b34d Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Wed, 11 Dec 2019 19:35:48 +0100 Subject: [PATCH 06/29] Rename make_read_event to make_rw_event --- crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index b45e10e086..b82054cdc6 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -81,7 +81,7 @@ fn stdin_nonempty() -> bool { std::io::stdin().bytes().peekable().peek().is_some() } -fn make_read_event(event: &FdEventData, nbytes: Result) -> wasi::__wasi_event_t { +fn make_rw_event(event: &FdEventData, nbytes: Result) -> wasi::__wasi_event_t { use crate::error::WasiErrno; let error = nbytes.as_wasi_errno(); let nbytes = nbytes.unwrap_or_default(); @@ -186,7 +186,7 @@ pub(crate) fn poll_oneoff( Descriptor::Stdout | Descriptor::Stderr => Ok(0), }; - let new_event = make_read_event(&event, size); + let new_event = make_rw_event(&event, size); events.push(new_event) } } else if !stdin_events.is_empty() { @@ -259,7 +259,7 @@ pub(crate) fn poll_oneoff( // to find out the number bytes available in the buffer, // so we return the only universally correct lower bound, // which is 1 byte. - let new_event = make_read_event(&event, Ok(1)); + let new_event = make_rw_event(&event, Ok(1)); events.push(new_event); } } From a2b556f1b0a48308c337dd0b4e57c4290d01001a Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Sun, 15 Dec 2019 19:27:12 +0100 Subject: [PATCH 07/29] Do not loop with nfds=0, timeout=-1 --- .../src/sys/windows/hostcalls_impl/misc.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index b82054cdc6..c8c8f34ed6 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -125,18 +125,13 @@ pub(crate) fn poll_oneoff( // With no events to listen, poll_oneoff just becomes a sleep. if fd_events.is_empty() { match timeout_duration { - Some(t) => { - thread::sleep(t); - return Ok(()); - } - None => { - // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep - // The thread is not guanteed to remain parked forever, so we need to loop - loop { - thread::park(); - } - } + Some(t) => thread::sleep(t), + // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep + // Even though the thread is not guanteed to remain parked forever, `poll(2)` + // mentions that spurious readiness notifications may occur, so it's probably fine + None => thread::park(), } + return Ok(()); } // Currently WASI file support is only (a) regular files (b) directories (c) symlinks on Windows, From 54a398ad69da917478c175767dd263de844f052d Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Wed, 8 Jan 2020 19:44:07 +0100 Subject: [PATCH 08/29] Fix writing timeout events. Check that we only return one timeout event. --- .../wasi-tests/src/bin/poll_oneoff.rs | 6 ++++++ .../src/sys/windows/hostcalls_impl/misc.rs | 15 +++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs b/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs index 1af4ed0575..ddfbce0df9 100644 --- a/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs +++ b/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs @@ -65,11 +65,17 @@ unsafe fn test_stdin_read() { r#type: wasi::EVENTTYPE_CLOCK, u: wasi::SubscriptionU { clock }, }, + // Make sure that timeout is returned only once even if there are multiple read events wasi::Subscription { userdata: 1, r#type: wasi::EVENTTYPE_FD_READ, u: wasi::SubscriptionU { fd_readwrite }, }, + wasi::Subscription { + userdata: 42, + r#type: wasi::EVENTTYPE_FD_READ, + u: wasi::SubscriptionU { fd_readwrite }, + }, ]; let out = poll_oneoff_impl(&r#in, 1); let event = &out[0]; diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index c8c8f34ed6..e23bc6507c 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -95,7 +95,7 @@ fn make_rw_event(event: &FdEventData, nbytes: Result) -> wasi::__wasi_event } } -fn make_timeout_event(event: &FdEventData, timeout: &ClockEventData) -> wasi::__wasi_event_t { +fn make_timeout_event(timeout: &ClockEventData) -> wasi::__wasi_event_t { wasi::__wasi_event_t { userdata: timeout.userdata, r#type: wasi::__WASI_EVENTTYPE_CLOCK, @@ -125,7 +125,12 @@ pub(crate) fn poll_oneoff( // With no events to listen, poll_oneoff just becomes a sleep. if fd_events.is_empty() { match timeout_duration { - Some(t) => thread::sleep(t), + Some(t) => { + thread::sleep(t); + let timeout_event = timeout.expect("timeout should be Some"); + let new_event = make_timeout_event(&timeout_event); + events.push(new_event); + } // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep // Even though the thread is not guanteed to remain parked forever, `poll(2)` // mentions that spurious readiness notifications may occur, so it's probably fine @@ -236,10 +241,8 @@ pub(crate) fn poll_oneoff( match timeout_occurred { Some(timeout_info) => { - for event in stdin_events { - let new_event = make_timeout_event(&event, &timeout_info); - events.push(new_event); - } + let new_event = make_timeout_event(&timeout_info); + events.push(new_event); } None => { // stdin became ready for reading From 0b50274680eea4a1553fc6119b30464bfb097454 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Wed, 8 Jan 2020 20:15:05 +0100 Subject: [PATCH 09/29] WIP --- .../wasi-tests/src/bin/poll_oneoff.rs | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs b/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs index ddfbce0df9..362ff95c94 100644 --- a/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs +++ b/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs @@ -33,10 +33,6 @@ unsafe fn test_timeout() { }]; let out = poll_oneoff_impl(&r#in, 1); let event = &out[0]; - assert_eq!( - event.userdata, CLOCK_ID, - "the event.userdata should contain clock_id specified by the user" - ); assert_eq!( event.error, wasi::ERRNO_SUCCESS, @@ -47,6 +43,10 @@ unsafe fn test_timeout() { wasi::EVENTTYPE_CLOCK, "the event.type should equal clock" ); + assert_eq!( + event.userdata, CLOCK_ID, + "the event.userdata should contain clock_id specified by the user" + ); } unsafe fn test_stdin_read() { @@ -71,18 +71,9 @@ unsafe fn test_stdin_read() { r#type: wasi::EVENTTYPE_FD_READ, u: wasi::SubscriptionU { fd_readwrite }, }, - wasi::Subscription { - userdata: 42, - r#type: wasi::EVENTTYPE_FD_READ, - u: wasi::SubscriptionU { fd_readwrite }, - }, ]; let out = poll_oneoff_impl(&r#in, 1); let event = &out[0]; - assert_eq!( - event.userdata, CLOCK_ID, - "the event.userdata should contain clock_id specified by the user" - ); assert_eq!( event.error, wasi::ERRNO_SUCCESS, @@ -93,6 +84,10 @@ unsafe fn test_stdin_read() { wasi::EVENTTYPE_CLOCK, "the event.type should equal clock" ); + assert_eq!( + event.userdata, CLOCK_ID, + "the event.userdata should contain clock_id specified by the user" + ); } unsafe fn test_stdout_stderr_write() { From f20b5a4cacf0f53843509f3e552cce7b3be896fb Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 9 Jan 2020 14:45:36 +0100 Subject: [PATCH 10/29] WIP --- crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index e23bc6507c..4de7f88165 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -167,6 +167,7 @@ pub(crate) fn poll_oneoff( // Process all the events that do not require waiting. if !immediate_events.is_empty() { + trace!(" | have immediate events, will return immediately"); for mut event in immediate_events { let size = match event.descriptor { Descriptor::OsHandle(os_handle) => { @@ -190,6 +191,7 @@ pub(crate) fn poll_oneoff( events.push(new_event) } } else if !stdin_events.is_empty() { + trace!(" | actively polling stdin"); // There are some stdin poll requests and there's no data available immediately // We are busy-polling the stdin with delay, unfortunately. From 9197a68837c0e4a4dcfdd96dbfbc6c053918f11d Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 9 Jan 2020 14:51:35 +0100 Subject: [PATCH 11/29] WIP --- crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 4de7f88165..b45fb638f3 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -7,6 +7,7 @@ use crate::sys::host_impl; use crate::{wasi, wasi32, Error, Result}; use cpu_time::{ProcessTime, ThreadTime}; use lazy_static::lazy_static; +use log::trace; use std::convert::TryInto; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; From 432dbf0e74c4bdf3b7dd9f3ac00ca9a1172c3d75 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Tue, 14 Jan 2020 16:33:35 +0100 Subject: [PATCH 12/29] More WIP --- crates/wasi-common/Cargo.toml | 1 + .../src/sys/windows/hostcalls_impl/misc.rs | 260 ++++++++++++------ 2 files changed, 175 insertions(+), 86 deletions(-) diff --git a/crates/wasi-common/Cargo.toml b/crates/wasi-common/Cargo.toml index f9d0f08ec2..370e615397 100644 --- a/crates/wasi-common/Cargo.toml +++ b/crates/wasi-common/Cargo.toml @@ -22,6 +22,7 @@ filetime = "0.2.7" lazy_static = "1.4.0" num = { version = "0.2.0", default-features = false } wig = { path = "wig" } +crossbeam = "0.7.3" [target.'cfg(unix)'.dependencies] yanix = { path = "yanix" } diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index b45fb638f3..dc58d6de3d 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -1,19 +1,79 @@ #![allow(non_camel_case_types)] #![allow(unused_unsafe)] #![allow(unused)] +use crate::fdentry::Descriptor; use crate::hostcalls_impl::{ClockEventData, FdEventData}; use crate::memory::*; use crate::sys::host_impl; use crate::{wasi, wasi32, Error, Result}; use cpu_time::{ProcessTime, ThreadTime}; +use crossbeam::channel::{self, Receiver, Sender}; use lazy_static::lazy_static; -use log::trace; +use log::{error, trace}; use std::convert::TryInto; +use std::io; +use std::os::windows::io::AsRawHandle; +use std::thread; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; +type StdinPayload = io::Result; +struct StdinPoll { + request_tx: Sender<()>, + notify_rx: Receiver, +} + +enum PollState { + Ready, + Closed, + TimedOut, + Error(Error), +} + +impl StdinPoll { + fn poll(&self, timeout: Option) -> PollState { + use crossbeam::channel::{RecvTimeoutError, TryRecvError}; + // Clean up possible unread result from previous poll + match self.notify_rx.try_recv() { + Ok(_) | Err(TryRecvError::Empty) => {} + Err(TryRecvError::Disconnected) => panic!("FIXME"), + } + self.request_tx.send(()).expect("FIXME"); + let pollret = match timeout { + Some(timeout) => self.notify_rx.recv_timeout(timeout), + None => Ok(self.notify_rx.recv().expect("FIXME")), + }; + match pollret { + Ok(Ok(true)) => PollState::Ready, + Ok(Ok(false)) => PollState::Closed, + Ok(Err(e)) => PollState::Error(e.into()), + Err(RecvTimeoutError::Timeout) => PollState::TimedOut, + Err(RecvTimeoutError::Disconnected) => panic!("FIXME"), + } + } + + fn event_loop(request_rx: Receiver<()>, notify_tx: Sender) -> ! { + use std::io::BufRead; + loop { + request_rx.recv().expect("FIXME"); + let buf = std::io::stdin().lock().fill_buf().map(|s| !s.is_empty()); + notify_tx.send(buf).expect("FIXME"); + } + } +} + lazy_static! { static ref START_MONOTONIC: Instant = Instant::now(); static ref PERF_COUNTER_RES: u64 = get_perf_counter_resolution_ns(); + static ref STDIN_POLL: StdinPoll = { + let channel_size = 1; + let (request_tx, request_rx) = channel::bounded(channel_size); + let (notify_tx, notify_rx) = channel::bounded(channel_size); + thread::spawn(move || StdinPoll::event_loop(request_rx, notify_tx)); + StdinPoll { + request_tx, + notify_rx, + } + }; } // Timer resolution on Windows is really hard. We may consider exposing the resolution of the respective @@ -110,34 +170,80 @@ fn make_timeout_event(timeout: &ClockEventData) -> wasi::__wasi_event_t { } } +fn handle_timeout( + timeout_event: ClockEventData, + timeout: Duration, + events: &mut Vec, +) { + thread::sleep(timeout); + handle_timeout_event(timeout_event, events); +} + +fn handle_timeout_event(timeout_event: ClockEventData, events: &mut Vec) { + let new_event = make_timeout_event(&timeout_event); + events.push(new_event); +} + +fn handle_rw_event(event: FdEventData, out_events: &mut Vec) { + let size = match event.descriptor { + Descriptor::OsHandle(os_handle) => { + if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ { + os_handle.metadata().map(|m| m.len()).map_err(Into::into) + } else { + // The spec is unclear what nbytes should actually be for __WASI_EVENTTYPE_FD_WRITE and + // the implementation on Unix just returns 0 here, so it's probably fine + // to do the same on Windows for now. + // cf. https://github.com/WebAssembly/WASI/issues/148 + Ok(0) + } + } + // We return the only universally correct lower bound, see the comment later in the function. + Descriptor::Stdin => Ok(1), + // On Unix, ioctl(FIONREAD) will return 0 for stdout/stderr. Emulate the same behavior on Windows. + Descriptor::Stdout | Descriptor::Stderr => Ok(0), + }; + + let new_event = make_rw_event(&event, size); + out_events.push(new_event); +} + +fn handle_error_event( + event: FdEventData, + error: Error, + out_events: &mut Vec, +) { + let new_event = make_rw_event(&event, Err(error)); + out_events.push(new_event); +} + pub(crate) fn poll_oneoff( timeout: Option, fd_events: Vec, events: &mut Vec, ) -> Result<()> { - use crate::fdentry::Descriptor; use std::fs::Metadata; use std::thread; - let timeout_duration = timeout - .map(|t| t.delay.try_into().map(Duration::from_nanos)) + let timeout = timeout + .map(|event| { + event + .delay + .try_into() + .map(Duration::from_nanos) + .map(|dur| (event, dur)) + }) .transpose()?; // With no events to listen, poll_oneoff just becomes a sleep. if fd_events.is_empty() { - match timeout_duration { - Some(t) => { - thread::sleep(t); - let timeout_event = timeout.expect("timeout should be Some"); - let new_event = make_timeout_event(&timeout_event); - events.push(new_event); - } - // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep - // Even though the thread is not guanteed to remain parked forever, `poll(2)` - // mentions that spurious readiness notifications may occur, so it's probably fine - None => thread::park(), + match timeout { + Some((event, dur)) => return Ok(handle_timeout(event, dur, events)), + // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep on Unix + // usually meant to be interrupted by a signal. Unfortunately, WASI doesn't currently + // support signals and there is no way to interrupt this infinite sleep, so we + // return `ENOTSUP` + None => return Err(Error::ENOTSUP), } - return Ok(()); } // Currently WASI file support is only (a) regular files (b) directories (c) symlinks on Windows, @@ -149,6 +255,7 @@ pub(crate) fn poll_oneoff( // Therefore, we only poll the stdin. let mut stdin_events = vec![]; let mut immediate_events = vec![]; + let mut pipe_events = vec![]; let mut stdin_ready = None; for event in fd_events { @@ -162,7 +269,22 @@ pub(crate) fn poll_oneoff( stdin_events.push(event) } } - _ => immediate_events.push(event), + Descriptor::Stdin | Descriptor::Stderr | Descriptor::Stdout => { + immediate_events.push(event) + } + Descriptor::OsHandle(os_handle) => { + let ftype = unsafe { winx::file::get_file_type(os_handle.as_raw_handle()) }?; + if ftype.is_unknown() || ftype.is_char() { + error!("poll_oneoff: unsupported file type: {:?}", ftype); + handle_error_event(event, Error::ENOTSUP, events); + } else if ftype.is_disk() { + immediate_events.push(event); + } else if ftype.is_pipe() { + pipe_events.push(event); + } else { + unreachable!(); + } + } } } @@ -170,30 +292,43 @@ pub(crate) fn poll_oneoff( if !immediate_events.is_empty() { trace!(" | have immediate events, will return immediately"); for mut event in immediate_events { - let size = match event.descriptor { - Descriptor::OsHandle(os_handle) => { - if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ { - os_handle.metadata().map(|m| m.len()).map_err(Into::into) - } else { - // The spec is unclear what nbytes should actually be for __WASI_EVENTTYPE_FD_WRITE and - // the implementation on Unix just returns 0 here, so it's probably fine - // to do the same on Windows for now. - // cf. https://github.com/WebAssembly/WASI/issues/148 - Ok(0) - } - } - // We return the only universally correct lower bound, see the comment later in the function. - Descriptor::Stdin => Ok(1), - // On Unix, ioctl(FIONREAD) will return 0 for stdout/stderr. Emulate the same behavior on Windows. - Descriptor::Stdout | Descriptor::Stderr => Ok(0), - }; - - let new_event = make_rw_event(&event, size); - events.push(new_event) + handle_rw_event(event, events); } } else if !stdin_events.is_empty() { - trace!(" | actively polling stdin"); - // There are some stdin poll requests and there's no data available immediately + // REVIEW: is there a better place to document this? Perhaps in + // `struct PollStdin`? + // + // If there's a request to poll stdin, we spin up a separate thread to + // waiting for data to arrive on stdin. This thread will not terminate. + // + // TODO more explain why this way + trace!(" | passively waiting on stdin"); + let dur = timeout.map(|t| t.1); + let state = STDIN_POLL.poll(dur); + for event in stdin_events { + match state { + PollState::Ready => handle_rw_event(event, events), + PollState::Closed => { /* error? FIXME */ } + PollState::TimedOut => { /* FIXME */ } + PollState::Error(ref e) => { + handle_error_event(event, Error::ENOTSUP /*FIXME*/, events); + } + } + } + } else if !pipe_events.is_empty() { + trace!(" | actively polling stdin or pipes"); + match timeout { + Some((event, dur)) => { + error!("Polling pipes not supported on Windows, will just time out."); + return Ok(handle_timeout(event, dur, events)); + } + None => { + error!("Polling only pipes with no timeout not supported on Windows."); + return Err(Error::ENOTSUP); + } + } + // TODO remove these old comments!!! + // There are some stdin or pipe poll requests and there's no data available immediately // We are busy-polling the stdin with delay, unfortunately. // @@ -216,55 +351,8 @@ pub(crate) fn poll_oneoff( // // However, polling stdin is a relatively infrequent use case, so this hopefully won't be // a major issue. - let timeout_duration = timeout - .map(|t| t.delay.try_into().map(Duration::from_nanos)) - .transpose()?; // avoid issuing more syscalls if we're requested to return immediately - if timeout_duration == Some(Duration::from_nanos(0)) { - return Ok(()); - } - - let poll_interval = Duration::from_millis(10); - let poll_start = Instant::now(); - - let timeout_occurred: Option = loop { - // Even though we assume that stdin is not ready, it's better to check it - // sooner than later, as we're going to wait anyway if it's the case. - if stdin_nonempty() { - break None; - } - if let Some(timeout_duration) = timeout_duration { - if poll_start.elapsed() >= timeout_duration { - break timeout; - } - } - thread::sleep(poll_interval); - }; - - match timeout_occurred { - Some(timeout_info) => { - let new_event = make_timeout_event(&timeout_info); - events.push(new_event); - } - None => { - // stdin became ready for reading - for event in stdin_events { - assert_eq!( - event.r#type, - wasi::__WASI_EVENTTYPE_FD_READ, - "stdin was expected to be polled for reading" - ); - - // Another limitation is that `std::io::BufRead` doesn't allow us - // to find out the number bytes available in the buffer, - // so we return the only universally correct lower bound, - // which is 1 byte. - let new_event = make_rw_event(&event, Ok(1)); - events.push(new_event); - } - } - } } Ok(()) From d31242c2e28de49611ef618f3b0444179271652a Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Tue, 14 Jan 2020 16:41:05 +0100 Subject: [PATCH 13/29] Update Cargo.lock --- Cargo.lock | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index d8b036cd0f..a79bb8bb96 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -429,6 +429,29 @@ dependencies = [ "wasmparser 0.45.2", ] +[[package]] +name = "crossbeam" +version = "0.7.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "69323bff1fb41c635347b8ead484a5ca6c3f11914d784170b158d8449ab07f8e" +dependencies = [ + "cfg-if", + "crossbeam-channel", + "crossbeam-deque", + "crossbeam-epoch", + "crossbeam-queue", + "crossbeam-utils 0.7.0", +] + +[[package]] +name = "crossbeam-channel" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "acec9a3b0b3559f15aee4f90746c4e5e293b701c0f7d3925d24e01645267b68c" +dependencies = [ + "crossbeam-utils 0.7.0", +] + [[package]] name = "crossbeam-deque" version = "0.7.2" @@ -1916,6 +1939,7 @@ dependencies = [ "anyhow", "cfg-if", "cpu-time", + "crossbeam", "filetime", "getrandom", "lazy_static", From 748894a121778e7e6fd97a86d9f57aac41c90907 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 13:20:24 +0100 Subject: [PATCH 14/29] wip --- .../wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index dc58d6de3d..0b0d3cb4b5 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -9,7 +9,7 @@ use crate::{wasi, wasi32, Error, Result}; use cpu_time::{ProcessTime, ThreadTime}; use crossbeam::channel::{self, Receiver, Sender}; use lazy_static::lazy_static; -use log::{error, trace}; +use log::{error, trace, warning}; use std::convert::TryInto; use std::io; use std::os::windows::io::AsRawHandle; @@ -242,7 +242,10 @@ pub(crate) fn poll_oneoff( // usually meant to be interrupted by a signal. Unfortunately, WASI doesn't currently // support signals and there is no way to interrupt this infinite sleep, so we // return `ENOTSUP` - None => return Err(Error::ENOTSUP), + None => { + error!("poll_oneoff: invoking with neither timeout nor fds not supported"); + return Err(Error::ENOTSUP); + } } } @@ -311,6 +314,7 @@ pub(crate) fn poll_oneoff( PollState::Closed => { /* error? FIXME */ } PollState::TimedOut => { /* FIXME */ } PollState::Error(ref e) => { + error!("PollState error"); handle_error_event(event, Error::ENOTSUP /*FIXME*/, events); } } @@ -319,7 +323,7 @@ pub(crate) fn poll_oneoff( trace!(" | actively polling stdin or pipes"); match timeout { Some((event, dur)) => { - error!("Polling pipes not supported on Windows, will just time out."); + warning!("Polling pipes not supported on Windows, will just time out."); return Ok(handle_timeout(event, dur, events)); } None => { From 8e8826d19f4157309f0f0e580649e5d8b6a002e2 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 13:22:33 +0100 Subject: [PATCH 15/29] wip --- crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 0b0d3cb4b5..005f6d66d9 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -9,7 +9,7 @@ use crate::{wasi, wasi32, Error, Result}; use cpu_time::{ProcessTime, ThreadTime}; use crossbeam::channel::{self, Receiver, Sender}; use lazy_static::lazy_static; -use log::{error, trace, warning}; +use log::{error, trace, warn}; use std::convert::TryInto; use std::io; use std::os::windows::io::AsRawHandle; @@ -323,7 +323,7 @@ pub(crate) fn poll_oneoff( trace!(" | actively polling stdin or pipes"); match timeout { Some((event, dur)) => { - warning!("Polling pipes not supported on Windows, will just time out."); + warn!("Polling pipes not supported on Windows, will just time out."); return Ok(handle_timeout(event, dur, events)); } None => { From 3261626fd8127680e05242ca16989e22a68696f6 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 13:29:33 +0100 Subject: [PATCH 16/29] wip --- crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs | 6 ++++++ crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs index 34135f59de..cff4ea56cb 100644 --- a/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs @@ -60,6 +60,12 @@ pub(crate) fn poll_oneoff( Errno, }; + trace!( + "poll_oneoff_impl: timeout={:?}, fd_events={:?}", + timeout, + fd_events + ); + if fd_events.is_empty() && timeout.is_none() { return Ok(()); } diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 005f6d66d9..0b9a26a60a 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -224,6 +224,12 @@ pub(crate) fn poll_oneoff( use std::fs::Metadata; use std::thread; + trace!( + "poll_oneoff_impl: timeout={:?}, fd_events={:?}", + timeout, + fd_events + ); + let timeout = timeout .map(|event| { event From 33818ea18ece104838ec0a7e73eb52a58d936cd2 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 14:59:18 +0100 Subject: [PATCH 17/29] Align with Unix --- .../wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 0b9a26a60a..d949f982bc 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -247,10 +247,14 @@ pub(crate) fn poll_oneoff( // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep on Unix // usually meant to be interrupted by a signal. Unfortunately, WASI doesn't currently // support signals and there is no way to interrupt this infinite sleep, so we - // return `ENOTSUP` + // intend to return `ENOTSUP`. + // + // Unfortunately, current implementation of poll_oneoff on Unix relies on + // `poll_oneoff` returning `Ok(())` in such case, so we emulate this behavior for now + // and will address it in a subsequent PR. None => { - error!("poll_oneoff: invoking with neither timeout nor fds not supported"); - return Err(Error::ENOTSUP); + // error!("poll_oneoff: invoking with neither timeout nor fds not supported"); + return Ok(()); // Err(Error::ENOTSUP); } } } From 410777de52e29578fa4a1750a87c4c4be3fa8874 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 15:09:36 +0100 Subject: [PATCH 18/29] Handle timeout --- .../src/sys/windows/hostcalls_impl/misc.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index d949f982bc..cbef97794f 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -269,18 +269,11 @@ pub(crate) fn poll_oneoff( let mut stdin_events = vec![]; let mut immediate_events = vec![]; let mut pipe_events = vec![]; - let mut stdin_ready = None; for event in fd_events { match event.descriptor { Descriptor::Stdin if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ => { - // Cache the non-emptiness for better performance. - let immediate = stdin_ready.get_or_insert_with(stdin_nonempty); - if *immediate { - immediate_events.push(event) - } else { - stdin_events.push(event) - } + stdin_events.push(event) } Descriptor::Stdin | Descriptor::Stderr | Descriptor::Stdout => { immediate_events.push(event) @@ -322,7 +315,7 @@ pub(crate) fn poll_oneoff( match state { PollState::Ready => handle_rw_event(event, events), PollState::Closed => { /* error? FIXME */ } - PollState::TimedOut => { /* FIXME */ } + PollState::TimedOut => handle_timeout_event(timeout.unwrap().0, events), PollState::Error(ref e) => { error!("PollState error"); handle_error_event(event, Error::ENOTSUP /*FIXME*/, events); From 5b9272f2a68fd38f49fa188a79cf6e7d42594d7f Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 15:23:08 +0100 Subject: [PATCH 19/29] fix build --- crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs | 6 ------ crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 6 ------ 2 files changed, 12 deletions(-) diff --git a/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs index cff4ea56cb..34135f59de 100644 --- a/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/unix/hostcalls_impl/misc.rs @@ -60,12 +60,6 @@ pub(crate) fn poll_oneoff( Errno, }; - trace!( - "poll_oneoff_impl: timeout={:?}, fd_events={:?}", - timeout, - fd_events - ); - if fd_events.is_empty() && timeout.is_none() { return Ok(()); } diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index cbef97794f..0f293fa9e7 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -224,12 +224,6 @@ pub(crate) fn poll_oneoff( use std::fs::Metadata; use std::thread; - trace!( - "poll_oneoff_impl: timeout={:?}, fd_events={:?}", - timeout, - fd_events - ); - let timeout = timeout .map(|event| { event From 3c132d6909bcebb9996da74793721458821dc32f Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 17:54:12 +0100 Subject: [PATCH 20/29] Improve comments --- .../src/sys/windows/hostcalls_impl/misc.rs | 78 ++++++++----------- 1 file changed, 32 insertions(+), 46 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 0f293fa9e7..cdb3e6a35a 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -137,11 +137,6 @@ pub(crate) fn clock_time_get(clock_id: wasi::__wasi_clockid_t) -> Result bool { - use std::io::Read; - std::io::stdin().bytes().peekable().peek().is_some() -} - fn make_rw_event(event: &FdEventData, nbytes: Result) -> wasi::__wasi_event_t { use crate::error::WasiErrno; let error = nbytes.as_wasi_errno(); @@ -253,13 +248,6 @@ pub(crate) fn poll_oneoff( } } - // Currently WASI file support is only (a) regular files (b) directories (c) symlinks on Windows, - // which are always ready to write on Unix. - // - // We need to consider stdin/stdout/stderr separately. - // We treat stdout/stderr as always ready to write. I'm not sure if it's correct - // on Windows but I have not find any way of checking if a write to stdout would block. - // Therefore, we only poll the stdin. let mut stdin_events = vec![]; let mut immediate_events = vec![]; let mut pipe_events = vec![]; @@ -269,13 +257,18 @@ pub(crate) fn poll_oneoff( Descriptor::Stdin if event.r#type == wasi::__WASI_EVENTTYPE_FD_READ => { stdin_events.push(event) } + // stdout/stderr are always considered ready to write because there seems to + // be no way of checking if a write to stdout would block. + // + // If stdin is polled for anything else then reading, then it is also + // considered immediately ready, following the behavior on Linux. Descriptor::Stdin | Descriptor::Stderr | Descriptor::Stdout => { immediate_events.push(event) } Descriptor::OsHandle(os_handle) => { let ftype = unsafe { winx::file::get_file_type(os_handle.as_raw_handle()) }?; if ftype.is_unknown() || ftype.is_char() { - error!("poll_oneoff: unsupported file type: {:?}", ftype); + debug!("poll_oneoff: unsupported file type: {:?}", ftype); handle_error_event(event, Error::ENOTSUP, events); } else if ftype.is_disk() { immediate_events.push(event); @@ -295,13 +288,28 @@ pub(crate) fn poll_oneoff( handle_rw_event(event, events); } } else if !stdin_events.is_empty() { - // REVIEW: is there a better place to document this? Perhaps in - // `struct PollStdin`? - // - // If there's a request to poll stdin, we spin up a separate thread to + // During the firt request to poll stdin, we spin up a separate thread to // waiting for data to arrive on stdin. This thread will not terminate. // - // TODO more explain why this way + // FIXME this is not thread safe! + // + // We'd like to do the following: + // (1) wait in a non-blocking way for data to be available in stdin, with timeout + // (2) find out, how many bytes are there available to be read. + // + // One issue is that we are currently relying on the Rust libstd for interaction + // with stdin. More precisely, `io::stdin` is used via the `BufRead` trait, + // in the `fd_read` function, which always does buffering on the libstd side. [1] + // This means that even if there's still some unread data in stdin, + // the lower-level Windows system calls may return false negatives, + // claiming that stdin is empty. + // + // Theoretically, one could use `WaitForSingleObject` on the stdin handle + // to achieve (1). Unfortunately, this function doesn't seem to honor the + // requested timeout and to misbehaves after the stdin is closed. + // + // There appears to be no way of achieving (2) on Windows. + // [1]: https://github.com/rust-lang/rust/pull/12422 trace!(" | passively waiting on stdin"); let dur = timeout.map(|t| t.1); let state = STDIN_POLL.poll(dur); @@ -316,10 +324,14 @@ pub(crate) fn poll_oneoff( } } } - } else if !pipe_events.is_empty() { - trace!(" | actively polling stdin or pipes"); + } + + if !pipe_events.is_empty() { + trace!(" | actively polling pipes"); match timeout { Some((event, dur)) => { + // In the tests stdin is replaced with a dummy pipe, so for now + // we just time out. Support for pipes will be decided later on. warn!("Polling pipes not supported on Windows, will just time out."); return Ok(handle_timeout(event, dur, events)); } @@ -328,32 +340,6 @@ pub(crate) fn poll_oneoff( return Err(Error::ENOTSUP); } } - // TODO remove these old comments!!! - // There are some stdin or pipe poll requests and there's no data available immediately - - // We are busy-polling the stdin with delay, unfortunately. - // - // We'd like to do the following: - // (1) wait in a non-blocking way for data to be available in stdin, with timeout - // (2) find out, how many bytes are there available to be read. - // For one, using `WaitForSingleObject` on the stdin handle could possibly be one way of - // achieving (1). - // I don't know of any way to achieve (2). - // - // While both of these are not as trivial on Windows as they are on Linux, there's a much - // more fundamental issue preventing us from achieving such behavior with the current - // implementation of wasi-common. - // - // Precisely, in `fd_read` we are using `io::stdin` via the `BufRead` trait, which does - // buffering on the libstd side. This means that even if there's still some unread data - // in stdin, the Windows system calls may return false negatives, indicating that stdin is empty. - // Therefore, avoiding the busy-poll here would require us to ditch libstd for the interaction - // with stdin altogether. - // - // However, polling stdin is a relatively infrequent use case, so this hopefully won't be - // a major issue. - - // avoid issuing more syscalls if we're requested to return immediately } Ok(()) From 716acf77d1791efaac50287a02b6675bd0cc74d0 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 18:34:08 +0100 Subject: [PATCH 21/29] Move to mpsc, drop crossbeam. Simplify --- Cargo.lock | 24 ---- crates/wasi-common/Cargo.toml | 1 - .../src/sys/windows/hostcalls_impl/misc.rs | 107 ++++++++++++------ 3 files changed, 70 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 71ae1c7039..f829cc5bff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -439,29 +439,6 @@ dependencies = [ "wasmparser 0.47.0", ] -[[package]] -name = "crossbeam" -version = "0.7.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "69323bff1fb41c635347b8ead484a5ca6c3f11914d784170b158d8449ab07f8e" -dependencies = [ - "cfg-if", - "crossbeam-channel", - "crossbeam-deque", - "crossbeam-epoch", - "crossbeam-queue", - "crossbeam-utils 0.7.0", -] - -[[package]] -name = "crossbeam-channel" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "acec9a3b0b3559f15aee4f90746c4e5e293b701c0f7d3925d24e01645267b68c" -dependencies = [ - "crossbeam-utils 0.7.0", -] - [[package]] name = "crossbeam-deque" version = "0.7.2" @@ -2008,7 +1985,6 @@ dependencies = [ "anyhow", "cfg-if", "cpu-time", - "crossbeam", "filetime", "getrandom", "lazy_static", diff --git a/crates/wasi-common/Cargo.toml b/crates/wasi-common/Cargo.toml index 3905ae0860..6c17f64a52 100644 --- a/crates/wasi-common/Cargo.toml +++ b/crates/wasi-common/Cargo.toml @@ -21,7 +21,6 @@ log = "0.4" filetime = "0.2.7" lazy_static = "1.4.0" num = { version = "0.2.0", default-features = false } -crossbeam = "0.7.3" wig = { path = "wig", version = "0.9.2" } [target.'cfg(unix)'.dependencies] diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index cdb3e6a35a..5e960d5137 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -7,56 +7,79 @@ use crate::memory::*; use crate::sys::host_impl; use crate::{wasi, wasi32, Error, Result}; use cpu_time::{ProcessTime, ThreadTime}; -use crossbeam::channel::{self, Receiver, Sender}; use lazy_static::lazy_static; -use log::{error, trace, warn}; +use log::{debug, error, trace, warn}; use std::convert::TryInto; use std::io; use std::os::windows::io::AsRawHandle; +use std::sync::mpsc::{self, Receiver, RecvTimeoutError, Sender, TryRecvError}; +use std::sync::Mutex; use std::thread; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; -type StdinPayload = io::Result; struct StdinPoll { request_tx: Sender<()>, - notify_rx: Receiver, + notify_rx: Receiver, } enum PollState { Ready, Closed, - TimedOut, + NotReady, // it's not ready, but we didn't wait + TimedOut, // it's not ready and a timeout has occurred Error(Error), } +enum WaitMode { + Timeout(Duration), + Infinite, + Immediate, +} + impl StdinPoll { - fn poll(&self, timeout: Option) -> PollState { - use crossbeam::channel::{RecvTimeoutError, TryRecvError}; - // Clean up possible unread result from previous poll + // This function should not be used directly + // Correctness of this function crucially depends on the fact that + // mpsc::Receiver is !Sync. + fn poll(&self, wait_mode: WaitMode) -> PollState { + // Clean up possible unread result from the previous poll match self.notify_rx.try_recv() { Ok(_) | Err(TryRecvError::Empty) => {} - Err(TryRecvError::Disconnected) => panic!("FIXME"), + Err(TryRecvError::Disconnected) => panic!("notify_rx channel closed"), } - self.request_tx.send(()).expect("FIXME"); - let pollret = match timeout { - Some(timeout) => self.notify_rx.recv_timeout(timeout), - None => Ok(self.notify_rx.recv().expect("FIXME")), + + // Notify the worker thread that we want to poll stdin + self.request_tx.send(()).expect("request_tx channel closed"); + + // Wait for the worker thread to send a readiness notification + let pollret = match wait_mode { + WaitMode::Timeout(timeout) => { + self.notify_rx + .recv_timeout(timeout) + .unwrap_or_else(|e| match e { + RecvTimeoutError::Disconnected => panic!("notify_rx channel closed"), + RecvTimeoutError::Timeout => PollState::TimedOut, + }) + } + WaitMode::Infinite => self.notify_rx.recv().expect("notify_rx channel closed"), + WaitMode::Immediate => self.notify_rx.try_recv().unwrap_or_else(|e| match e { + TryRecvError::Disconnected => panic!("notify_rx channel closed"), + TryRecvError::Empty => PollState::NotReady, + }), }; - match pollret { - Ok(Ok(true)) => PollState::Ready, - Ok(Ok(false)) => PollState::Closed, - Ok(Err(e)) => PollState::Error(e.into()), - Err(RecvTimeoutError::Timeout) => PollState::TimedOut, - Err(RecvTimeoutError::Disconnected) => panic!("FIXME"), - } + + pollret } - fn event_loop(request_rx: Receiver<()>, notify_tx: Sender) -> ! { + fn event_loop(request_rx: Receiver<()>, notify_tx: Sender) -> ! { use std::io::BufRead; loop { - request_rx.recv().expect("FIXME"); - let buf = std::io::stdin().lock().fill_buf().map(|s| !s.is_empty()); - notify_tx.send(buf).expect("FIXME"); + request_rx.recv().expect("request_rx channel closed"); + let resp = match std::io::stdin().lock().fill_buf().map(|s| !s.is_empty()) { + Ok(true) => PollState::Ready, + Ok(false) => PollState::Closed, + Err(e) => PollState::Error(e.into()), + }; + notify_tx.send(resp).expect("notify_tx channel closed"); } } } @@ -64,15 +87,14 @@ impl StdinPoll { lazy_static! { static ref START_MONOTONIC: Instant = Instant::now(); static ref PERF_COUNTER_RES: u64 = get_perf_counter_resolution_ns(); - static ref STDIN_POLL: StdinPoll = { - let channel_size = 1; - let (request_tx, request_rx) = channel::bounded(channel_size); - let (notify_tx, notify_rx) = channel::bounded(channel_size); + static ref STDIN_POLL: Mutex = { + let (request_tx, request_rx) = mpsc::channel(); + let (notify_tx, notify_rx) = mpsc::channel(); thread::spawn(move || StdinPoll::event_loop(request_rx, notify_tx)); - StdinPoll { + Mutex::new(StdinPoll { request_tx, notify_rx, - } + }) }; } @@ -281,13 +303,15 @@ pub(crate) fn poll_oneoff( } } + let immediate = !immediate_events.is_empty(); // Process all the events that do not require waiting. - if !immediate_events.is_empty() { + if immediate { trace!(" | have immediate events, will return immediately"); for mut event in immediate_events { handle_rw_event(event, events); } - } else if !stdin_events.is_empty() { + } + if !stdin_events.is_empty() { // During the firt request to poll stdin, we spin up a separate thread to // waiting for data to arrive on stdin. This thread will not terminate. // @@ -310,12 +334,21 @@ pub(crate) fn poll_oneoff( // // There appears to be no way of achieving (2) on Windows. // [1]: https://github.com/rust-lang/rust/pull/12422 - trace!(" | passively waiting on stdin"); - let dur = timeout.map(|t| t.1); - let state = STDIN_POLL.poll(dur); + let dur = if immediate { + trace!(" | tentatively checking stdin"); + WaitMode::Immediate + } else { + trace!(" | passively waiting on stdin"); + match timeout { + Some((event, dur)) => WaitMode::Timeout(dur), + None => WaitMode::Infinite, + } + }; + let state = STDIN_POLL.lock().unwrap().poll(dur); for event in stdin_events { match state { PollState::Ready => handle_rw_event(event, events), + PollState::NotReady => {} PollState::Closed => { /* error? FIXME */ } PollState::TimedOut => handle_timeout_event(timeout.unwrap().0, events), PollState::Error(ref e) => { @@ -326,14 +359,14 @@ pub(crate) fn poll_oneoff( } } - if !pipe_events.is_empty() { + if !immediate && !pipe_events.is_empty() { trace!(" | actively polling pipes"); match timeout { Some((event, dur)) => { // In the tests stdin is replaced with a dummy pipe, so for now // we just time out. Support for pipes will be decided later on. warn!("Polling pipes not supported on Windows, will just time out."); - return Ok(handle_timeout(event, dur, events)); + handle_timeout(event, dur, events); } None => { error!("Polling only pipes with no timeout not supported on Windows."); From caa6897af5634e93f7ee73a7ba53e914fc8ce087 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 19:24:24 +0100 Subject: [PATCH 22/29] Finish minimal impl --- .../src/sys/windows/hostcalls_impl/misc.rs | 33 ++++++++++++++----- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 5e960d5137..16c1a58586 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -187,6 +187,20 @@ fn make_timeout_event(timeout: &ClockEventData) -> wasi::__wasi_event_t { } } +fn make_hangup_event(fd_event: &FdEventData) -> wasi::__wasi_event_t { + wasi::__wasi_event_t { + userdata: fd_event.userdata, + r#type: fd_event.r#type, + error: wasi::__WASI_ERRNO_SUCCESS, + u: wasi::__wasi_event_u_t { + fd_readwrite: wasi::__wasi_event_fd_readwrite_t { + nbytes: 0, + flags: wasi::__WASI_EVENTRWFLAGS_FD_READWRITE_HANGUP, + }, + }, + } +} + fn handle_timeout( timeout_event: ClockEventData, timeout: Duration, @@ -201,6 +215,11 @@ fn handle_timeout_event(timeout_event: ClockEventData, events: &mut Vec) { + let new_event = make_hangup_event(&event); + events.push(new_event) +} + fn handle_rw_event(event: FdEventData, out_events: &mut Vec) { let size = match event.descriptor { Descriptor::OsHandle(os_handle) => { @@ -315,8 +334,6 @@ pub(crate) fn poll_oneoff( // During the firt request to poll stdin, we spin up a separate thread to // waiting for data to arrive on stdin. This thread will not terminate. // - // FIXME this is not thread safe! - // // We'd like to do the following: // (1) wait in a non-blocking way for data to be available in stdin, with timeout // (2) find out, how many bytes are there available to be read. @@ -334,7 +351,7 @@ pub(crate) fn poll_oneoff( // // There appears to be no way of achieving (2) on Windows. // [1]: https://github.com/rust-lang/rust/pull/12422 - let dur = if immediate { + let waitmode = if immediate { trace!(" | tentatively checking stdin"); WaitMode::Immediate } else { @@ -344,16 +361,16 @@ pub(crate) fn poll_oneoff( None => WaitMode::Infinite, } }; - let state = STDIN_POLL.lock().unwrap().poll(dur); + let state = STDIN_POLL.lock().unwrap().poll(waitmode); for event in stdin_events { match state { PollState::Ready => handle_rw_event(event, events), - PollState::NotReady => {} - PollState::Closed => { /* error? FIXME */ } + PollState::NotReady => {} // not immediately available, so just ignore + PollState::Closed => handle_hangup_event(event, events), // TODO check if actually a POLLHUP on Linux PollState::TimedOut => handle_timeout_event(timeout.unwrap().0, events), PollState::Error(ref e) => { - error!("PollState error"); - handle_error_event(event, Error::ENOTSUP /*FIXME*/, events); + error!("FIXME return real error"); + handle_error_event(event, Error::ENOTSUP, events); } } } From 4f9218ededdcdd7c34531723c66f5e8778116c59 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 19:41:34 +0100 Subject: [PATCH 23/29] Get rid of hangup, it's incorrect --- crates/api/c-examples/wasm-c-api | 1 + crates/wasi-common/WASI | 1 + .../src/sys/windows/hostcalls_impl/misc.rs | 30 ++++--------------- 3 files changed, 8 insertions(+), 24 deletions(-) create mode 160000 crates/api/c-examples/wasm-c-api create mode 160000 crates/wasi-common/WASI diff --git a/crates/api/c-examples/wasm-c-api b/crates/api/c-examples/wasm-c-api new file mode 160000 index 0000000000..d9a80099d4 --- /dev/null +++ b/crates/api/c-examples/wasm-c-api @@ -0,0 +1 @@ +Subproject commit d9a80099d496b5cdba6f3fe8fc77586e0e505ddc diff --git a/crates/wasi-common/WASI b/crates/wasi-common/WASI new file mode 160000 index 0000000000..04d4eba571 --- /dev/null +++ b/crates/wasi-common/WASI @@ -0,0 +1 @@ +Subproject commit 04d4eba571dc1d6fe9ab129ea9343911bcc256dc diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 16c1a58586..1403e2f4d5 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -24,7 +24,6 @@ struct StdinPoll { enum PollState { Ready, - Closed, NotReady, // it's not ready, but we didn't wait TimedOut, // it's not ready and a timeout has occurred Error(Error), @@ -74,9 +73,12 @@ impl StdinPoll { use std::io::BufRead; loop { request_rx.recv().expect("request_rx channel closed"); - let resp = match std::io::stdin().lock().fill_buf().map(|s| !s.is_empty()) { - Ok(true) => PollState::Ready, - Ok(false) => PollState::Closed, + // If `fill_buf` returns any slice, then it means that either + // (a) there some data in stdout, if it's non-empty + // (b) EOF was received, if it's empty + // Linux returns `POLLIN` in both cases, and we imitate this behavior. + let resp = match std::io::stdin().lock().fill_buf() { + Ok(_) => PollState::Ready, Err(e) => PollState::Error(e.into()), }; notify_tx.send(resp).expect("notify_tx channel closed"); @@ -187,20 +189,6 @@ fn make_timeout_event(timeout: &ClockEventData) -> wasi::__wasi_event_t { } } -fn make_hangup_event(fd_event: &FdEventData) -> wasi::__wasi_event_t { - wasi::__wasi_event_t { - userdata: fd_event.userdata, - r#type: fd_event.r#type, - error: wasi::__WASI_ERRNO_SUCCESS, - u: wasi::__wasi_event_u_t { - fd_readwrite: wasi::__wasi_event_fd_readwrite_t { - nbytes: 0, - flags: wasi::__WASI_EVENTRWFLAGS_FD_READWRITE_HANGUP, - }, - }, - } -} - fn handle_timeout( timeout_event: ClockEventData, timeout: Duration, @@ -215,11 +203,6 @@ fn handle_timeout_event(timeout_event: ClockEventData, events: &mut Vec) { - let new_event = make_hangup_event(&event); - events.push(new_event) -} - fn handle_rw_event(event: FdEventData, out_events: &mut Vec) { let size = match event.descriptor { Descriptor::OsHandle(os_handle) => { @@ -366,7 +349,6 @@ pub(crate) fn poll_oneoff( match state { PollState::Ready => handle_rw_event(event, events), PollState::NotReady => {} // not immediately available, so just ignore - PollState::Closed => handle_hangup_event(event, events), // TODO check if actually a POLLHUP on Linux PollState::TimedOut => handle_timeout_event(timeout.unwrap().0, events), PollState::Error(ref e) => { error!("FIXME return real error"); From 1c050b6a33557870951d4ccd85538af96b04e400 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 19:47:04 +0100 Subject: [PATCH 24/29] Reset crates/wasi-common/WASI to upstream/master --- crates/wasi-common/WASI | 1 - 1 file changed, 1 deletion(-) delete mode 160000 crates/wasi-common/WASI diff --git a/crates/wasi-common/WASI b/crates/wasi-common/WASI deleted file mode 160000 index 04d4eba571..0000000000 --- a/crates/wasi-common/WASI +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 04d4eba571dc1d6fe9ab129ea9343911bcc256dc From b5f293a71f75b512bea3ffdbe0762b2d44468e01 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 19:48:00 +0100 Subject: [PATCH 25/29] Reset crates/api/c-examples/wasm-c-api to upstream/master --- crates/api/c-examples/wasm-c-api | 1 - 1 file changed, 1 deletion(-) delete mode 160000 crates/api/c-examples/wasm-c-api diff --git a/crates/api/c-examples/wasm-c-api b/crates/api/c-examples/wasm-c-api deleted file mode 160000 index d9a80099d4..0000000000 --- a/crates/api/c-examples/wasm-c-api +++ /dev/null @@ -1 +0,0 @@ -Subproject commit d9a80099d496b5cdba6f3fe8fc77586e0e505ddc From e4905c31006eb6aa36e7586cc57c9104d2f9973e Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Thu, 16 Jan 2020 19:50:16 +0100 Subject: [PATCH 26/29] Extra comments --- crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 1403e2f4d5..40e6d694e2 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -72,7 +72,10 @@ impl StdinPoll { fn event_loop(request_rx: Receiver<()>, notify_tx: Sender) -> ! { use std::io::BufRead; loop { + // Wait for the request to poll stdin request_rx.recv().expect("request_rx channel closed"); + + // Wait for data to appear in stdin. // If `fill_buf` returns any slice, then it means that either // (a) there some data in stdout, if it's non-empty // (b) EOF was received, if it's empty @@ -81,6 +84,9 @@ impl StdinPoll { Ok(_) => PollState::Ready, Err(e) => PollState::Error(e.into()), }; + + // Notify the requestor about data in stdin. They may have already timed out, + // then the next requestor will have to clean the channel. notify_tx.send(resp).expect("notify_tx channel closed"); } } From 5b5f9a7b06f4894c692efd85ccdbaa91dfdd896f Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Fri, 17 Jan 2020 09:12:12 +0100 Subject: [PATCH 27/29] Properly return errors. --- crates/wasi-common/src/error.rs | 10 ++++----- .../src/sys/windows/hostcalls_impl/misc.rs | 21 ++++++++----------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/crates/wasi-common/src/error.rs b/crates/wasi-common/src/error.rs index c18856d4bc..4ce56bbc57 100644 --- a/crates/wasi-common/src/error.rs +++ b/crates/wasi-common/src/error.rs @@ -248,15 +248,15 @@ pub(crate) trait FromRawOsError { pub(crate) type Result = std::result::Result; -pub(crate) trait WasiErrno { - fn as_wasi_errno(&self) -> wasi::__wasi_errno_t; +pub(crate) trait AsWasiError { + fn as_wasi_error(&self) -> WasiError; } -impl WasiErrno for Result { - fn as_wasi_errno(&self) -> wasi::__wasi_errno_t { +impl AsWasiError for Result { + fn as_wasi_error(&self) -> WasiError { self.as_ref() .err() .unwrap_or(&Error::ESUCCESS) - .as_wasi_errno() + .as_wasi_error() } } diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 40e6d694e2..8d2ffa0efb 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -5,7 +5,7 @@ use crate::fdentry::Descriptor; use crate::hostcalls_impl::{ClockEventData, FdEventData}; use crate::memory::*; use crate::sys::host_impl; -use crate::{wasi, wasi32, Error, Result}; +use crate::{error::WasiError, wasi, wasi32, Error, Result}; use cpu_time::{ProcessTime, ThreadTime}; use lazy_static::lazy_static; use log::{debug, error, trace, warn}; @@ -24,9 +24,9 @@ struct StdinPoll { enum PollState { Ready, - NotReady, // it's not ready, but we didn't wait - TimedOut, // it's not ready and a timeout has occurred - Error(Error), + NotReady, // it's not ready, but we didn't wait + TimedOut, // it's not ready and a timeout has occurred + Error(WasiError), // not using the top-lever Error because it's not Clone } enum WaitMode { @@ -82,7 +82,7 @@ impl StdinPoll { // Linux returns `POLLIN` in both cases, and we imitate this behavior. let resp = match std::io::stdin().lock().fill_buf() { Ok(_) => PollState::Ready, - Err(e) => PollState::Error(e.into()), + Err(e) => PollState::Error(Error::from(e).as_wasi_error()), }; // Notify the requestor about data in stdin. They may have already timed out, @@ -168,13 +168,13 @@ pub(crate) fn clock_time_get(clock_id: wasi::__wasi_clockid_t) -> Result) -> wasi::__wasi_event_t { - use crate::error::WasiErrno; - let error = nbytes.as_wasi_errno(); + use crate::error::AsWasiError; + let error = nbytes.as_wasi_error(); let nbytes = nbytes.unwrap_or_default(); wasi::__wasi_event_t { userdata: event.userdata, r#type: event.r#type, - error, + error: error.as_raw_errno(), u: wasi::__wasi_event_u_t { fd_readwrite: wasi::__wasi_event_fd_readwrite_t { nbytes, flags: 0 }, }, @@ -356,10 +356,7 @@ pub(crate) fn poll_oneoff( PollState::Ready => handle_rw_event(event, events), PollState::NotReady => {} // not immediately available, so just ignore PollState::TimedOut => handle_timeout_event(timeout.unwrap().0, events), - PollState::Error(ref e) => { - error!("FIXME return real error"); - handle_error_event(event, Error::ENOTSUP, events); - } + PollState::Error(e) => handle_error_event(event, Error::Wasi(e), events), } } } From 3d29244203e816e9537e5eccf891db207fbfc083 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Fri, 17 Jan 2020 22:26:22 +0100 Subject: [PATCH 28/29] Cleanup empty event behavior --- .../src/sys/windows/hostcalls_impl/misc.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 8d2ffa0efb..4837003ec9 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -263,18 +263,9 @@ pub(crate) fn poll_oneoff( if fd_events.is_empty() { match timeout { Some((event, dur)) => return Ok(handle_timeout(event, dur, events)), - // `poll` invoked with nfds = 0, timeout = -1 appears to be an infinite sleep on Unix - // usually meant to be interrupted by a signal. Unfortunately, WASI doesn't currently - // support signals and there is no way to interrupt this infinite sleep, so we - // intend to return `ENOTSUP`. - // - // Unfortunately, current implementation of poll_oneoff on Unix relies on - // `poll_oneoff` returning `Ok(())` in such case, so we emulate this behavior for now - // and will address it in a subsequent PR. - None => { - // error!("poll_oneoff: invoking with neither timeout nor fds not supported"); - return Ok(()); // Err(Error::ENOTSUP); - } + // The implementation has to return Ok(()) in this case, + // cf. the comment in src/hostcalls_impl/misc.rs + None => return Ok(()), } } From 13afbd0bae4d443107ef279c14b3270e669fae5d Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Fri, 17 Jan 2020 22:27:37 +0100 Subject: [PATCH 29/29] Fix a typo. Co-Authored-By: Peter Huene --- crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs index 4837003ec9..1e2db73404 100644 --- a/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/sys/windows/hostcalls_impl/misc.rs @@ -77,7 +77,7 @@ impl StdinPoll { // Wait for data to appear in stdin. // If `fill_buf` returns any slice, then it means that either - // (a) there some data in stdout, if it's non-empty + // (a) there some data in stdin, if it's non-empty // (b) EOF was received, if it's empty // Linux returns `POLLIN` in both cases, and we imitate this behavior. let resp = match std::io::stdin().lock().fill_buf() {