From c57207e96a577b7f13514fecdde5c47c7a497405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Fri, 8 Nov 2019 00:52:21 +0100 Subject: [PATCH] refactor: move Child resource to ops/process.rs (#3291) --- cli/ops/process.rs | 78 ++++++++++++++++++++++++----- cli/resources.rs | 120 ++++++++------------------------------------- core/resources.rs | 4 +- 3 files changed, 88 insertions(+), 114 deletions(-) diff --git a/cli/ops/process.rs b/cli/ops/process.rs index c6dd2f2d37..f7897ec519 100644 --- a/cli/ops/process.rs +++ b/cli/ops/process.rs @@ -1,15 +1,19 @@ // Copyright 2018-2019 the Deno authors. All rights reserved. MIT license. use super::dispatch_json::{Deserialize, JsonOp, Value}; +use crate::deno_error::bad_resource; use crate::ops::json_op; use crate::resources; +use crate::resources::CloneFileFuture; use crate::signal::kill; use crate::state::ThreadSafeState; use deno::*; use futures; use futures::Future; +use futures::Poll; use std; use std::convert::From; use std::process::Command; +use std::process::ExitStatus; use tokio_process::CommandExt; #[cfg(unix)] @@ -47,6 +51,12 @@ struct RunArgs { stderr_rid: u32, } +struct ChildResource { + child: tokio_process::Child, +} + +impl Resource for ChildResource {} + fn op_run( state: &ThreadSafeState, args: Value, @@ -73,40 +83,84 @@ fn op_run( // TODO: make this work with other resources, eg. sockets let stdin_rid = run_args.stdin_rid; if stdin_rid > 0 { - c.stdin(resources::get_file(stdin_rid)?); + let file = (CloneFileFuture { rid: stdin_rid }).wait()?.into_std(); + c.stdin(file); } else { c.stdin(subprocess_stdio_map(run_args.stdin.as_ref())); } let stdout_rid = run_args.stdout_rid; if stdout_rid > 0 { - c.stdout(resources::get_file(stdout_rid)?); + let file = (CloneFileFuture { rid: stdout_rid }).wait()?.into_std(); + c.stdout(file); } else { c.stdout(subprocess_stdio_map(run_args.stdout.as_ref())); } let stderr_rid = run_args.stderr_rid; if stderr_rid > 0 { - c.stderr(resources::get_file(stderr_rid)?); + let file = (CloneFileFuture { rid: stderr_rid }).wait()?.into_std(); + c.stderr(file); } else { c.stderr(subprocess_stdio_map(run_args.stderr.as_ref())); } // Spawn the command. - let child = c.spawn_async().map_err(ErrBox::from)?; - + let mut child = c.spawn_async().map_err(ErrBox::from)?; let pid = child.id(); - let resources = resources::add_child(child); + + let stdin_rid = if child.stdin().is_some() { + let rid = resources::add_child_stdin(child.stdin().take().unwrap()); + Some(rid) + } else { + None + }; + + let stdout_rid = if child.stdout().is_some() { + let rid = resources::add_child_stdout(child.stdout().take().unwrap()); + Some(rid) + } else { + None + }; + + let stderr_rid = if child.stderr().is_some() { + let rid = resources::add_child_stderr(child.stderr().take().unwrap()); + Some(rid) + } else { + None + }; + + let child_resource = ChildResource { child }; + let mut table = resources::lock_resource_table(); + let child_rid = table.add("child", Box::new(child_resource)); Ok(JsonOp::Sync(json!({ - "rid": resources.child_rid.unwrap(), + "rid": child_rid, "pid": pid, - "stdinRid": resources.stdin_rid, - "stdoutRid": resources.stdout_rid, - "stderrRid": resources.stderr_rid, + "stdinRid": stdin_rid, + "stdoutRid": stdout_rid, + "stderrRid": stderr_rid, }))) } +pub struct ChildStatus { + rid: ResourceId, +} + +impl Future for ChildStatus { + type Item = ExitStatus; + type Error = ErrBox; + + fn poll(&mut self) -> Poll { + let mut table = resources::lock_resource_table(); + let child_resource = table + .get_mut::(self.rid) + .ok_or_else(bad_resource)?; + let child = &mut child_resource.child; + child.poll().map_err(ErrBox::from) + } +} + #[derive(Deserialize)] #[serde(rename_all = "camelCase")] struct RunStatusArgs { @@ -123,7 +177,7 @@ fn op_run_status( state.check_run()?; - let future = resources::child_status(rid)?; + let future = ChildStatus { rid }; let future = future.and_then(move |run_status| { let code = run_status.code(); @@ -139,7 +193,7 @@ fn op_run_status( let got_signal = signal.is_some(); futures::future::ok(json!({ - "gotSignal": got_signal, + "gotSignal": got_signal, "exitCode": code.unwrap_or(-1), "exitSignal": signal.unwrap_or(-1), })) diff --git a/cli/resources.rs b/cli/resources.rs index 2e6306761b..db9b43eeb1 100644 --- a/cli/resources.rs +++ b/cli/resources.rs @@ -20,7 +20,6 @@ use futures::Future; use futures::Poll; use reqwest::r#async::Decoder as ReqwestDecoder; use std; -use std::process::ExitStatus; use std::sync::Mutex; use std::sync::MutexGuard; use tokio; @@ -62,7 +61,7 @@ lazy_static! { }); } -// TODO: move listeners out of this enum and rename to `StreamResource` +// TODO: rename to `StreamResource` pub enum CliResource { Stdin(tokio::io::Stdin), Stdout(tokio::fs::File), @@ -72,10 +71,6 @@ pub enum CliResource { ServerTlsStream(Box>), ClientTlsStream(Box>), HttpBody(HttpBody), - // Enum size is bounded by the largest variant. - // Use `Box` around large `Child` struct. - // https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant - Child(Box), ChildStdin(tokio_process::ChildStdin), ChildStdout(tokio_process::ChildStdout), ChildStderr(tokio_process::ChildStderr), @@ -176,112 +171,39 @@ pub fn add_reqwest_body(body: ReqwestDecoder) -> ResourceId { table.add("httpBody", Box::new(CliResource::HttpBody(body))) } -pub struct ChildResources { - pub child_rid: Option, - pub stdin_rid: Option, - pub stdout_rid: Option, - pub stderr_rid: Option, -} - -pub fn add_child(mut child: tokio_process::Child) -> ChildResources { +pub fn add_child_stdin(stdin: tokio_process::ChildStdin) -> ResourceId { let mut table = lock_resource_table(); - - let mut resources = ChildResources { - child_rid: None, - stdin_rid: None, - stdout_rid: None, - stderr_rid: None, - }; - - if child.stdin().is_some() { - let stdin = child.stdin().take().unwrap(); - let rid = table.add("childStdin", Box::new(CliResource::ChildStdin(stdin))); - resources.stdin_rid = Some(rid); - } - if child.stdout().is_some() { - let stdout = child.stdout().take().unwrap(); - let rid = - table.add("childStdout", Box::new(CliResource::ChildStdout(stdout))); - resources.stdout_rid = Some(rid); - } - if child.stderr().is_some() { - let stderr = child.stderr().take().unwrap(); - let rid = - table.add("childStderr", Box::new(CliResource::ChildStderr(stderr))); - resources.stderr_rid = Some(rid); - } - - let rid = table.add("child", Box::new(CliResource::Child(Box::new(child)))); - resources.child_rid = Some(rid); - - resources + table.add("childStdin", Box::new(CliResource::ChildStdin(stdin))) } -pub struct ChildStatus { - rid: ResourceId, +pub fn add_child_stdout(stdout: tokio_process::ChildStdout) -> ResourceId { + let mut table = lock_resource_table(); + table.add("childStdout", Box::new(CliResource::ChildStdout(stdout))) } -// Invert the dumbness that tokio_process causes by making Child itself a future. -impl Future for ChildStatus { - type Item = ExitStatus; +pub fn add_child_stderr(stderr: tokio_process::ChildStderr) -> ResourceId { + let mut table = lock_resource_table(); + table.add("childStderr", Box::new(CliResource::ChildStderr(stderr))) +} + +pub struct CloneFileFuture { + pub rid: ResourceId, +} + +impl Future for CloneFileFuture { + type Item = tokio::fs::File; type Error = ErrBox; - fn poll(&mut self) -> Poll { + fn poll(&mut self) -> Poll { let mut table = lock_resource_table(); let repr = table .get_mut::(self.rid) .ok_or_else(bad_resource)?; match repr { - CliResource::Child(ref mut child) => child.poll().map_err(ErrBox::from), + CliResource::FsFile(ref mut file) => { + file.poll_try_clone().map_err(ErrBox::from) + } _ => Err(bad_resource()), } } } - -pub fn child_status(rid: ResourceId) -> Result { - let mut table = lock_resource_table(); - let maybe_repr = - table.get_mut::(rid).ok_or_else(bad_resource)?; - match maybe_repr { - CliResource::Child(ref mut _child) => Ok(ChildStatus { rid }), - _ => Err(bad_resource()), - } -} - -// TODO: revamp this after the following lands: -// https://github.com/tokio-rs/tokio/pull/785 -pub fn get_file(rid: ResourceId) -> Result { - let mut table = lock_resource_table(); - // We take ownership of File here. - // It is put back below while still holding the lock. - let (_name, repr) = table.map.remove(&rid).ok_or_else(bad_resource)?; - let repr = repr - .downcast::() - .or_else(|_| Err(bad_resource()))?; - - match *repr { - CliResource::FsFile(r) => { - // Trait Clone not implemented on tokio::fs::File, - // so convert to std File first. - let std_file = r.into_std(); - // Create a copy and immediately put back. - // We don't want to block other resource ops. - // try_clone() would yield a copy containing the same - // underlying fd, so operations on the copy would also - // affect the one in resource table, and we don't need - // to write back. - let maybe_std_file_copy = std_file.try_clone(); - // Insert the entry back with the same rid. - table.map.insert( - rid, - ( - "fsFile".to_string(), - Box::new(CliResource::FsFile(tokio_fs::File::from_std(std_file))), - ), - ); - - maybe_std_file_copy.map_err(ErrBox::from) - } - _ => Err(bad_resource()), - } -} diff --git a/core/resources.rs b/core/resources.rs index a66fb91207..da4fb6b078 100644 --- a/core/resources.rs +++ b/core/resources.rs @@ -21,9 +21,7 @@ type ResourceMap = HashMap)>; #[derive(Default)] pub struct ResourceTable { - // TODO(bartlomieju): remove pub modifier, it is used by - // `get_file` method in CLI - pub map: ResourceMap, + map: ResourceMap, next_id: u32, }