From 76471e05bb48ab1664fea28412aa323ad37be673 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 10 May 2022 10:49:56 -0400 Subject: [PATCH 1/7] refactor: `StdFileResource` - remove unused cancel handle --- runtime/ops/io.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/runtime/ops/io.rs b/runtime/ops/io.rs index 709d2fb3ea0dab..05388ac5c41f75 100644 --- a/runtime/ops/io.rs +++ b/runtime/ops/io.rs @@ -301,11 +301,9 @@ impl Resource for ChildStderrResource { } } -#[derive(Default)] pub struct StdFileResource { fs_file: Option>>, metadata: RefCell, - cancel: CancelHandle, name: String, } @@ -313,18 +311,16 @@ impl StdFileResource { pub fn stdio(std_file: &StdFile, name: &str) -> Self { Self { fs_file: std_file.try_clone().map(|s| Arc::new(Mutex::new(s))).ok(), - metadata: RefCell::new(FileMetadata::default()), + metadata: Default::default(), name: name.to_string(), - ..Default::default() } } pub fn fs_file(fs_file: StdFile) -> Self { Self { fs_file: Some(Arc::new(Mutex::new(fs_file))), - metadata: RefCell::new(FileMetadata::default()), + metadata: Default::default(), name: "fsFile".to_string(), - ..Default::default() } } @@ -407,8 +403,7 @@ impl Resource for StdFileResource { } fn close(self: Rc) { - // TODO: do not cancel file I/O when file is writable. - self.cancel.cancel() + // do nothing } } From b16db899aafdd730d1dc338e7907ed78e79d0a76 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 10 May 2022 13:20:48 -0400 Subject: [PATCH 2/7] fix: windows stdout and stderr encoding --- cli/tests/integration/repl_tests.rs | 4 +- runtime/ops/fs.rs | 94 +++++------- runtime/ops/io.rs | 219 +++++++++++++++++++--------- runtime/ops/process.rs | 5 +- runtime/ops/tty.rs | 114 +++++++-------- 5 files changed, 249 insertions(+), 187 deletions(-) diff --git a/cli/tests/integration/repl_tests.rs b/cli/tests/integration/repl_tests.rs index 44036795f5da5a..c4e03d65098568 100644 --- a/cli/tests/integration/repl_tests.rs +++ b/cli/tests/integration/repl_tests.rs @@ -164,7 +164,9 @@ fn pty_complete_imports() { let output = console.read_all_output(); assert!(output.contains("Hello World")); if cfg!(windows) { - assert!(output.contains("testing output\u{1b}")); + if !output.contains("testing output\u{1b}") { + panic!("Output did not contain expected:\n{:?}", output); + } } else { assert!(output.contains("\ntesting output")); } diff --git a/runtime/ops/fs.rs b/runtime/ops/fs.rs index 2bcd6caccaff87..fdfdff6b436e17 100644 --- a/runtime/ops/fs.rs +++ b/runtime/ops/fs.rs @@ -322,11 +322,8 @@ fn seek_helper(args: SeekArgs) -> Result<(u32, SeekFrom), AnyError> { #[op] fn op_seek_sync(state: &mut OpState, args: SeekArgs) -> Result { let (rid, seek_from) = seek_helper(args)?; - let pos = StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => std_file.seek(seek_from).map_err(AnyError::from), - Err(_) => Err(type_error( - "cannot seek on this type of resource".to_string(), - )), + let pos = StdFileResource::with_file(state, rid, |std_file| { + std_file.seek(seek_from).map_err(AnyError::from) })?; Ok(pos) } @@ -343,10 +340,10 @@ async fn op_seek_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); tokio::task::spawn_blocking(move || { - let mut std_file = std_file.lock().unwrap(); + let mut std_file = std_file.lock(); std_file.seek(seek_from) }) .await? @@ -358,9 +355,8 @@ fn op_fdatasync_sync( state: &mut OpState, rid: ResourceId, ) -> Result<(), AnyError> { - StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => std_file.sync_data().map_err(AnyError::from), - Err(_) => Err(type_error("cannot sync this type of resource".to_string())), + StdFileResource::with_file(state, rid, |std_file| { + std_file.sync_data().map_err(AnyError::from) })?; Ok(()) } @@ -375,10 +371,10 @@ async fn op_fdatasync_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); tokio::task::spawn_blocking(move || { - let std_file = std_file.lock().unwrap(); + let std_file = std_file.lock(); std_file.sync_data() }) .await? @@ -387,9 +383,8 @@ async fn op_fdatasync_async( #[op] fn op_fsync_sync(state: &mut OpState, rid: ResourceId) -> Result<(), AnyError> { - StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => std_file.sync_all().map_err(AnyError::from), - Err(_) => Err(type_error("cannot sync this type of resource".to_string())), + StdFileResource::with_file(state, rid, |std_file| { + std_file.sync_all().map_err(AnyError::from) })?; Ok(()) } @@ -404,10 +399,10 @@ async fn op_fsync_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); tokio::task::spawn_blocking(move || { - let std_file = std_file.lock().unwrap(); + let std_file = std_file.lock(); std_file.sync_all() }) .await? @@ -419,9 +414,8 @@ fn op_fstat_sync( state: &mut OpState, rid: ResourceId, ) -> Result { - let metadata = StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => std_file.metadata().map_err(AnyError::from), - Err(_) => Err(type_error("cannot stat this type of resource".to_string())), + let metadata = StdFileResource::with_file(state, rid, |std_file| { + std_file.metadata().map_err(AnyError::from) })?; Ok(get_stat(metadata)) } @@ -436,10 +430,10 @@ async fn op_fstat_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); let metadata = tokio::task::spawn_blocking(move || { - let std_file = std_file.lock().unwrap(); + let std_file = std_file.lock(); std_file.metadata() }) .await? @@ -456,16 +450,13 @@ fn op_flock_sync( use fs3::FileExt; super::check_unstable(state, "Deno.flockSync"); - StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => { - if exclusive { - std_file.lock_exclusive()?; - } else { - std_file.lock_shared()?; - } - Ok(()) + StdFileResource::with_file(state, rid, |std_file| { + if exclusive { + std_file.lock_exclusive()?; + } else { + std_file.lock_shared()?; } - Err(_) => Err(type_error("cannot lock this type of resource".to_string())), + Ok(()) }) } @@ -483,10 +474,10 @@ async fn op_flock_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); tokio::task::spawn_blocking(move || -> Result<(), AnyError> { - let std_file = std_file.lock().unwrap(); + let std_file = std_file.lock(); if exclusive { std_file.lock_exclusive()?; } else { @@ -505,12 +496,9 @@ fn op_funlock_sync( use fs3::FileExt; super::check_unstable(state, "Deno.funlockSync"); - StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => { - std_file.unlock()?; - Ok(()) - } - Err(_) => Err(type_error("cannot lock this type of resource".to_string())), + StdFileResource::with_file(state, rid, |std_file| { + std_file.unlock()?; + Ok(()) }) } @@ -527,10 +515,10 @@ async fn op_funlock_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); tokio::task::spawn_blocking(move || -> Result<(), AnyError> { - let std_file = std_file.lock().unwrap(); + let std_file = std_file.lock(); std_file.unlock()?; Ok(()) }) @@ -1590,9 +1578,8 @@ fn op_ftruncate_sync( ) -> Result<(), AnyError> { let rid = args.rid; let len = args.len as u64; - StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => std_file.set_len(len).map_err(AnyError::from), - Err(_) => Err(type_error("cannot truncate this type of resource")), + StdFileResource::with_file(state, rid, |std_file| { + std_file.set_len(len).map_err(AnyError::from) })?; Ok(()) } @@ -1610,10 +1597,10 @@ async fn op_ftruncate_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); tokio::task::spawn_blocking(move || { - let std_file = std_file.lock().unwrap(); + let std_file = std_file.lock(); std_file.set_len(len) }) .await? @@ -1879,14 +1866,9 @@ fn op_futime_sync( let atime = filetime::FileTime::from_unix_time(args.atime.0, args.atime.1); let mtime = filetime::FileTime::from_unix_time(args.mtime.0, args.mtime.1); - StdFileResource::with(state, rid, |r| match r { - Ok(std_file) => { - filetime::set_file_handle_times(std_file, Some(atime), Some(mtime)) - .map_err(AnyError::from) - } - Err(_) => Err(type_error( - "cannot futime on this type of resource".to_string(), - )), + StdFileResource::with_file(state, rid, |std_file| { + filetime::set_file_handle_times(std_file, Some(atime), Some(mtime)) + .map_err(AnyError::from) })?; Ok(()) @@ -1907,9 +1889,9 @@ async fn op_futime_async( .resource_table .get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); tokio::task::spawn_blocking(move || { - let std_file = std_file.lock().unwrap(); + let std_file = std_file.lock(); filetime::set_file_handle_times(&std_file, Some(atime), Some(mtime))?; Ok(()) }) diff --git a/runtime/ops/io.rs b/runtime/ops/io.rs index 05388ac5c41f75..d5570e88bb619f 100644 --- a/runtime/ops/io.rs +++ b/runtime/ops/io.rs @@ -1,10 +1,8 @@ // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. -use deno_core::error::bad_resource_id; -use deno_core::error::not_supported; -use deno_core::error::resource_unavailable; use deno_core::error::AnyError; use deno_core::op; +use deno_core::parking_lot::Mutex; use deno_core::AsyncMutFuture; use deno_core::AsyncRefCell; use deno_core::AsyncResult; @@ -24,7 +22,6 @@ use std::io::Read; use std::io::Write; use std::rc::Rc; use std::sync::Arc; -use std::sync::Mutex; use tokio::io::AsyncRead; use tokio::io::AsyncReadExt; use tokio::io::AsyncWrite; @@ -40,6 +37,9 @@ use { winapi::um::{processenv::GetStdHandle, winbase}, }; +// Store the stdio fd/handles in global statics in order to keep them +// alive for the duration of the application since the last handle/fd +// being dropped will close the corresponding pipe. #[cfg(unix)] static STDIN_HANDLE: Lazy = Lazy::new(|| unsafe { StdFile::from_raw_fd(0) }); @@ -50,14 +50,6 @@ static STDOUT_HANDLE: Lazy = static STDERR_HANDLE: Lazy = Lazy::new(|| unsafe { StdFile::from_raw_fd(2) }); -/// Due to portability issues on Windows handle to stdout is created from raw -/// file descriptor. The caveat of that approach is fact that when this -/// handle is dropped underlying file descriptor is closed - that is highly -/// not desirable in case of stdout. That's why we store this global handle -/// that is then cloned when obtaining stdio for process. In turn when -/// resource table is dropped storing reference to that handle, the handle -/// itself won't be closed (so Deno.core.print) will still work. -// TODO(ry) It should be possible to close stdout. #[cfg(windows)] static STDIN_HANDLE: Lazy = Lazy::new(|| unsafe { StdFile::from_raw_handle(GetStdHandle(winbase::STD_INPUT_HANDLE)) @@ -122,23 +114,23 @@ pub fn init_stdio(stdio: Stdio) -> Extension { .expect("Extension only supports being used once."); let t = &mut state.resource_table; t.add(StdFileResource::stdio( - match &stdio.stdin { - StdioPipe::Inherit => &STDIN_HANDLE, - StdioPipe::File(pipe) => pipe, + match stdio.stdin { + StdioPipe::Inherit => StdFileResourceInner::Stdin, + StdioPipe::File(pipe) => StdFileResourceInner::file(pipe), }, "stdin", )); t.add(StdFileResource::stdio( - match &stdio.stdout { - StdioPipe::Inherit => &STDOUT_HANDLE, - StdioPipe::File(pipe) => pipe, + match stdio.stdout { + StdioPipe::Inherit => StdFileResourceInner::Stdout, + StdioPipe::File(pipe) => StdFileResourceInner::file(pipe), }, "stdout", )); t.add(StdFileResource::stdio( - match &stdio.stderr { - StdioPipe::Inherit => &STDERR_HANDLE, - StdioPipe::File(pipe) => pipe, + match stdio.stderr { + StdioPipe::Inherit => StdFileResourceInner::Stderr, + StdioPipe::File(pipe) => StdFileResourceInner::file(pipe), }, "stderr", )); @@ -301,16 +293,94 @@ impl Resource for ChildStderrResource { } } +#[derive(Clone)] +enum StdFileResourceInner { + // Ideally we would store stdio as an StdFile, but we get some Windows + // specific functionality for free by using Rust std's wrappers. So we + // take a bit of a complexity hit here in order to not have to duplicate + // the functionality in Rust's std/src/sys/windows/stdio.rs + Stdin, + Stdout, + Stderr, + File(Arc>), +} + +impl StdFileResourceInner { + pub fn file(fs_file: StdFile) -> Self { + StdFileResourceInner::File(Arc::new(Mutex::new(fs_file))) + } + + pub fn with_file(&self, mut f: impl FnMut(&mut StdFile) -> R) -> R { + match self { + Self::Stdin => f(&mut STDIN_HANDLE.try_clone().unwrap()), + Self::Stdout => f(&mut STDOUT_HANDLE.try_clone().unwrap()), + Self::Stderr => f(&mut STDERR_HANDLE.try_clone().unwrap()), + Self::File(file) => { + let mut file = file.lock(); + f(&mut file) + } + } + } + + pub fn write_and_maybe_flush( + &mut self, + buf: &[u8], + ) -> Result { + let nwritten = self.write(buf)?; + if !matches!(self, StdFileResourceInner::File(_)) { + // Rust will line buffer and we don't want that behavior + // (see https://github.com/denoland/deno/issues/948), so flush. + // Although an alternative solution could be to bypass Rust's std by + // using the raw fds/handles, it will cause encoding issues on Windows + // that we get solved for free by using Rust's stdio wrappers (see + // std/src/sys/windows/stdio.rs in Rust's source code). + self.flush()?; + } + Ok(nwritten) + } +} + +impl Read for StdFileResourceInner { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + match self { + Self::Stdout => unreachable!(), + Self::Stderr => unreachable!(), + Self::Stdin => std::io::stdin().read(buf), + Self::File(file) => file.lock().read(buf), + } + } +} + +impl Write for StdFileResourceInner { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + match self { + Self::Stdout => std::io::stdout().write(buf), + Self::Stderr => std::io::stderr().write(buf), + Self::Stdin => unreachable!(), + Self::File(file) => file.lock().write(buf), + } + } + + fn flush(&mut self) -> std::io::Result<()> { + match self { + Self::Stdout => std::io::stdout().flush(), + Self::Stderr => std::io::stderr().flush(), + Self::Stdin => unreachable!(), + Self::File(file) => file.lock().flush(), + } + } +} + pub struct StdFileResource { - fs_file: Option>>, + inner: StdFileResourceInner, metadata: RefCell, name: String, } impl StdFileResource { - pub fn stdio(std_file: &StdFile, name: &str) -> Self { + fn stdio(inner: StdFileResourceInner, name: &str) -> Self { Self { - fs_file: std_file.try_clone().map(|s| Arc::new(Mutex::new(s))).ok(), + inner, metadata: Default::default(), name: name.to_string(), } @@ -318,16 +388,24 @@ impl StdFileResource { pub fn fs_file(fs_file: StdFile) -> Self { Self { - fs_file: Some(Arc::new(Mutex::new(fs_file))), + inner: StdFileResourceInner::file(fs_file), metadata: Default::default(), name: "fsFile".to_string(), } } - pub fn std_file(&self) -> Result>, AnyError> { - match &self.fs_file { - Some(fs_file) => Ok(fs_file.clone()), - None => Err(bad_resource_id()), + pub fn std_file(&self) -> Arc> { + match &self.inner { + StdFileResourceInner::File(fs_file) => fs_file.clone(), + StdFileResourceInner::Stdin => { + Arc::new(Mutex::new(STDIN_HANDLE.try_clone().unwrap())) + } + StdFileResourceInner::Stdout => { + Arc::new(Mutex::new(STDOUT_HANDLE.try_clone().unwrap())) + } + StdFileResourceInner::Stderr => { + Arc::new(Mutex::new(STDERR_HANDLE.try_clone().unwrap())) + } } } @@ -339,49 +417,65 @@ impl StdFileResource { self: Rc, mut buf: ZeroCopyBuf, ) -> Result<(usize, ZeroCopyBuf), AnyError> { - let std_file = self.fs_file.as_ref().unwrap().clone(); + let mut inner = self.inner.clone(); tokio::task::spawn_blocking( move || -> Result<(usize, ZeroCopyBuf), AnyError> { - let mut std_file = std_file.lock().unwrap(); - Ok((std_file.read(&mut buf)?, buf)) + Ok((inner.read(&mut buf)?, buf)) }, ) .await? } async fn write(self: Rc, buf: ZeroCopyBuf) -> Result { - let std_file = self.fs_file.as_ref().unwrap().clone(); - tokio::task::spawn_blocking(move || { - let mut std_file = std_file.lock().unwrap(); - std_file.write(&buf) - }) - .await? - .map_err(AnyError::from) + let mut inner = self.inner.clone(); + tokio::task::spawn_blocking(move || inner.write_and_maybe_flush(&buf)) + .await? + .map_err(AnyError::from) } - pub fn with( + fn with_inner( state: &mut OpState, rid: ResourceId, mut f: F, ) -> Result where - F: FnMut(Result<&mut std::fs::File, ()>) -> Result, + F: FnMut(StdFileResourceInner) -> Result, { let resource = state.resource_table.get::(rid)?; + f(resource.inner.clone()) + } - match &resource.fs_file { - Some(r) => f(Ok(&mut r.as_ref().lock().unwrap())), - None => Err(resource_unavailable()), - } + pub fn with_file( + state: &mut OpState, + rid: ResourceId, + f: F, + ) -> Result + where + F: FnMut(&mut StdFile) -> Result, + { + let resource = state.resource_table.get::(rid)?; + resource.inner.with_file(f) } pub fn clone_file( state: &mut OpState, rid: ResourceId, - ) -> Result { - Self::with(state, rid, move |r| match r { - Ok(std_file) => std_file.try_clone().map_err(AnyError::from), - Err(_) => Err(bad_resource_id()), + ) -> Result { + Self::with_file(state, rid, move |std_file| { + std_file.try_clone().map_err(AnyError::from) + }) + } + + pub fn as_stdio( + state: &mut OpState, + rid: u32, + ) -> Result { + Self::with_inner(state, rid, |inner| match inner { + StdFileResourceInner::File(file) => { + let file = file.lock().try_clone()?; + Ok(file.into()) + } + _ => Ok(std::process::Stdio::inherit()), }) } } @@ -415,13 +509,10 @@ pub fn op_print( is_err: bool, ) -> Result<(), AnyError> { let rid = if is_err { 2 } else { 1 }; - StdFileResource::with(state, rid, move |r| match r { - Ok(std_file) => { - std_file.write_all(msg.as_bytes())?; - std_file.flush().unwrap(); - Ok(()) - } - Err(_) => Err(not_supported()), + StdFileResource::with_inner(state, rid, move |mut inner| { + inner.write_all(msg.as_bytes())?; + inner.flush().unwrap(); + Ok(()) }) } @@ -431,12 +522,11 @@ fn op_read_sync( rid: ResourceId, mut buf: ZeroCopyBuf, ) -> Result { - StdFileResource::with(state, rid, move |r| match r { - Ok(std_file) => std_file + StdFileResource::with_inner(state, rid, move |mut inner| { + inner .read(&mut buf) .map(|n: usize| n as u32) - .map_err(AnyError::from), - Err(_) => Err(not_supported()), + .map_err(AnyError::from) }) } @@ -446,11 +536,10 @@ fn op_write_sync( rid: ResourceId, buf: ZeroCopyBuf, ) -> Result { - StdFileResource::with(state, rid, move |r| match r { - Ok(std_file) => std_file - .write(&buf) + StdFileResource::with_inner(state, rid, move |mut inner| { + inner + .write_and_maybe_flush(&buf) .map(|nwritten: usize| nwritten as u32) - .map_err(AnyError::from), - Err(_) => Err(not_supported()), + .map_err(AnyError::from) }) } diff --git a/runtime/ops/process.rs b/runtime/ops/process.rs index e22ed3ac319c12..ab303e21040c0f 100644 --- a/runtime/ops/process.rs +++ b/runtime/ops/process.rs @@ -93,10 +93,7 @@ impl StdioOrRid { ) -> Result { match &self { StdioOrRid::Stdio(val) => Ok(val.as_stdio()), - StdioOrRid::Rid(rid) => { - let file = StdFileResource::clone_file(state, *rid)?; - Ok(file.into()) - } + StdioOrRid::Rid(rid) => StdFileResource::as_stdio(state, *rid), } } } diff --git a/runtime/ops/tty.rs b/runtime/ops/tty.rs index 7644d7567b1b1c..86ba54aad2bba2 100644 --- a/runtime/ops/tty.rs +++ b/runtime/ops/tty.rs @@ -1,7 +1,6 @@ // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. use super::io::StdFileResource; -use deno_core::error::bad_resource_id; use deno_core::error::AnyError; use deno_core::op; use deno_core::Extension; @@ -89,8 +88,8 @@ fn op_set_raw(state: &mut OpState, args: SetRawArgs) -> Result<(), AnyError> { return Err(deno_core::error::not_supported()); } - let std_file = resource.std_file()?; - let std_file = std_file.lock().unwrap(); // hold the lock + let std_file = resource.std_file(); + let std_file = std_file.lock(); // hold the lock let handle = std_file.as_raw_handle(); if handle == handleapi::INVALID_HANDLE_VALUE { @@ -164,25 +163,23 @@ fn op_set_raw(state: &mut OpState, args: SetRawArgs) -> Result<(), AnyError> { #[op] fn op_isatty(state: &mut OpState, rid: ResourceId) -> Result { - let isatty: bool = StdFileResource::with(state, rid, move |r| match r { - Ok(std_file) => { - #[cfg(windows)] - { - use winapi::um::consoleapi; - - let handle = get_windows_handle(std_file)?; - let mut test_mode: DWORD = 0; - // If I cannot get mode out of console, it is not a console. - Ok(unsafe { consoleapi::GetConsoleMode(handle, &mut test_mode) != 0 }) - } - #[cfg(unix)] - { - use std::os::unix::io::AsRawFd; - let raw_fd = std_file.as_raw_fd(); - Ok(unsafe { libc::isatty(raw_fd as libc::c_int) == 1 }) - } + let isatty: bool = StdFileResource::with_file(state, rid, move |std_file| { + #[cfg(windows)] + { + use winapi::shared::minwindef::FALSE; + use winapi::um::consoleapi; + + let handle = get_windows_handle(std_file)?; + let mut test_mode: DWORD = 0; + // If I cannot get mode out of console, it is not a console. + Ok(unsafe { consoleapi::GetConsoleMode(handle, &mut test_mode) != FALSE }) + } + #[cfg(unix)] + { + use std::os::unix::io::AsRawFd; + let raw_fd = std_file.as_raw_fd(); + Ok(unsafe { libc::isatty(raw_fd as libc::c_int) == 1 }) } - _ => Ok(false), })?; Ok(isatty) } @@ -200,52 +197,47 @@ fn op_console_size( ) -> Result { super::check_unstable(state, "Deno.consoleSize"); - let size = StdFileResource::with(state, rid, move |r| match r { - Ok(std_file) => { - #[cfg(windows)] - { - use std::os::windows::io::AsRawHandle; - let handle = std_file.as_raw_handle(); - - unsafe { - let mut bufinfo: winapi::um::wincon::CONSOLE_SCREEN_BUFFER_INFO = - std::mem::zeroed(); - - if winapi::um::wincon::GetConsoleScreenBufferInfo( - handle, - &mut bufinfo, - ) == 0 - { - return Err(Error::last_os_error().into()); - } - - Ok(ConsoleSize { - columns: bufinfo.dwSize.X as u32, - rows: bufinfo.dwSize.Y as u32, - }) + let size = StdFileResource::with_file(state, rid, move |std_file| { + #[cfg(windows)] + { + use std::os::windows::io::AsRawHandle; + let handle = std_file.as_raw_handle(); + + unsafe { + let mut bufinfo: winapi::um::wincon::CONSOLE_SCREEN_BUFFER_INFO = + std::mem::zeroed(); + + if winapi::um::wincon::GetConsoleScreenBufferInfo(handle, &mut bufinfo) + == 0 + { + return Err(Error::last_os_error().into()); } + + Ok(ConsoleSize { + columns: bufinfo.dwSize.X as u32, + rows: bufinfo.dwSize.Y as u32, + }) } + } - #[cfg(unix)] - { - use std::os::unix::io::AsRawFd; - - let fd = std_file.as_raw_fd(); - unsafe { - let mut size: libc::winsize = std::mem::zeroed(); - if libc::ioctl(fd, libc::TIOCGWINSZ, &mut size as *mut _) != 0 { - return Err(Error::last_os_error().into()); - } - - // TODO (caspervonb) return a tuple instead - Ok(ConsoleSize { - columns: size.ws_col as u32, - rows: size.ws_row as u32, - }) + #[cfg(unix)] + { + use std::os::unix::io::AsRawFd; + + let fd = std_file.as_raw_fd(); + unsafe { + let mut size: libc::winsize = std::mem::zeroed(); + if libc::ioctl(fd, libc::TIOCGWINSZ, &mut size as *mut _) != 0 { + return Err(Error::last_os_error().into()); } + + // TODO (caspervonb) return a tuple instead + Ok(ConsoleSize { + columns: size.ws_col as u32, + rows: size.ws_row as u32, + }) } } - Err(_) => Err(bad_resource_id()), })?; Ok(size) From 3f4b6b38f819b002e035220bfad5cdd3dd5602c6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 10 May 2022 13:31:22 -0400 Subject: [PATCH 3/7] Maybe fix unix. --- runtime/ops/tty.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/ops/tty.rs b/runtime/ops/tty.rs index 86ba54aad2bba2..22206cb5dd6b7e 100644 --- a/runtime/ops/tty.rs +++ b/runtime/ops/tty.rs @@ -119,7 +119,7 @@ fn op_set_raw(state: &mut OpState, args: SetRawArgs) -> Result<(), AnyError> { use std::os::unix::io::AsRawFd; let resource = state.resource_table.get::(rid)?; - let std_file = resource.std_file()?; + let std_file = resource.std_file(); let raw_fd = std_file.lock().unwrap().as_raw_fd(); let mut meta_data = resource.metadata_mut(); let maybe_tty_mode = &mut meta_data.tty.mode; From 374052be1cff70644eca3a779c90335de83b494c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 10 May 2022 13:40:05 -0400 Subject: [PATCH 4/7] Fix unix. --- runtime/ops/tty.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/ops/tty.rs b/runtime/ops/tty.rs index 22206cb5dd6b7e..ad152e2dab8e7a 100644 --- a/runtime/ops/tty.rs +++ b/runtime/ops/tty.rs @@ -120,7 +120,7 @@ fn op_set_raw(state: &mut OpState, args: SetRawArgs) -> Result<(), AnyError> { let resource = state.resource_table.get::(rid)?; let std_file = resource.std_file(); - let raw_fd = std_file.lock().unwrap().as_raw_fd(); + let raw_fd = std_file.lock().as_raw_fd(); let mut meta_data = resource.metadata_mut(); let maybe_tty_mode = &mut meta_data.tty.mode; From 1956717fc67432575e82f60d22e6d75682db4e86 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 10 May 2022 13:41:18 -0400 Subject: [PATCH 5/7] Revert accidentally committed section. --- cli/tests/integration/repl_tests.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cli/tests/integration/repl_tests.rs b/cli/tests/integration/repl_tests.rs index c4e03d65098568..44036795f5da5a 100644 --- a/cli/tests/integration/repl_tests.rs +++ b/cli/tests/integration/repl_tests.rs @@ -164,9 +164,7 @@ fn pty_complete_imports() { let output = console.read_all_output(); assert!(output.contains("Hello World")); if cfg!(windows) { - if !output.contains("testing output\u{1b}") { - panic!("Output did not contain expected:\n{:?}", output); - } + assert!(output.contains("testing output\u{1b}")); } else { assert!(output.contains("\ntesting output")); } From a201bc07bfb97925081ab079cf320b22fd314c97 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 11 May 2022 10:14:43 -0400 Subject: [PATCH 6/7] Add test. --- cli/tests/integration/repl_tests.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cli/tests/integration/repl_tests.rs b/cli/tests/integration/repl_tests.rs index 44036795f5da5a..0b69f8ebf6dc1e 100644 --- a/cli/tests/integration/repl_tests.rs +++ b/cli/tests/integration/repl_tests.rs @@ -206,6 +206,20 @@ fn pty_assign_global_this() { }); } +#[test] +fn pty_emoji() { + // windows was having issues displaying this + util::with_pty(&["repl"], |mut console| { + console.write_line("console.log('🦕');"); + console.write_line("close();"); + + let output = console.read_all_output(); + // one for input, one for output + let emoji_count = output.chars().filter(|c| *c == '🦕').count(); + assert_eq!(emoji_count, 2); + }); +} + #[test] fn console_log() { let (out, err) = util::run_and_collect_output( From cb21fe3f973394c5b9a01fb9d52ef2cf3fb1fc0f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 11 May 2022 11:04:41 -0400 Subject: [PATCH 7/7] Return error instead of panicking when writing/reading when not supported from stdio --- runtime/ops/io.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/runtime/ops/io.rs b/runtime/ops/io.rs index 8f979611778723..b43e42e328532d 100644 --- a/runtime/ops/io.rs +++ b/runtime/ops/io.rs @@ -18,6 +18,7 @@ use once_cell::sync::Lazy; use std::borrow::Cow; use std::cell::RefCell; use std::fs::File as StdFile; +use std::io::ErrorKind; use std::io::Read; use std::io::Write; use std::rc::Rc; @@ -343,8 +344,8 @@ impl StdFileResourceInner { impl Read for StdFileResourceInner { fn read(&mut self, buf: &mut [u8]) -> std::io::Result { match self { - Self::Stdout => unreachable!(), - Self::Stderr => unreachable!(), + Self::Stdout => Err(ErrorKind::Unsupported.into()), + Self::Stderr => Err(ErrorKind::Unsupported.into()), Self::Stdin => std::io::stdin().read(buf), Self::File(file) => file.lock().read(buf), } @@ -356,7 +357,7 @@ impl Write for StdFileResourceInner { match self { Self::Stdout => std::io::stdout().write(buf), Self::Stderr => std::io::stderr().write(buf), - Self::Stdin => unreachable!(), + Self::Stdin => Err(ErrorKind::Unsupported.into()), Self::File(file) => file.lock().write(buf), } } @@ -365,7 +366,7 @@ impl Write for StdFileResourceInner { match self { Self::Stdout => std::io::stdout().flush(), Self::Stderr => std::io::stderr().flush(), - Self::Stdin => unreachable!(), + Self::Stdin => Err(ErrorKind::Unsupported.into()), Self::File(file) => file.lock().flush(), } }