Avoid blocking when `o+e>` redirects too much stderr message by WindSoilder · Pull Request #8784 · nushell/nushell

WindSoilder · 2023-04-06T22:53:45Z

Description

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:

# 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

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

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

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

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
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 after the PR is merged, if necessary. This will help us keep the docs up to date.

…t::SameTargetRedirection

sholderbach · 2023-04-06T23:27:29Z

Thank you for this detailed PR description!

sophiajt · 2023-04-09T22:01:18Z

crates/nu-command/src/system/run_external.rs

    call: &Call,
    redirect_stdout: bool,
    redirect_stderr: bool,
+    out_file: Option<Spanned<String>>,


This doesn't feel right to me. create_external_command shouldn't need to know that its output will be going to a file.

sophiajt · 2023-04-09T22:02:23Z

I'm glad we're trying to fix this issue. I'm not sure if this is quite the right approach. run_external is already pretty complex, and it doesn't feel like the right direction to make it aware that it will be writing to a file too.

WindSoilder · 2023-04-09T22:16:19Z

it doesn't feel like the right direction to make it aware that it will be writing to a file too

Yeah I know about your concern, but I think if we write piped() external commands' output, we'd never have a chance to write both message to a file in a correct way.
How bash implements redirection is using something like dup call to clone file handler.
So I think the external result handling should be done at the beginning of process creation, and tell OS to write these message to relative file directly.

codecov · 2023-04-15T00:09:53Z

Codecov Report

Merging #8784 (604b596) into main (a45bd03) will decrease coverage by 0.02%.
The diff coverage is 63.37%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8784      +/-   ##
==========================================
- Coverage   68.96%   68.94%   -0.02%     
==========================================
  Files         641      641              
  Lines      102434   102540     +106     
==========================================
+ Hits        70646    70701      +55     
- Misses      31788    31839      +51

Impacted Files	Coverage Δ
crates/nu-cli/src/completions/completer.rs	`88.52% <0.00%> (-0.20%)`	⬇️
crates/nu-cli/src/syntax_highlight.rs	`65.56% <0.00%> (-0.20%)`	⬇️
crates/nu-command/src/formats/from/nuon.rs	`34.32% <0.00%> (-0.08%)`	⬇️
crates/nu-parser/src/flatten.rs	`52.53% <0.00%> (-1.19%)`	⬇️
crates/nu-parser/src/parse_keywords.rs	`77.70% <0.00%> (-0.06%)`	⬇️
crates/nu-protocol/src/ast/pipeline.rs	`34.90% <12.50%> (-4.05%)`	⬇️
crates/nu-parser/src/parser.rs	`85.15% <51.06%> (-0.41%)`	⬇️
crates/nu-engine/src/eval.rs	`75.26% <84.84%> (+1.51%)`	⬆️
crates/nu-parser/src/lite_parser.rs	`69.51% <96.42%> (+2.33%)`	⬆️

... and 1 file with indirect coverage changes

amtoine · 2023-04-15T12:01:22Z

i see you have made quite a few merges @WindSoilder 😮

if it's ready, maybe this PR should land to avoid more conflicts? 😋

WindSoilder · 2023-04-15T14:39:00Z

i see you have made quite a few merges

Ah yeah, I merged from main frequently because I want to have latest main with the fix installed on my machine :-)

WindSoilder · 2023-05-01T05:37:19Z

Finally I think it's ready to review, it only involves parsing change, and we don't need to change run-external

crates/nu-command/tests/commands/redirection.rs

fdncred · 2023-05-12T17:19:33Z

@WindSoilder glad to see you arrived and a green ci, let's wait until the release and land this early on in the dev cycle so it can get ample testing. thanks!

WindSoilder · 2023-05-17T22:02:09Z

So...I think it's ready and we can have full test under current version :-)

fdncred · 2023-05-17T22:47:18Z

thanks @WindSoilder. let's give it a try.

WindSoilder added 6 commits April 7, 2023 06:22

introduce a new PipelineElement::SameTargetRedirection and LiteElemen…

22129d6

…t::SameTargetRedirection

finish implementation

d26ffe4

adjust func name, add comment and ut

0365e70

fix clippy, add one more comment

2d8c684

make less changeset

18c3085

fix ut on windows

7f3d919

WindSoilder added 6 commits April 7, 2023 09:09

merge from main

84a75b9

fix error after merge from main

d935d75

merge from main

3bdc73a

fix error after merge from main

f67112d

fmt

bee0f6f

Merge branch 'main' into outerr

de4c5e5

sophiajt reviewed Apr 9, 2023

View reviewed changes

WindSoilder added 5 commits April 10, 2023 09:16

Merge branch 'main' into outerr

b39bffa

Merge branch 'main' into outerr

f9510b5

Merge branch 'main' into outerr

23d418e

Merge branch 'main' into outerr

d7930bd

Merge branch 'main' into outerr

19c87e7

WindSoilder marked this pull request as draft April 28, 2023 01:05

WindSoilder added 3 commits May 1, 2023 09:40

merge from main

5174b55

make some fix after merging

57bc919

finish implementation

f3ceb6c

WindSoilder marked this pull request as ready for review May 1, 2023 05:33

fmt

10eeb4a

Merge branch 'main' into outerr

4159d67

sophiajt reviewed May 6, 2023

View reviewed changes

crates/nu-command/tests/commands/redirection.rs Outdated Show resolved Hide resolved

WindSoilder added 7 commits May 8, 2023 14:06

Merge branch 'main' into outerr

5d64561

fix test

b07b8b8

remove extra print

bde9c8d

try to make test work on windows

d4d4fdc

Merge branch 'main' into outerr

46ef570

Merge branch 'main' into outerr

9673093

try fix test on windows because cmd line is too long to run

604b596

fdncred added the status:wait-until-after-nushell-release label May 12, 2023

fdncred merged commit b150f9f into nushell:main May 17, 2023

WindSoilder deleted the outerr branch May 17, 2023 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid blocking when `o+e>` redirects too much stderr message#8784

Avoid blocking when `o+e>` redirects too much stderr message#8784
fdncred merged 29 commits intonushell:mainfrom
WindSoilder:outerr

WindSoilder commented Apr 6, 2023 •

edited

Loading

Uh oh!

sholderbach commented Apr 6, 2023

Uh oh!

sophiajt Apr 9, 2023

Uh oh!

sophiajt commented Apr 9, 2023

Uh oh!

WindSoilder commented Apr 9, 2023 •

edited

Loading

Uh oh!

codecov bot commented Apr 15, 2023 •

edited

Loading

Uh oh!

amtoine commented Apr 15, 2023

Uh oh!

WindSoilder commented Apr 15, 2023

Uh oh!

WindSoilder commented May 1, 2023

Uh oh!

Uh oh!

fdncred commented May 12, 2023

Uh oh!

WindSoilder commented May 17, 2023

Uh oh!

fdncred commented May 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

WindSoilder commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

About the change

What happened internally?

LiteBlock before

LiteBlock after

Block before

Block after

Tests + Formatting

After Submitting

Uh oh!

sholderbach commented Apr 6, 2023

Uh oh!

sophiajt Apr 9, 2023

Choose a reason for hiding this comment

Uh oh!

sophiajt commented Apr 9, 2023

Uh oh!

WindSoilder commented Apr 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amtoine commented Apr 15, 2023

Uh oh!

WindSoilder commented Apr 15, 2023

Uh oh!

WindSoilder commented May 1, 2023

Uh oh!

Uh oh!

fdncred commented May 12, 2023

Uh oh!

WindSoilder commented May 17, 2023

Uh oh!

fdncred commented May 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WindSoilder commented Apr 6, 2023 •

edited

Loading

WindSoilder commented Apr 9, 2023 •

edited

Loading

codecov bot commented Apr 15, 2023 •

edited

Loading