Conversation
| @@ -212,7 +215,7 @@ impl<'js> ChildProcess<'js> { | |||
| } | |||
|
|
|||
| impl<'js> ChildProcess<'js> { | |||
There was a problem hiding this comment.
This file is getting very large, we should start splitting it up.
| Ok(instance) | ||
| } | ||
|
|
||
| fn exec_file( |
There was a problem hiding this comment.
There is a lot of duplicate code with the spawn function, I am wondering if there are some missing helpers to add?
There was a problem hiding this comment.
Agree. execFile can build on exec which can build on spawn. They're just higher level APIs. execFile doesn't spawn a shell
| ChildProcess::spawn(ctx.clone(), cmd, command_args, command.spawn()) | ||
| } | ||
|
|
||
| fn exec_file<'js>( |
There was a problem hiding this comment.
The refactor of spawn to accommodate exec_file is good. Let's split the helpers in another file (I would split functions together, the class on its own and the helpers together).
|
|
||
| fn get_callback_fn<'js>( | ||
| ctx: &Ctx<'js>, | ||
| args: Vec<Option<&Value<'js>>>, |
There was a problem hiding this comment.
we should be able to avoid a vec allocation here I am pretty sure &[Option<&Value<'js>>] should work
| fn exec_file<'js>( | ||
| ctx: Ctx<'js>, | ||
| cmd: String, | ||
| args_and_opts: Rest<Value<'js>>, |
There was a problem hiding this comment.
I don't like this style of arguments, this execFile and spawn don't have variadic arguments.
I know this is the style of @richarddavison :D
At minimum you can write:
fn exec_file<'js>(
ctx: Ctx<'js>,
cmd: String,
args_0: Opt<Value<'js>>,
args_1: Opt<Value<'js>>,
args_2: Opt<Value<'js>>,
)Even better would be to start using proper typing directly in the arguments (this is approximate code):
fn exec_file<'js>(
ctx: Ctx<'js>,
cmd: String,
args_0: Opt<Either<Either<Array<'js>, Object<'js>, Function<'js>>>>, // I wouldnt againt be an Either3 implementation
args_1: Opt<Either<Object<'js>, Function<'js>>,
args_2: Opt<Function<'js>>,
)Opt is different from Option since it doesnt require undefined to be passed to the function
There was a problem hiding this comment.
Agree. We can do Either to clean it up. Similar approach can be added in spawn
modules/llrt_stream/src/readable.rs
Outdated
| if let Some(listener) = listener { | ||
| if listener == "data" { | ||
| Self::emit_str( | ||
| This(this2.clone()), | ||
| &ctx3, | ||
| "data", | ||
| vec![Buffer(buffer.clone()).into_js(&ctx3)?], | ||
| false | ||
| )?; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a change in behaviour for which I not qualified to comment, @richarddavison
modules/llrt_stream/src/readable.rs
Outdated
| combined_std_buffer: Option<Arc<Mutex<Vec<u8>>>>, | ||
| cb: Option<Function<'js>>, |
There was a problem hiding this comment.
Is there a case where we would want to use a combined_std_buffer independently from cb? All usages in this PR assume that cb and combined_std_buffer are either some or none.
I am not really a want of adding it to the stream, I wonder if we could instead either pipe the stdout/err through an accumulation middleware before forwarding it to the DefaultReadableStream.
| let stdout_arc = Arc::new(Mutex::new(stdout_new)); | ||
| let stderr_arc = Arc::new(Mutex::new(stderr_new)); | ||
| let combined_stdout_buffer = Some(Arc::clone(&stdout_arc)); | ||
| let combined_stderr_buffer = Some(Arc::clone(&stderr_arc)); |
There was a problem hiding this comment.
As far as I can see, we don't use None on this value anywhere.
Also depending on the usecase, it is usually fine to use a RefCell since the JS engine is single-threaded. As long as you don't hold the borrow across an await point or a callback to JS (anything that yields control back to the JS engine really).
|
For reference, the node implementation uses spawn under the cover: https://github.com/nodejs/node/blob/1720b18260d7f4bbd9fe0d2cbbbb05cc33e7f945/lib/child_process.js#L323 and they do in fact set a listener on |
richarddavison
left a comment
richarddavison
left a comment
There was a problem hiding this comment.
Thanks for the PR! @Sytten covered most points. Mainly we should build exec and execFile upon spawn
|
Thank you @Sytten and @richarddavison for the review. I will refactor it to build |
|
@nithinkjoy-tech Do you plan on finishing the PR? |
|
|
@richarddavison Do you want to take that on or not? |
Implements exec and execFile on top of spawn following the Node.js pattern, as suggested in PR awslabs#949 review feedback. Uses data event listeners to accumulate stdout/stderr output. Closes awslabs#818
3 participants
Issue
Closes #818
Description of changes
Added execFile support to the child_process module.
Checklist
tests/unitand/or in Rust for my feature if neededmake fixto format JS and apply Clippy auto fixesmake checkBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.