fix: stdout and stderr encoding on Windows (#14559)

1 files changed, 155 insertions, 65 deletions

diff --git a/runtime/ops/io.rs b/runtime/ops/io.rs
index 0fd282b9a..b43e42e32 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;
@@ -20,11 +18,11 @@ 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;
 use std::sync::Arc;
-use std::sync::Mutex;
 use tokio::io::AsyncRead;
 use tokio::io::AsyncReadExt;
 use tokio::io::AsyncWrite;
@@ -40,6 +38,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<StdFile> =
   Lazy::new(|| unsafe { StdFile::from_raw_fd(0) });
@@ -50,14 +51,6 @@ static STDOUT_HANDLE: Lazy<StdFile> =
 static STDERR_HANDLE: Lazy<StdFile> =
   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<StdFile> = Lazy::new(|| unsafe {
   StdFile::from_raw_handle(GetStdHandle(winbase::STD_INPUT_HANDLE))
@@ -122,23 +115,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 +294,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<Mutex<StdFile>>),
+}
+
+impl StdFileResourceInner {
+  pub fn file(fs_file: StdFile) -> Self {
+    StdFileResourceInner::File(Arc::new(Mutex::new(fs_file)))
+  }
+
+  pub fn with_file<R>(&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<usize, AnyError> {
+    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<usize> {
+    match self {
+      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),
+    }
+  }
+}
+
+impl Write for StdFileResourceInner {
+  fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
+    match self {
+      Self::Stdout => std::io::stdout().write(buf),
+      Self::Stderr => std::io::stderr().write(buf),
+      Self::Stdin => Err(ErrorKind::Unsupported.into()),
+      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 => Err(ErrorKind::Unsupported.into()),
+      Self::File(file) => file.lock().flush(),
+    }
+  }
+}
+
 pub struct StdFileResource {
-  fs_file: Option<Arc<Mutex<StdFile>>>,
+  inner: StdFileResourceInner,
   metadata: RefCell<FileMetadata>,
   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 +389,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<Arc<Mutex<StdFile>>, AnyError> {
-    match &self.fs_file {
-      Some(fs_file) => Ok(fs_file.clone()),
-      None => Err(bad_resource_id()),
+  pub fn std_file(&self) -> Arc<Mutex<StdFile>> {
+    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 +418,65 @@ impl StdFileResource {
     self: Rc<Self>,
     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<Self>, buf: ZeroCopyBuf) -> Result<usize, AnyError> {
-    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<F, R>(
+  fn with_inner<F, R>(
     state: &mut OpState,
     rid: ResourceId,
     mut f: F,
   ) -> Result<R, AnyError>
   where
-    F: FnMut(Result<&mut std::fs::File, ()>) -> Result<R, AnyError>,
+    F: FnMut(StdFileResourceInner) -> Result<R, AnyError>,
   {
     let resource = state.resource_table.get::<StdFileResource>(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<F, R>(
+    state: &mut OpState,
+    rid: ResourceId,
+    f: F,
+  ) -> Result<R, AnyError>
+  where
+    F: FnMut(&mut StdFile) -> Result<R, AnyError>,
+  {
+    let resource = state.resource_table.get::<StdFileResource>(rid)?;
+    resource.inner.with_file(f)
   }
 
   pub fn clone_file(
     state: &mut OpState,
     rid: ResourceId,
-  ) -> Result<std::fs::File, AnyError> {
-    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<StdFile, AnyError> {
+    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<std::process::Stdio, AnyError> {
+    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()),
     })
   }
 }
@@ -411,13 +506,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(())
   })
 }
 
@@ -427,12 +519,11 @@ fn op_read_sync(
   rid: ResourceId,
   mut buf: ZeroCopyBuf,
 ) -> Result<u32, AnyError> {
-  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)
   })
 }
 
@@ -442,11 +533,10 @@ fn op_write_sync(
   rid: ResourceId,
   buf: ZeroCopyBuf,
 ) -> Result<u32, AnyError> {
-  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)
   })
 }