Fix panic if stdout disappears #1622
Conversation
There was a problem hiding this comment.
I'm not entirely sure I'm comfortable with the number of times we have to acquire term2::stdout but I think I understand why it'd have to be this way.
But I don't like how we're swallowing the errors from writeln! I'd rather we just early jump out and allow things to shut down more quickly.
| let mut t = term2::stdout(); | ||
| for it in installed_toolchains { | ||
| if default_name == it { | ||
| let _ = writeln!(t, "{} (default)", it); |
There was a problem hiding this comment.
Shouldn't we be using ? rather than ignoring the error returns from writeln!() since this is just informational and so the jump out shouldn't affect matters?
| } else { | ||
| println!("{}", t); | ||
| let _ = writeln!(t, "{}", it); |
| } | ||
| } | ||
| if show_headers { | ||
| println!("") | ||
| }; | ||
| let _ = writeln!(t, ""); |
| println!( | ||
| let mut t = term2::stdout(); | ||
| for at in active_targets { | ||
| let _ = writeln!( |
| .target | ||
| .as_ref() | ||
| .expect("rust-std should have a target") | ||
| ); | ||
| } | ||
| if show_headers { | ||
| println!("") | ||
| }; | ||
| let _ = writeln!(t, ""); |
| println!("{} (default)", toolchain.name()); | ||
| println!("{}", common::rustc_version(toolchain)); | ||
| let _ = writeln!(t, "{} (default)", toolchain.name()); | ||
| let _ = writeln!(t, "{}", common::rustc_version(toolchain)); |
| } | ||
| None => { | ||
| println!("no active toolchain"); | ||
| let _ = writeln!(t, "no active toolchain"); |
| } | ||
| }, | ||
| Err(err) => { | ||
| if let Some(cause) = err.source() { | ||
| println!("(error: {}, {})", err, cause); | ||
| let _ = writeln!(t, "(error: {}, {})", err, cause); |
| } else { | ||
| println!("(error: {})", err); | ||
| let _ = writeln!(t, "(error: {})", err); |
| } | ||
| } | ||
| } | ||
|
|
||
| if show_headers { | ||
| println!("") | ||
| }; | ||
| let _ = writeln!(t, ""); |
|
Thanks for the review. Is it better to only acquire stout once and to propagate it into for example "print_header"? |
|
What are our chances of being able to simulate this failure in a test? I agree that #1621 is a bug and should be fixed, but I'd prefer to find the minimal solution that swallows the least errors and panics as much as it used to. |
|
Another option is to register a handler to sigpipe and exit gracefully. I guess that is a straightforward solution if rustup already handles signals. Rust registers the "ignore" signal handler for sigpipe by default: On the other hand, rust std ignores all other reasonable stdout IO errors such as stdout being closed or read-only. So I'm not sure what other kinds of errors you would like to propagate from write!(). |
|
Mostly I just find |
The easiest reproducer is below. I don't know if that qualiefies as a "simulation of this failure". Actually both the current behavior and my PR is the wrong expected behavior. Current behavior exits with error code != 0, and with my PR rustup continues to run even though we technically have received SIGPIPE and should exit asap. Would you like me to propagate the error all the way up to main and there handle it appropriately? Which is die silently with error code 0. |
|
I think that #1630 is a better version of this. and I'd suggest closing this PR in favour of that. |
Fixes #1621