Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check for broken pipe when streaming the output. #11850

Closed
wants to merge 1 commit into from

Conversation

abhaykadam
Copy link

For instance, if we give command:

$rust | xargs -v

we get,

xargs: illegal option -- v

usage: xargs [-0opt] [-E eofstr] [-I replstr [-R replacements]] [-J replstr]

             [-L number] [-n number [-x]] [-P maxprocs] [-s size]

             [utility [argument ...]]

error: internal compiler error: unexpected failure

This message reflects a bug in the Rust compiler.

We would appreciate a bug report: http://static.rust-lang.org/doc/master/complement-bugreport.html

note: the compiler hit an unexpected failure path. this is a bug

task '<main>' failed at 'Unhandled condition: io_error: io::IoError{kind: BrokenPipe, desc: "broken pipe", detail: None}', /Users/abhay/devel/rust/rust/src/libstd/condition.rs:139

The BrokenPipe (or EPIPE) occurs when the receiving-end process returns non-zero exit-status.

I tested raising the condition with following C code:

//C File: test_line.c
#include <stdio.h>

int main() {
    char *line = NULL;
    size_t linecap = 0;
    ssize_t linelen;

    while ((linelen = getline(&line, &linecap, stdin)) > 0)
        fwrite(line, linelen, 1, stdout);   

    return 1;
}

The above code returns 1 after reading everything from the piped process.
The commands i executed:

$clang test_line.c -o test_line
$rustc | ./test_line

The last command gave the unexpected failure error.

This patch checks if raised condition is not BrokenPipe, and otherwise, it raised the condition.

For instance, if we give command:
$rust | xargs -v

we get,
xargs: illegal option -- v

usage: xargs [-0opt] [-E eofstr] [-I replstr [-R replacements]] [-J replstr]

             [-L number] [-n number [-x]] [-P maxprocs] [-s size]

             [utility [argument ...]]

error: internal compiler error: unexpected failure

This message reflects a bug in the Rust compiler.

We would appreciate a bug report: http://static.rust-lang.org/doc/master/complement-bugreport.html

note: the compiler hit an unexpected failure path. this is a bug

task '<main>' failed at 'Unhandled condition: io_error: io::IoError{kind: BrokenPipe, desc: "broken pipe", detail: None}', /Users/abhay/devel/rust/rust/src/

The BrokenPipe (or EPIPE) occurs when the receiving-end process returns non-zero exit-status.

This patch checks if raised condition is not BrokenPipe, and if not, it raises the condition.
@lilyball
Copy link
Contributor

Fixing this issue is certainly a good idea, but I'm not sure this is the right approach. What you've done here hides all EPIPE errors for stdout/stderr. Being able to detect that stdout has been closed is a valid thing to want to be able to do, but you can't do that anymore with this.

Additionally, this only hides it for stdout/stderr. It doesn't try to suppress EPIPE for any other file handles.

I'm not sure what the practical solution here is. Ideally, anything that writes to stdout/stderr would handle io_errors, because crashing rustc due to a stdout issue (including, but not limited to, EPIPE) seems less than ideal. But practically speaking, I don't know how much effort it would take to find every location in rustc that would need to be updated.

My understanding is that I/O functions were slated to stop using conditions and start using proper return values. If this happens, then that would be a good time to add correct handling of things like this, since all stdout writing would need to be updated anyway. Unfortunately the relevant ticket, #10449, has already been closed so I'm not quite sure what the status of this is. @alexcrichton, any idea?

@lilyball
Copy link
Contributor

cc #10602, #9795

@alexcrichton
Copy link
Member

Hm, I'm not entirely sure what we should do about this. We're trying to promote "no unused results" everywhere, so migrating this to conditions won't fix anything. What'll happen is that print and friends will all fail the task if the printing fails (in that imagined world, that is).

It does seem odd to ignore all EPIPE errors, I can imagine that there's a legitimate use-case somewhere for seeing it. On the other hand it seems odd that we print a failure message on a broken pipe. I'll have to think a little more on this.

@lilyball
Copy link
Contributor

@alexcrichton It's already a condition, which is why we print the failure message. My understanding is that we're going to be migrating I/O to returning a Result instead of a condition (or perhaps something with dynamic variables, but I don't see that as being any better than conditions for I/O). Perhaps print and friends could simply ignore errors, given the expectation that clients of this API don't particularly care about detecting them. Clients that do care about errors could then use io::stdout().write_str() instead of print().

On that note, if we're going to advocate calling stdout() for stuff like this, we may need to revisit the implementation of stdout()/stderr()/stdin(). Right now they always perform allocation. Also, from what I can tell, they seem to always represent the process's stdout/stderr, instead of the task-local stdout/stderr, which unfortunately means that io::stdout().write_str(s) is not equivalent to print(s).

@abhaykadam
Copy link
Author

Hi guys, thanks for the comments. I would like to work on this issue. Could you please tell me what i should do?

@alexcrichton
Copy link
Member

Here's what I found from some other languages

  • node - throws an exception
  • ruby - throws an exception
  • python - prints an error
  • C - up to the programmer to ignore/react to the error

Sounds like it's pretty standard to be expected to deal with this in the code itself (rather than ignoring the error). Closing because it seems like this is normal behavior.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 1, 2023
[`deprecated_semver`]: Allow `#[deprecated(since = "TBD")]`

"TBD" is allowed by rustdoc, saying that it will be deprecated in a future version. rustc will also not actually warn on it.
I found this while checking the rust-lang/rust with clippy.

changelog: [`deprecated_semver`]: allow using `since = "TBD"`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants