From 11c13fb9819a479a5a69278fff3703cf893d3df1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 10 May 2022 10:13:08 -0400 Subject: [PATCH] refactor: remove unused `Option`s on `StdFileResource.fs_file` (#14549) --- runtime/ops/fs.rs | 68 ++++++++++------------------------------ runtime/ops/io.rs | 77 ++++++++++++++++++++-------------------------- runtime/ops/tty.rs | 39 +++++------------------ 3 files changed, 57 insertions(+), 127 deletions(-) diff --git a/runtime/ops/fs.rs b/runtime/ops/fs.rs index 99b37916dc..2bcd6cacca 100644 --- a/runtime/ops/fs.rs +++ b/runtime/ops/fs.rs @@ -4,7 +4,6 @@ use super::io::StdFileResource; use super::utils::into_string; use crate::fs_util::canonicalize_path; use crate::permissions::Permissions; -use deno_core::error::bad_resource_id; use deno_core::error::custom_error; use deno_core::error::type_error; use deno_core::error::AnyError; @@ -24,11 +23,16 @@ use serde::Serialize; use std::borrow::Cow; use std::cell::RefCell; use std::convert::From; -use std::env::{current_dir, set_current_dir, temp_dir}; +use std::env::current_dir; +use std::env::set_current_dir; +use std::env::temp_dir; use std::io; +use std::io::Error; +use std::io::Seek; +use std::io::SeekFrom; use std::io::Write; -use std::io::{Error, Seek, SeekFrom}; -use std::path::{Path, PathBuf}; +use std::path::Path; +use std::path::PathBuf; use std::rc::Rc; use std::time::SystemTime; use std::time::UNIX_EPOCH; @@ -339,12 +343,7 @@ async fn op_seek_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; tokio::task::spawn_blocking(move || { let mut std_file = std_file.lock().unwrap(); @@ -376,12 +375,7 @@ async fn op_fdatasync_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; tokio::task::spawn_blocking(move || { let std_file = std_file.lock().unwrap(); @@ -410,12 +404,7 @@ async fn op_fsync_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; tokio::task::spawn_blocking(move || { let std_file = std_file.lock().unwrap(); @@ -447,12 +436,7 @@ async fn op_fstat_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; let metadata = tokio::task::spawn_blocking(move || { let std_file = std_file.lock().unwrap(); @@ -499,12 +483,7 @@ async fn op_flock_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; tokio::task::spawn_blocking(move || -> Result<(), AnyError> { let std_file = std_file.lock().unwrap(); @@ -548,12 +527,7 @@ async fn op_funlock_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; tokio::task::spawn_blocking(move || -> Result<(), AnyError> { let std_file = std_file.lock().unwrap(); @@ -1636,12 +1610,7 @@ async fn op_ftruncate_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; tokio::task::spawn_blocking(move || { let std_file = std_file.lock().unwrap(); @@ -1938,12 +1907,7 @@ async fn op_futime_async( .resource_table .get::(rid)?; - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let fs_file = resource.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().clone(); + let std_file = resource.std_file()?; tokio::task::spawn_blocking(move || { let std_file = std_file.lock().unwrap(); filetime::set_file_handle_times(&std_file, Some(atime), Some(mtime))?; diff --git a/runtime/ops/io.rs b/runtime/ops/io.rs index a357dd6f1a..709d2fb3ea 100644 --- a/runtime/ops/io.rs +++ b/runtime/ops/io.rs @@ -301,11 +301,10 @@ impl Resource for ChildStderrResource { } } -type MaybeSharedStdFile = Option>>; - #[derive(Default)] pub struct StdFileResource { - pub fs_file: Option<(MaybeSharedStdFile, Option>)>, + fs_file: Option>>, + metadata: RefCell, cancel: CancelHandle, name: String, } @@ -313,10 +312,8 @@ pub struct StdFileResource { impl StdFileResource { pub fn stdio(std_file: &StdFile, name: &str) -> Self { Self { - fs_file: Some(( - std_file.try_clone().map(|s| Arc::new(Mutex::new(s))).ok(), - Some(RefCell::new(FileMetadata::default())), - )), + fs_file: std_file.try_clone().map(|s| Arc::new(Mutex::new(s))).ok(), + metadata: RefCell::new(FileMetadata::default()), name: name.to_string(), ..Default::default() } @@ -324,47 +321,46 @@ impl StdFileResource { pub fn fs_file(fs_file: StdFile) -> Self { Self { - fs_file: Some(( - Some(Arc::new(Mutex::new(fs_file))), - Some(RefCell::new(FileMetadata::default())), - )), + fs_file: Some(Arc::new(Mutex::new(fs_file))), + metadata: RefCell::new(FileMetadata::default()), name: "fsFile".to_string(), ..Default::default() } } + 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 metadata_mut(&self) -> std::cell::RefMut { + self.metadata.borrow_mut() + } + async fn read( self: Rc, mut buf: ZeroCopyBuf, ) -> Result<(usize, ZeroCopyBuf), AnyError> { - if self.fs_file.is_some() { - let fs_file = self.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.as_ref().unwrap().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)) - }, - ) - .await? - } else { - Err(resource_unavailable()) - } + let std_file = self.fs_file.as_ref().unwrap().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)) + }, + ) + .await? } async fn write(self: Rc, buf: ZeroCopyBuf) -> Result { - if self.fs_file.is_some() { - let fs_file = self.fs_file.as_ref().unwrap(); - let std_file = fs_file.0.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) - } else { - Err(resource_unavailable()) - } + 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) } pub fn with( @@ -376,15 +372,8 @@ impl StdFileResource { F: FnMut(Result<&mut std::fs::File, ()>) -> Result, { let resource = state.resource_table.get::(rid)?; - // TODO(@AaronO): does this make sense ? - // Sync write only works for FsFile. It doesn't make sense to do this - // for non-blocking sockets. So we error out if not FsFile. - if resource.fs_file.is_none() { - return f(Err(())); - } - let (r, _) = resource.fs_file.as_ref().unwrap(); - match r { + match &resource.fs_file { Some(r) => f(Ok(&mut r.as_ref().lock().unwrap())), None => Err(resource_unavailable()), } diff --git a/runtime/ops/tty.rs b/runtime/ops/tty.rs index 789847697f..7644d7567b 100644 --- a/runtime/ops/tty.rs +++ b/runtime/ops/tty.rs @@ -2,8 +2,6 @@ use super::io::StdFileResource; 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::Extension; @@ -88,23 +86,12 @@ fn op_set_raw(state: &mut OpState, args: SetRawArgs) -> Result<(), AnyError> { let resource = state.resource_table.get::(rid)?; if cbreak { - return Err(not_supported()); + return Err(deno_core::error::not_supported()); } - if resource.fs_file.is_none() { - return Err(bad_resource_id()); - } - - let handle_result = if let Some((fs_file, _)) = &resource.fs_file { - let file = fs_file.as_ref().unwrap().clone(); - let std_file = file.lock().unwrap(); - let raw_handle = std_file.as_raw_handle(); - Ok(raw_handle) - } else { - Err(resource_unavailable()) - }; - - let handle = handle_result?; + let std_file = resource.std_file()?; + let std_file = std_file.lock().unwrap(); // hold the lock + let handle = std_file.as_raw_handle(); if handle == handleapi::INVALID_HANDLE_VALUE { return Err(Error::last_os_error().into()); @@ -133,20 +120,10 @@ fn op_set_raw(state: &mut OpState, args: SetRawArgs) -> Result<(), AnyError> { use std::os::unix::io::AsRawFd; let resource = state.resource_table.get::(rid)?; - - if resource.fs_file.is_none() { - return Err(not_supported()); - } - - let (fs_file, meta) = - resource.fs_file.as_ref().ok_or_else(resource_unavailable)?; - - if fs_file.is_none() { - return Err(resource_unavailable()); - } - - let raw_fd = fs_file.as_ref().unwrap().lock().unwrap().as_raw_fd(); - let maybe_tty_mode = &mut meta.as_ref().unwrap().borrow_mut().tty.mode; + 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; if is_raw { if maybe_tty_mode.is_none() {