Skip to content

Commit dec0a25

Browse files
WindSoilderWindSoilder
authored
Throw out error if external command in subexpression is failed to run (#8204)
1 parent 324d625 commit dec0a25

3 files changed

Lines changed: 128 additions & 19 deletions

File tree

‎crates/nu-command/tests/commands/run_external.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,17 @@ fn failed_command_with_semicolon_will_not_execute_following_cmds() {
139139
})
140140
}
141141

142+
#[test]
143+
fn semicolon_works_within_subcommand() {
144+
Playground::setup("external failed command with semicolon", |dirs, _| {
145+
let actual = nu!(
146+
cwd: dirs.test(), pipeline(r#"(nu --testbin outcome_err "aa"; echo done)"#
147+
));
148+
149+
assert!(!actual.out.contains("done"));
150+
})
151+
}
152+
142153
#[cfg(not(windows))]
143154
#[test]
144155
fn external_args_with_quoted() {

‎crates/nu-engine/src/eval.rs‎

Lines changed: 68 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -699,7 +699,14 @@ pub fn eval_expression_with_input(
699699
}
700700
};
701701

702-
Ok(might_consume_external_result(input))
702+
// Note: for `table` command, it mights returns `ExternalStream with stdout`
703+
// whatever `redirect_output` is true or false, so we only want to consume ExternalStream
704+
// if relative stdout is None.
705+
if let PipelineData::ExternalStream { stdout: None, .. } = input {
706+
Ok(might_consume_external_result(input))
707+
} else {
708+
Ok((input, false))
709+
}
703710
}
704711

705712
// Try to catch and detect if external command runs to failed.
@@ -1197,14 +1204,72 @@ pub fn eval_subexpression(
11971204
mut input: PipelineData,
11981205
) -> Result<PipelineData, ShellError> {
11991206
for pipeline in block.pipelines.iter() {
1200-
for expr in pipeline.elements.iter() {
1201-
input = eval_element_with_input(engine_state, stack, expr, input, true, false)?.0
1207+
for (expr_indx, expr) in pipeline.elements.iter().enumerate() {
1208+
if expr_indx != pipeline.elements.len() - 1 {
1209+
input = eval_element_with_input(engine_state, stack, expr, input, true, false)?.0;
1210+
} else {
1211+
// In subexpression, we always need to redirect stdout because the result is substituted to a Value.
1212+
//
1213+
// But we can check if external result is failed to run when it's the last expression
1214+
// in pipeline. e.g: (^false; echo aaa)
1215+
//
1216+
// If external command is failed to run, it can't be convert into value, in this case
1217+
// we throws out `ShellError::ExternalCommand`. And show it's stderr message information.
1218+
// In the case, we need to capture stderr first during eval.
1219+
input = eval_element_with_input(engine_state, stack, expr, input, true, true)?.0;
1220+
if matches!(input, PipelineData::ExternalStream { .. }) {
1221+
input = check_subexp_substitution(input)?;
1222+
}
1223+
}
12021224
}
12031225
}
12041226

12051227
Ok(input)
12061228
}
12071229

1230+
fn check_subexp_substitution(mut input: PipelineData) -> Result<PipelineData, ShellError> {
1231+
let consume_result = might_consume_external_result(input);
1232+
input = consume_result.0;
1233+
let failed_to_run = consume_result.1;
1234+
if let PipelineData::ExternalStream {
1235+
stdout,
1236+
stderr,
1237+
exit_code,
1238+
span,
1239+
metadata,
1240+
trim_end_newline,
1241+
} = input
1242+
{
1243+
let stderr_msg = match stderr {
1244+
None => "".to_string(),
1245+
Some(stderr_stream) => stderr_stream.into_string().map(|s| s.item)?,
1246+
};
1247+
if failed_to_run {
1248+
Err(ShellError::ExternalCommand(
1249+
"External command failed".to_string(),
1250+
stderr_msg,
1251+
span,
1252+
))
1253+
} else {
1254+
// we've captured stderr message, but it's running success.
1255+
// So we need to re-print stderr message out.
1256+
if !stderr_msg.is_empty() {
1257+
eprintln!("{stderr_msg}");
1258+
}
1259+
Ok(PipelineData::ExternalStream {
1260+
stdout,
1261+
stderr: None,
1262+
exit_code,
1263+
span,
1264+
metadata,
1265+
trim_end_newline,
1266+
})
1267+
}
1268+
} else {
1269+
Ok(input)
1270+
}
1271+
}
1272+
12081273
pub fn eval_variable(
12091274
engine_state: &EngineState,
12101275
stack: &Stack,

‎crates/nu-protocol/src/pipeline_data.rs‎

Lines changed: 49 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ impl PipelineData {
531531
// Only need ExternalStream without redirecting output.
532532
// It indicates we have no more commands to execute currently.
533533
if let PipelineData::ExternalStream {
534-
stdout: None,
534+
stdout,
535535
stderr,
536536
mut exit_code,
537537
span,
@@ -542,30 +542,63 @@ impl PipelineData {
542542
let exit_code = exit_code.take();
543543

544544
// Note:
545-
// In run-external's implementation detail, the result sender thread
546-
// send out stderr message first, then stdout message, then exit_code.
545+
// use a thread to receive stderr message.
546+
// Or we may get a deadlock if child process sends out too much bytes to stdout.
547547
//
548-
// In this clause, we already make sure that `stdout` is None
549-
// But not the case of `stderr`, so if `stderr` is not None
550-
// We need to consume stderr message before reading external commands' exit code.
551-
//
552-
// Or we'll never have a chance to read exit_code if stderr producer produce too much stderr message.
553-
// So we consume stderr stream and rebuild it.
554-
let stderr = stderr.map(|stderr_stream| {
555-
let stderr_ctrlc = stderr_stream.ctrlc.clone();
556-
let stderr_span = stderr_stream.span;
557-
let stderr_bytes = stderr_stream
548+
// For example: in normal linux system, stdout pipe's limit is 65535 bytes.
549+
// if child process sends out 65536 bytes, the process will be hanged because no consumer
550+
// consumes the first 65535 bytes
551+
// So we need a thread to receive stderr message, then the current thread can continue to consume
552+
// stdout messages.
553+
let stderr_handler = stderr.map(|stderr| {
554+
let stderr_span = stderr.span;
555+
let stderr_ctrlc = stderr.ctrlc.clone();
556+
(
557+
thread::Builder::new()
558+
.name("stderr consumer".to_string())
559+
.spawn(move || {
560+
stderr
561+
.into_bytes()
562+
.map(|bytes| bytes.item)
563+
.unwrap_or_default()
564+
})
565+
.expect("failed to create thread"),
566+
stderr_span,
567+
stderr_ctrlc,
568+
)
569+
});
570+
let stdout = stdout.map(|stdout_stream| {
571+
let stdout_ctrlc = stdout_stream.ctrlc.clone();
572+
let stdout_span = stdout_stream.span;
573+
let stdout_bytes = stdout_stream
558574
.into_bytes()
559575
.map(|bytes| bytes.item)
560576
.unwrap_or_default();
577+
RawStream::new(
578+
Box::new(vec![Ok(stdout_bytes)].into_iter()),
579+
stdout_ctrlc,
580+
stdout_span,
581+
None,
582+
)
583+
});
584+
let stderr = stderr_handler.map(|(handler, stderr_span, stderr_ctrlc)| {
585+
let stderr_bytes = handler
586+
.join()
587+
.map_err(|err| {
588+
ShellError::ExternalCommand(
589+
"Fail to receive external commands stderr message".to_string(),
590+
format!("{err:?}"),
591+
stderr_span,
592+
)
593+
})
594+
.unwrap_or_default();
561595
RawStream::new(
562596
Box::new(vec![Ok(stderr_bytes)].into_iter()),
563597
stderr_ctrlc,
564598
stderr_span,
565599
None,
566600
)
567601
});
568-
569602
match exit_code {
570603
Some(exit_code_stream) => {
571604
let ctrlc = exit_code_stream.ctrlc.clone();
@@ -578,7 +611,7 @@ impl PipelineData {
578611
}
579612
(
580613
PipelineData::ExternalStream {
581-
stdout: None,
614+
stdout,
582615
stderr,
583616
exit_code: Some(ListStream::from_stream(exit_code.into_iter(), ctrlc)),
584617
span,
@@ -590,7 +623,7 @@ impl PipelineData {
590623
}
591624
None => (
592625
PipelineData::ExternalStream {
593-
stdout: None,
626+
stdout,
594627
stderr,
595628
exit_code: None,
596629
span,

0 commit comments

Comments
 (0)