From b150f9f5d897e0183b066bc99e48c056aabe896a Mon Sep 17 00:00:00 2001 From: WindSoilder Date: Thu, 18 May 2023 06:47:03 +0800 Subject: [PATCH] Avoid blocking when `o+e>` redirects too much stderr message (#8784) # Description Fixes: #8565 Here is another pr #7240 tried to address the issue, but it works in a wrong way. After this change `o+e>` won't redirect all stdout message then stderr message and it works more like how bash does. # User-Facing Changes For the given python code: ```python # test.py import sys print('aa'*300, flush=True) print('bb'*999999, file=sys.stderr, flush=True) print('cc'*300, flush=True) ``` Running `python test.py out+err> a.txt` shoudn't hang nushell, and `a.txt` keeps output in the same order ## About the change The core idea is that when doing lite-parsing, introduce a new variant `LiteElement::SameTargetRedirection` if we meet `out+err>` redirection token(which is generated by lex function), During converting from lite block to block, LiteElement::SameTargetRedirection will be converted to PipelineElement::SameTargetRedirection. Then in the block eval process, if we get PipelineElement::SameTargetRedirection, we'll invoke `run-external` with `--redirect-combine` flag, then pipe the result into save command ## What happened internally? Take the following command as example: `^ls o+e> log.txt` lex parsing result(`Tokens`) are not changed, but `LiteBlock` and `Block` is changed after this pr. ### LiteBlock before ```rust LiteBlock { block: [ LitePipeline { commands: [ Command(None, LiteCommand { comments: [], parts: [Span { start: 39041, end: 39044 }] }), // actually the span of first Redirection is wrong too.. Redirection(Span { start: 39058, end: 39062 }, StdoutAndStderr, LiteCommand { comments: [], parts: [Span { start: 39050, end: 39057 }] }), ] }] } ``` ### LiteBlock after ```rust LiteBlock { block: [ LitePipeline { commands: [ SameTargetRedirection { cmd: (None, LiteCommand { comments: [], parts: [Span { start: 147945, end: 147948}]}), redirection: (Span { start: 147949, end: 147957 }, LiteCommand { comments: [], parts: [Span { start: 147958, end: 147965 }]}) } ] } ] } ``` ### Block before ```rust Pipeline { elements: [ Expression(None, Expression { expr: ExternalCall(Expression { expr: String("ls"), span: Span { start: 39042, end: 39044 }, ty: String, custom_completion: None }, [], false), span: Span { start: 39041, end: 39044 }, ty: Any, custom_completion: None }), Redirection(Span { start: 39058, end: 39062 }, StdoutAndStderr, Expression { expr: String("out.txt"), span: Span { start: 39050, end: 39057 }, ty: String, custom_completion: None })] } ``` ### Block after ```rust Pipeline { elements: [ SameTargetRedirection { cmd: (None, Expression { expr: ExternalCall(Expression { expr: String("ls"), span: Span { start: 147946, end: 147948 }, ty: String, custom_completion: None}, [], false), span: Span { start: 147945, end: 147948}, ty: Any, custom_completion: None }), redirection: (Span { start: 147949, end: 147957}, Expression {expr: String("log.txt"), span: Span { start: 147958, end: 147965 },ty: String,custom_completion: None} } ] } ``` # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass - `cargo run -- crates/nu-utils/standard_library/tests.nu` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-cli/src/completions/completer.rs | 1 + crates/nu-cli/src/syntax_highlight.rs | 1 + crates/nu-command/src/formats/from/nuon.rs | 4 + .../nu-command/tests/commands/redirection.rs | 37 ++++- crates/nu-engine/src/eval.rs | 140 ++++++++++-------- crates/nu-parser/src/flatten.rs | 15 ++ crates/nu-parser/src/lite_parser.rs | 40 +++++ crates/nu-parser/src/parse_keywords.rs | 3 + crates/nu-parser/src/parser.rs | 61 +++++++- crates/nu-protocol/src/ast/pipeline.rs | 38 ++++- 10 files changed, 265 insertions(+), 75 deletions(-) diff --git a/crates/nu-cli/src/completions/completer.rs b/crates/nu-cli/src/completions/completer.rs index eb32088536..bbcac9d131 100644 --- a/crates/nu-cli/src/completions/completer.rs +++ b/crates/nu-cli/src/completions/completer.rs @@ -128,6 +128,7 @@ impl NuCompleter { | PipelineElement::Redirection(_, _, expr) | PipelineElement::And(_, expr) | PipelineElement::Or(_, expr) + | PipelineElement::SameTargetRedirection { cmd: (_, expr), .. } | PipelineElement::SeparateRedirection { out: (_, expr), .. } => { let flattened: Vec<_> = flatten_expression(&working_set, &expr); let mut spans: Vec = vec![]; diff --git a/crates/nu-cli/src/syntax_highlight.rs b/crates/nu-cli/src/syntax_highlight.rs index bec6fff28a..ef93ba2f7a 100644 --- a/crates/nu-cli/src/syntax_highlight.rs +++ b/crates/nu-cli/src/syntax_highlight.rs @@ -237,6 +237,7 @@ fn find_matching_block_end_in_block( | PipelineElement::Redirection(_, _, e) | PipelineElement::And(_, e) | PipelineElement::Or(_, e) + | PipelineElement::SameTargetRedirection { cmd: (_, e), .. } | PipelineElement::SeparateRedirection { out: (_, e), .. } => { if e.span.contains(global_cursor_offset) { if let Some(pos) = find_matching_block_end_in_expr( diff --git a/crates/nu-command/src/formats/from/nuon.rs b/crates/nu-command/src/formats/from/nuon.rs index 7c3fa14c52..9a040881b8 100644 --- a/crates/nu-command/src/formats/from/nuon.rs +++ b/crates/nu-command/src/formats/from/nuon.rs @@ -136,6 +136,10 @@ impl Command for FromNuon { | PipelineElement::Redirection(_, _, expression) | PipelineElement::And(_, expression) | PipelineElement::Or(_, expression) + | PipelineElement::SameTargetRedirection { + cmd: (_, expression), + .. + } | PipelineElement::SeparateRedirection { out: (_, expression), .. diff --git a/crates/nu-command/tests/commands/redirection.rs b/crates/nu-command/tests/commands/redirection.rs index 115d08f23a..c4b0c02b9a 100644 --- a/crates/nu-command/tests/commands/redirection.rs +++ b/crates/nu-command/tests/commands/redirection.rs @@ -32,10 +32,11 @@ fn redirect_err() { #[test] fn redirect_outerr() { Playground::setup("redirect_outerr_test", |dirs, _sandbox| { - let output = nu!( + nu!( cwd: dirs.test(), - "cat asdfasdfasdf.txt out+err> a; cat a" + "cat asdfasdfasdf.txt out+err> a" ); + let output = nu!(cwd: dirs.test(), "cat a"); assert!(output.out.contains("asdfasdfasdf.txt")); }) @@ -45,10 +46,11 @@ fn redirect_outerr() { #[test] fn redirect_outerr() { Playground::setup("redirect_outerr_test", |dirs, _sandbox| { - let output = nu!( + nu!( cwd: dirs.test(), - "vol missingdrive out+err> a; (open a | size).bytes >= 16" + "vol missingdrive out+err> a" ); + let output = nu!(cwd: dirs.test(), "(open a | size).bytes >= 16"); assert!(output.out.contains("true")); }) @@ -126,6 +128,33 @@ fn separate_redirection() { ) } +#[test] +fn same_target_redirection_with_too_much_stderr_not_hang_nushell() { + use nu_test_support::pipeline; + use nu_test_support::playground::Playground; + Playground::setup("external with many stderr message", |dirs, sandbox| { + let bytes: usize = 81920; + let mut large_file_body = String::with_capacity(bytes); + for _ in 0..bytes { + large_file_body.push('a'); + } + sandbox.with_files(vec![FileWithContent("a_large_file.txt", &large_file_body)]); + + nu!( + cwd: dirs.test(), pipeline( + r#" + let-env LARGE = (open --raw a_large_file.txt); + nu --testbin echo_env_stderr LARGE out+err> another_large_file.txt + "# + ), + ); + + let expected_file = dirs.test().join("another_large_file.txt"); + let actual = file_contents(expected_file); + assert_eq!(actual, format!("{large_file_body}\n")); + }) +} + #[test] fn redirection_keep_exit_codes() { Playground::setup( diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index f48791d1f1..8e0fadf8dd 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -191,6 +191,11 @@ pub fn redirect_env(engine_state: &EngineState, caller_stack: &mut Stack, callee } } +enum RedirectTarget { + Piped(bool, bool), + CombinedPipe, +} + #[allow(clippy::too_many_arguments)] fn eval_external( engine_state: &EngineState, @@ -198,8 +203,7 @@ fn eval_external( head: &Expression, args: &[Expression], input: PipelineData, - redirect_stdout: bool, - redirect_stderr: bool, + redirect_target: RedirectTarget, is_subexpression: bool, ) -> Result { let decl_id = engine_state @@ -216,26 +220,38 @@ fn eval_external( call.add_positional(arg.clone()) } - if redirect_stdout { - call.add_named(( - Spanned { - item: "redirect-stdout".into(), - span: head.span, - }, - None, - None, - )) - } + match redirect_target { + RedirectTarget::Piped(redirect_stdout, redirect_stderr) => { + if redirect_stdout { + call.add_named(( + Spanned { + item: "redirect-stdout".into(), + span: head.span, + }, + None, + None, + )) + } - if redirect_stderr { - call.add_named(( + if redirect_stderr { + call.add_named(( + Spanned { + item: "redirect-stderr".into(), + span: head.span, + }, + None, + None, + )) + } + } + RedirectTarget::CombinedPipe => call.add_named(( Spanned { - item: "redirect-stderr".into(), + item: "redirect-combine".into(), span: head.span, }, None, None, - )) + )), } if is_subexpression { @@ -332,8 +348,7 @@ pub fn eval_expression( head, args, PipelineData::empty(), - false, - false, + RedirectTarget::Piped(false, false), *is_subexpression, )? .into_value(span)) @@ -698,8 +713,7 @@ pub fn eval_expression_with_input( head, args, input, - redirect_stdout, - redirect_stderr, + RedirectTarget::Piped(redirect_stdout, redirect_stderr), *is_subexpression, )?; } @@ -792,50 +806,6 @@ pub fn eval_element_with_input( metadata, trim_end_newline, }, - ( - Redirection::StdoutAndStderr, - PipelineData::ExternalStream { - stdout, - stderr, - exit_code, - span, - metadata, - trim_end_newline, - }, - ) => match (stdout, stderr) { - (Some(stdout), Some(stderr)) => PipelineData::ExternalStream { - stdout: Some(stdout.chain(stderr)), - stderr: None, - exit_code, - span, - metadata, - trim_end_newline, - }, - (None, Some(stderr)) => PipelineData::ExternalStream { - stdout: Some(stderr), - stderr: None, - exit_code, - span, - metadata, - trim_end_newline, - }, - (Some(stdout), None) => PipelineData::ExternalStream { - stdout: Some(stdout), - stderr: None, - exit_code, - span, - metadata, - trim_end_newline, - }, - (None, None) => PipelineData::ExternalStream { - stdout: None, - stderr: None, - exit_code, - span, - metadata, - trim_end_newline, - }, - }, (_, input) => input, }; @@ -970,6 +940,48 @@ pub fn eval_element_with_input( } } }, + PipelineElement::SameTargetRedirection { + cmd: (cmd_span, cmd_exp), + redirection: (redirect_span, redirect_exp), + } => { + // general idea: eval cmd and call save command to redirect stdout to result. + input = match &cmd_exp.expr { + Expr::ExternalCall(head, args, is_subexpression) => { + // if cmd's expression is ExternalStream, then invoke run-external with + // special --redirect-combine flag. + eval_external( + engine_state, + stack, + head, + args, + input, + RedirectTarget::CombinedPipe, + *is_subexpression, + )? + } + _ => eval_element_with_input( + engine_state, + stack, + &PipelineElement::Expression(*cmd_span, cmd_exp.clone()), + input, + redirect_stdout, + redirect_stderr, + ) + .map(|x| x.0)?, + }; + eval_element_with_input( + engine_state, + stack, + &PipelineElement::Redirection( + *redirect_span, + Redirection::Stdout, + redirect_exp.clone(), + ), + input, + redirect_stdout, + redirect_stderr, + ) + } PipelineElement::And(_, expr) => eval_expression_with_input( engine_state, stack, diff --git a/crates/nu-parser/src/flatten.rs b/crates/nu-parser/src/flatten.rs index 8ffd354634..b68a8a037e 100644 --- a/crates/nu-parser/src/flatten.rs +++ b/crates/nu-parser/src/flatten.rs @@ -532,6 +532,21 @@ pub fn flatten_pipeline_element( output.append(&mut flatten_expression(working_set, err_expr)); output } + PipelineElement::SameTargetRedirection { + cmd: (cmd_span, cmd_expr), + redirection: (redirect_span, redirect_expr), + } => { + let mut output = if let Some(span) = cmd_span { + let mut output = vec![(*span, FlatShape::Pipe)]; + output.append(&mut flatten_expression(working_set, cmd_expr)); + output + } else { + flatten_expression(working_set, cmd_expr) + }; + output.push((*redirect_span, FlatShape::Redirection)); + output.append(&mut flatten_expression(working_set, redirect_expr)); + output + } PipelineElement::And(span, expr) => { let mut output = vec![(*span, FlatShape::And)]; output.append(&mut flatten_expression(working_set, expr)); diff --git a/crates/nu-parser/src/lite_parser.rs b/crates/nu-parser/src/lite_parser.rs index 8c1f563d9a..53b495b646 100644 --- a/crates/nu-parser/src/lite_parser.rs +++ b/crates/nu-parser/src/lite_parser.rs @@ -43,6 +43,11 @@ pub enum LiteElement { out: (Span, LiteCommand), err: (Span, LiteCommand), }, + // SameTargetRedirection variant can only be generated by Command with Redirection::OutAndErr + SameTargetRedirection { + cmd: (Option, LiteCommand), + redirection: (Span, LiteCommand), + }, } #[derive(Debug)] @@ -95,6 +100,7 @@ impl LiteBlock { // the block takes ownership of `pipeline`, which means that // our `pipeline` is complete on collecting commands. self.merge_redirections(&mut pipeline); + self.merge_cmd_with_outerr_redirection(&mut pipeline); self.block.push(pipeline); } @@ -103,6 +109,40 @@ impl LiteBlock { self.block.is_empty() } + fn merge_cmd_with_outerr_redirection(&self, pipeline: &mut LitePipeline) { + let mut cmd_index = None; + let mut outerr_index = None; + for (index, cmd) in pipeline.commands.iter().enumerate() { + if let LiteElement::Command(..) = cmd { + cmd_index = Some(index); + } + if let LiteElement::Redirection(_span, Redirection::StdoutAndStderr, _target_cmd) = cmd + { + outerr_index = Some(index); + break; + } + } + if let (Some(cmd_index), Some(outerr_index)) = (cmd_index, outerr_index) { + // we can make sure that cmd_index is less than outerr_index. + let outerr_redirect = pipeline.commands.remove(outerr_index); + let cmd = pipeline.commands.remove(cmd_index); + // `outerr_redirect` and `cmd` should always be `LiteElement::Command` and `LiteElement::Redirection` + if let ( + LiteElement::Command(cmd_span, lite_cmd), + LiteElement::Redirection(span, _, outerr_cmd), + ) = (cmd, outerr_redirect) + { + pipeline.insert( + cmd_index, + LiteElement::SameTargetRedirection { + cmd: (cmd_span, lite_cmd), + redirection: (span, outerr_cmd), + }, + ) + } + } + } + fn merge_redirections(&self, pipeline: &mut LitePipeline) { // In case our command may contains both stdout and stderr redirection. // We pick them out and Combine them into one LiteElement::SeparateRedirection variant. diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index de158e0c0a..361e7c2112 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -1703,6 +1703,9 @@ pub fn parse_module_block( LiteElement::SeparateRedirection { out: (_, command), .. } => block.pipelines.push(garbage_pipeline(&command.parts)), + LiteElement::SameTargetRedirection { + cmd: (_, command), .. + } => block.pipelines.push(garbage_pipeline(&command.parts)), } } else { working_set.error(ParseError::Expected("not a pipeline".into(), span)); diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 05134945cd..e4ba727b0d 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -4003,6 +4003,9 @@ pub fn parse_table_expression( | LiteElement::Redirection(_, _, command) | LiteElement::SeparateRedirection { out: (_, command), .. + } + | LiteElement::SameTargetRedirection { + cmd: (_, command), .. } => { let mut table_headers = vec![]; @@ -4022,6 +4025,9 @@ pub fn parse_table_expression( | LiteElement::Redirection(_, _, command) | LiteElement::SeparateRedirection { out: (_, command), .. + } + | LiteElement::SameTargetRedirection { + cmd: (_, command), .. } => { let mut rows = vec![]; for part in &command.parts { @@ -5316,6 +5322,9 @@ pub fn parse_block( | LiteElement::Redirection(_, _, command) | LiteElement::SeparateRedirection { out: (_, command), .. + } + | LiteElement::SameTargetRedirection { + cmd: (_, command), .. } => parse_def_predecl(working_set, &command.parts), } } @@ -5362,6 +5371,20 @@ pub fn parse_block( err: (*err_span, err_expr), } } + LiteElement::SameTargetRedirection { + cmd: (cmd_span, command), + redirection: (redirect_span, redirect_command), + } => { + trace!("parsing: pipeline element: same target redirection"); + let expr = parse_expression(working_set, &command.parts, is_subexpression); + working_set.type_scope.add_type(expr.ty.clone()); + let redirect_expr = parse_string(working_set, redirect_command.parts[0]); + working_set.type_scope.add_type(redirect_expr.ty.clone()); + PipelineElement::SameTargetRedirection { + cmd: (*cmd_span, expr), + redirection: (*redirect_span, redirect_expr), + } + } }) .collect::>(); @@ -5436,9 +5459,27 @@ pub fn parse_block( } } } - block.pipelines.push(pipeline) } + LiteElement::SameTargetRedirection { + cmd: (span, command), + redirection: (redirect_span, redirect_cmd), + } => { + trace!("parsing: pipeline element: same target redirection"); + let expr = parse_expression(working_set, &command.parts, is_subexpression); + working_set.type_scope.add_type(expr.ty.clone()); + + let redirect_expr = parse_string(working_set, redirect_cmd.parts[0]); + + working_set.type_scope.add_type(redirect_expr.ty.clone()); + + block.pipelines.push(Pipeline { + elements: vec![PipelineElement::SameTargetRedirection { + cmd: (*span, expr), + redirection: (*redirect_span, redirect_expr), + }], + }) + } } } } @@ -5526,6 +5567,14 @@ pub fn discover_captures_in_pipeline_element( discover_captures_in_expr(working_set, err_expr, seen, seen_blocks, output)?; Ok(()) } + PipelineElement::SameTargetRedirection { + cmd: (_, cmd_expr), + redirection: (_, redirect_expr), + } => { + discover_captures_in_expr(working_set, cmd_expr, seen, seen_blocks, output)?; + discover_captures_in_expr(working_set, redirect_expr, seen, seen_blocks, output)?; + Ok(()) + } } } @@ -5819,6 +5868,16 @@ fn wrap_element_with_collect( out: (*out_span, wrap_expr_with_collect(working_set, out_exp)), err: (*err_span, wrap_expr_with_collect(working_set, err_exp)), }, + PipelineElement::SameTargetRedirection { + cmd: (cmd_span, cmd_exp), + redirection: (redirect_span, redirect_exp), + } => PipelineElement::SameTargetRedirection { + cmd: (*cmd_span, wrap_expr_with_collect(working_set, cmd_exp)), + redirection: ( + *redirect_span, + wrap_expr_with_collect(working_set, redirect_exp), + ), + }, PipelineElement::And(span, expression) => { PipelineElement::And(*span, wrap_expr_with_collect(working_set, expression)) } diff --git a/crates/nu-protocol/src/ast/pipeline.rs b/crates/nu-protocol/src/ast/pipeline.rs index e39d9b3ce2..c839463b43 100644 --- a/crates/nu-protocol/src/ast/pipeline.rs +++ b/crates/nu-protocol/src/ast/pipeline.rs @@ -18,6 +18,10 @@ pub enum PipelineElement { out: (Span, Expression), err: (Span, Expression), }, + SameTargetRedirection { + cmd: (Option, Expression), + redirection: (Span, Expression), + }, And(Span, Expression), Or(Span, Expression), } @@ -31,6 +35,10 @@ impl PipelineElement { out: (_, expression), .. } => expression, + PipelineElement::SameTargetRedirection { + cmd: (_, expression), + .. + } => expression, PipelineElement::And(_, expression) => expression, PipelineElement::Or(_, expression) => expression, } @@ -38,7 +46,11 @@ impl PipelineElement { pub fn span(&self) -> Span { match self { - PipelineElement::Expression(None, expression) => expression.span, + PipelineElement::Expression(None, expression) + | PipelineElement::SameTargetRedirection { + cmd: (None, expression), + .. + } => expression.span, PipelineElement::Expression(Some(span), expression) | PipelineElement::Redirection(span, _, expression) | PipelineElement::SeparateRedirection { @@ -46,7 +58,11 @@ impl PipelineElement { .. } | PipelineElement::And(span, expression) - | PipelineElement::Or(span, expression) => Span { + | PipelineElement::Or(span, expression) + | PipelineElement::SameTargetRedirection { + cmd: (Some(span), expression), + .. + } => Span { start: span.start, end: expression.span.end, }, @@ -57,7 +73,11 @@ impl PipelineElement { PipelineElement::Expression(_, expression) | PipelineElement::Redirection(_, _, expression) | PipelineElement::And(_, expression) - | PipelineElement::Or(_, expression) => expression.has_in_variable(working_set), + | PipelineElement::Or(_, expression) + | PipelineElement::SameTargetRedirection { + cmd: (_, expression), + .. + } => expression.has_in_variable(working_set), PipelineElement::SeparateRedirection { out: (_, out_expr), err: (_, err_expr), @@ -70,9 +90,11 @@ impl PipelineElement { PipelineElement::Expression(_, expression) | PipelineElement::Redirection(_, _, expression) | PipelineElement::And(_, expression) - | PipelineElement::Or(_, expression) => { - expression.replace_in_variable(working_set, new_var_id) - } + | PipelineElement::Or(_, expression) + | PipelineElement::SameTargetRedirection { + cmd: (_, expression), + .. + } => expression.replace_in_variable(working_set, new_var_id), PipelineElement::SeparateRedirection { out: (_, out_expr), err: (_, err_expr), @@ -98,6 +120,10 @@ impl PipelineElement { | PipelineElement::Redirection(_, _, expression) | PipelineElement::And(_, expression) | PipelineElement::Or(_, expression) + | PipelineElement::SameTargetRedirection { + cmd: (_, expression), + .. + } | PipelineElement::SeparateRedirection { out: (_, expression), ..