Tracking Issue for std::process error handling

ijackson commented 4 years ago

Hi. This is a tracking issue for various error handling awkwardnesses in std::process. It seemed useful to gather them together here. I hope you consider this helpful. I don't think this needs an RFC but I can make an RFC if people feel that would be best.

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Relevant RFCs and proposals:

https://github.com/rust-lang/rfcs/pull/3362 is a proposal to add a new methods to Command and a new SubprocessError type

Platform-independent issues

[ ] #70186 request for #[must_use] on std::process::Child (first cut in #81452)
[ ] #73127 request for #[must_use] on std::process::ExitStatus (first cut in #81452)
[ ] #73126 request for less hazardous version of output()
[ ] #73125 request for affordance to make Result from ExitStatus

Unix issues relating to `ExitStatusExt`

[x] #73128 request for std::os::unix::process::ExitStatusExt into_raw
[x] #73129 request for std::os::unix::process::ExitStatusExt .coredumped() method

ijackson commented 3 years ago

@rustbot modify labels +T-libs

KodrAus commented 3 years ago

@ijackson Are you happy if we collapse those child issues into this one and consider everything we want to do to improve our std::process error handling together?

KodrAus commented 3 years ago

I was thinking our path could look something like:

Design our API for converting ExitStatus into a Result.
Update our docs (in particular our docs around ExitStatus are pretty sparse).
Get the ExitStatus into Result API stabilized and update our examples to use it instead of ignoring them.
Consider #[must_use] on ExitStatus.

What do you think?

KodrAus commented 3 years ago

On unix, failure to use a Child leaks the process. It continues running in parallel, even after we have exited (or crashed), possibly interfering with the user's terminal or driveling nonsense into a logfile long after the original event.

In #81452, am I reading that right and you're saying it's possible to spawn a child that outlives its parent process entirely?

KodrAus commented 3 years ago

As for what to make the API for ExitStatus into Result look like, I arrived at success_or_else after thinking:

We should try give our methods that convert a non Option/Result/Poll-like type into one a more semantic name, so that you can identify it in a chain of combinators.
We don't necessarily need to introduce a new ExitStatusError type that will look roughly the same as ExitStatus, except implement the Error trait, because ExitStatus is Copy, if you want to add it as context you can freely refer to it anywhere.

We could consider returning a io::Result<()> though, and passing a private inner error type instead that includes the exit code in its Display message.

I think it would be nice to try come up with a name that's a little clearer than either ok or into_result. In your example:

Command::new(adb_path)
      .arg("push")
      .arg(&exe_file)
      .arg(&self.config.adb_test_dir)
      .status()
      .unwrap_or_else(|_| panic!("failed to exec `{:?}`", adb_path));
      .ok()
      .unwrap_or_else(|s| panic!("`{:?}` failed: {}", adb_path, s));

I don't find it clear that .ok() is operating on a ExitStatus, I'm expecting it to be on a Result, making the last line operate on an Option, except that it's taking an argument into the closure, which Option doesn't do.

ijackson commented 3 years ago

Hi. Thanks for the attention.

I was thinking our path could look something like:

Design our API for converting ExitStatus into a Result.

Update our docs (in particular our docs around ExitStatus are pretty sparse).

Get the ExitStatus into Result API stabilized and update our examples to use it instead of ignoring them.

Consider #[must_use] on ExitStatus.

What do you think?

The current situation seems quite bad to me but it has been like this for quie a while. So I want to fix it in whatever way will get general approval :-). I am fine with doing things in this order.

In #81452, am I reading that right and you're saying it's possible to spawn a child that outlives its parent process entirely?

Yes. That is the default behaviour of fork() if you exit without waiting for the child. This is rarely desirable.

As for what to make the API for ExitStatus into Result look like, I arrived at success_or_else after thinking:

If it were called or_else it ought to take a closure. I think we need to provide something returning a Result (on which you could say unwrap_or_else if you wanted). Any ..._or ought to take a parameter for use when it's failure, as it does for Result, Option, et al. E.g. Option has ok_or and ok_or_else taking an E (somehow) and returning Result<T,E>.

â¢ We should try give our methods that convert a non Option/Result/Poll-like type into one a more semantic name, so that you can identify it in a chain of combinators.

That suggests .exit_ok() to me. We don't want it to be a very long name because this is supposed to be the easy way to do it right.

It didn't occur to me to mention something about the type in the method name but that seems to address nicely the concern that the combinator list is confusing otherwise.

â¢ We don't necessarily need to introduce a new ExitStatusError type that will look roughly the same as ExitStatus, except implement the Error trait, because ExitStatus is Copy, if you want to add it as context you can freely refer to it anywhere.

I don't follow this. You're saying that the Error value from .exit_ok() (or whatever we call it) ought not to contain the exit status ? Or that it ought to be something much more complicated ?

I definitely don't think the usual usage pattern ought to involve putting the exit status into a variable and then handling it again in a closure passed to a Result combinator.

We could consider returning a io::Result<()> though, and passing a private inner error type instead that includes the exit code in its Display message.

I find this quite unattractive. io::Error is primarily a container for an OS error number for use with ErrorKind etc. It's capable of containing other things mostly so that APIs (eg important traits) that invole io::Error can be provided by other things. To use io::Error for anything else is clumsy (and involves boxing twice...).

(Following the discussion in my pre-RFC) I think by far the most sensible alternative is a dedicated ExitStatusError which is just the (nonzero) wait status. Then it would be Copy, Into, and so on.

Thanks, Ian.

-- Ian Jackson ijackson@chiark.greenend.org.uk These opinions are my own.

Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

ijackson commented 3 years ago

I guess in summary, I think next step in this area should probably be to work up an MR for ExitStatusError. I think once you see it you'll like it :-).

KodrAus commented 3 years ago

We don't necessarily need to introduce a new ExitStatusError type that will look roughly the same as ExitStatus, except implement the Error trait, because ExitStatus is Copy, if you want to add it as context you can freely refer to it anywhere.

I don't follow this. You're saying that the Error value from .exit_ok() (or whatever we call it) ought not to contain the exit status ? Or that it ought to be something much more complicated ?

Ah, what I meant here was that I don't think we should introduce a dedicated ExitStatusError type. I think our options would be:

fn make_exit_status_a_result<E: Error>(self, err: E) -> Result<(), E>

or:

fn make_exit_status_a_result(self) -> io::Result<()>

Every other Result return in std::process uses io::Result, so I think we'd be best to use that here too. I don't believe a public dedicated error type is worth introducing, especially if it's exactly just ExitStatus, but implements the Error trait. If you want to get an ExitStatus to work with then you can inspect it directly from .status() and produce your own error type. It would be especially odd to introduce an Error type that can be converted back into an ExitStatus if we intend to mark it as #[must_use], ~~the warning suggestion only really applies to the output of .status(), not to other accidentally unused ExitStatuses that should fall under the unused_variables lint.~~

KodrAus commented 3 years ago

Yes. That is the default behaviour of fork() if you exit without waiting for the child. This is rarely desirable.

Yeh I can't think of a case where that's ever what I've wanted! We've previously rejected attempts to add #[must_use] to thread::spawn, because explicitly not joining a thread is a legitimate pattern and when your process goes away so do all those threads anyways.

Sorry for the mix-around, would you be happy to resubmit a PR that adds #[must_use] to Child and we can see what the rest of Libs think of it? It does seem justifiably different to thread::spawn, and the docs are already full of warnings.

ijackson commented 3 years ago

Sorry for the mix-around, would you be happy to resubmit a PR that adds #[must_use] to Child and we can see what the rest of Libs think of it? It does seem justifiably different to thread::spawn, and the docs are already full of warnings.

Most of the examples in the stdlib leak the Child (!) and would need fixing. Doing that one #[must_use] now, and the rest later, would mean half-fixing the examples now and then fixing them properly later.

ijackson commented 3 years ago

FYI the behaviour if you leak the child:

rustcargo@zealot:~$ cat t.rs
use std::process::Command;
fn main() {
    println!("Hello, world!");
    let mut cmd = Command::new("sh");
    cmd.args(&["-xec","
        echo hi
        sleep 5
        echo ho
    "]);
    let child = cmd.spawn().expect("spawn");
    dbg!(&child);
}
rustcargo@zealot:~$ rustc t.rs
rustcargo@zealot:~$ ./t
Hello, world!
[t.rs:11] &child = Child {
    stdin: None,
    stdout: None,
    stderr: None,
}
rustcargo@zealot:~$ + echo hi
hi
+ sleep 5
+ echo ho
ho

ijackson commented 3 years ago

Ah, what I meant here was that I don't think we should introduce a dedicated ExitStatusError type.

Thanks for your comments. I still think, especially after the discussion on internals, that the dedicated ExitStatusError type is the right approach. I think I ought to be able to convince you but I don't want to annoy you and a bullet-point to-and-fro seems a poor approach. Also this tracking bug is not the ideal place to have this rather more specific conversation about this one question.

How about I write up my proposal in a mini-RFC style ("motivation", "alternatives" etc.) in #/73125 ?

KodrAus commented 3 years ago

@ijackson No problem! I'll also go and read through the pre-RFC discussion so I'm properly informed too.

ijackson commented 1 year ago

Just added a link to https://github.com/rust-lang/rfcs/pull/3362

rust-lang / rust