Provide ExitStatusError #82973

ijackson · 2021-03-10T10:25:11Z

Tracking Issue: #84908

In MR #81452 "Add #[must_use] to [...] process::ExitStatus" we concluded that the existing arrangements in are too awkward so adding that #[must_use] is blocked on improving the ergonomics.

I wrote a mini-RFC-style discusion of the approach in #73125 (comment)

rust-highfive · 2021-03-10T10:25:15Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

ijackson · 2021-03-10T10:55:31Z

This is a follow-on from #82411. See also #82974 which I have just submitted.

bors · 2021-03-10T22:19:36Z

☔ The latest upstream changes (presumably #82982) made this pull request unmergeable. Please resolve the merge conflicts.

library/std/src/process.rs

joshtriplett · 2021-03-16T09:46:48Z

cc @yaahc for review regarding the handling of exit statuses as errors, and potential interaction with Termination and other error-handling proposals.

ijackson · 2021-03-16T15:46:42Z

cc @yaahc for review regarding the handling of exit statuses as errors, and potential interaction with Termination and other error-handling proposals.

I considered the potential interactions with several areas of process exit status handling. A review is a good idea, but it might be helpful to set out my thoughs:

Termination is the return value from main. I think the docs for process::ExitStatus (in Nightly) and for ExitStatusError (in this MR) already properly relate the return value from ExitStatus[Error]::code() to the the value returned from main. Introducing ExitStatusError doesn't change that. So I don't think there is any nontrivial interaction with Termination.

There is also std::process::ExitCode (unstable). Here is more of a can of worms.

To discuss this clearly means dealing with a terminological problem. On Unix there is a "wait status" (as you get from wait* system calls) and an "exit status" (as you pass to exit). A wait status might represent an exit status, but it might alternatively represent some other kind of process termination (or, more generally, state).

Rust stdlib uses the term "exit code" (or just code) for what Unix calls "exit status"; and it uses "exit status" for what Unix calls "wait status". So a Rust "exit status" can be an "exit code" or some other kind of process termination.

I am going to use the term "exit value integer" for the value passed to exit.

One problem is that Rust stdlib thinks an exit value integer is i32. But in C it is an int. But but on Unix only the bottom 8 bits are used. This leads to a number of strangenesses.

For example, on Fuchsia (AFAICT) the actual exit value integer is a u64. But ExitStatus::code must return an i32 and is therefore broken.

Another example: on Unix you can pass a value >255 to std::process::exit or return it from main, but only the bottom bits are preserved: observe that here https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=de0af65b4d80dedc8b8c4934c353bd90 we appear to see success! This is a footgun.

ExitCode might be part of the solution to these problems, by generalising the exit value integer. But the other methods in process, and the integer return value type of main and in Termination, are all i32.

My present MR does not attempt to drain this swamp. I think it could only be drained by replacing many of the affected methods in process, and maybe changing the allowable return types from main (!) There are many problems with process::Command, so it might be a good idea to replace process in its entirety anyway.

I think that my addition of ExitStatusError does not make this any worse, at least. Whatever solution is adopted for ExitStatus::code() can be adopted for ExitStatusError::code().

joshtriplett · 2021-03-16T16:20:52Z

I'm not suggesting this PR needs to solve this problem. Rather, there are specific proposals in development to allow attaching information to errors, such as the numeric value to exit the process with, or am HTTP response code. I'm asking for @yaahc's review to see if this potentially interacts with any of those proposals.

yaahc · 2021-03-16T19:09:39Z

cc @yaahc for review regarding the handling of exit statuses as errors, and potential interaction with Termination and other error-handling proposals.

reviewing, also cc @JDuchniewicz who is currently leading the effort on stabilizing Termination.

edit: still reviewing, but this seems for the most part separate from Termination so far, so I'm assuming this wouldn't conflict with the generic member access RFC. The only way they interact is that we'd probably want to have ExitStatusError use provide to pass it's ExitCode out too the Termination impl, though I just learned some bad news related to specialization so I'm not as positive that the Termination plans we had are doable.

library/std/src/process.rs

library/std/src/sys/unix/process/process_fuchsia.rs

library/std/src/sys/unix/process/process_unix.rs

ijackson · 2021-04-30T15:34:18Z

Hi. I think all the comments so far have been addressed. I would like to get this merged because it's blocking fixing almost all the examples for Command. Currently those examples generally fail to check the exit status at all, and several of them leak the child process.

@yaahc I think you are happy that this doesn't cause trouble for the stabilisation of Termination. Do you want to wait for a double-check from @JDuchniewicz? Apart from that, I think this is then just in need of a review by a T-libs member.

If that review is positive, I'll open a tracking issue, update the MR (and probably rebase...), etc.

Thanks.

library/std/src/process.rs

yaahc · 2021-05-03T18:28:54Z

@yaahc I think you are happy that this doesn't cause trouble for the stabilisation of Termination. Do you want to wait for a double-check from @JDuchniewicz? Apart from that, I think this is then just in need of a review by a T-libs member.

I'm quite confident there won't be any issues here with respect to the Termination trait.

If that review is positive, I'll open a tracking issue, update the MR (and probably rebase...), etc.

I've gone ahead and done another review pass with a few more comments. Once those get resolved I think we can either go ahead and merge this, or, if there are still changes to the stable API of std in the PR, we will need to start an FCP.

JDuchniewicz · 2021-05-04T09:08:27Z

Read through the PR and it looks good to me. As @yaahc mentioned, it will allow us for more options for reporting the error/exit status inside the Termination trait.

ijackson · 2021-05-04T15:23:34Z

@yaahc Thanks for the review. I think I have addressed your comments. I also opened a tracking issue and updated the #[unstable] attributes with it.

For now I haven't squashed this down to assist re-review. Let me know when I should do that (and rebase onto current trunk).

library/std/src/process.rs

bors · 2021-05-05T16:53:57Z

☔ The latest upstream changes (presumably #84200) made this pull request unmergeable. Please resolve the merge conflicts.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

yaahc

Sorry for not catching this earlier but there is one last change we need to make to the documentation on ExitStatusExt before this can merge, but I think it's good to go once this is resolved.

library/std/src/os/unix/process.rs

It is unergnomic to have to say things like bad.into_status().signal() Implementing `ExitStatusExt` for `ExitStatusError` fixes this. Unfortunately it does mean making a previously-infallible method capable of panicing, although of course the existing impl remains infallible. The alternative would be a whole new `ExitStatusErrorExt` trait. `<ExitStatus as ExitStatusExt>::into_raw()` is not particularly ergonomic to call because of the often-required type annotation. See for example the code in the test case in library/std/src/sys/unix/process/process_unix/tests.rs Perhaps we should provide equivalent free functions for `ExitStatus` and `ExitStatusExt` in std::os::unix::process and maybe deprecate this trait method. But I think that is for the future. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

Co-authored-by: Josh Triplett <josh@joshtriplett.org>

Co-authored-by: Jane Lusby <jlusby@yaah.dev>

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

We should revert this commit when this is stabilised. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson · 2021-05-12T10:30:51Z

@yaahc Thanks for the review. I have fixed the WASM build, I think - it compiles locally, anyway. I also checked the entries in library/std/src/sys/mod.rs and they all seem to go to unsupported so I hope it should be good everywhere now.

I also took the opportunity to autosquash the fixmes into the underlying commits. I didn't rebase, so the "force pushed" message from github (or plain git diff fb2db8d..26c782b) has just the diff for the WASM fix.

ijackson · 2021-05-14T13:40:11Z

@yaahc Would you mind giving this your (re)review and approval?

yaahc · 2021-05-17T18:37:09Z

@bors r+

bors · 2021-05-17T18:37:10Z

📌 Commit 26c782b has been approved by yaahc

bors · 2021-05-18T08:01:38Z

⌛ Testing commit 26c782b with merge 25a277f...

bors · 2021-05-18T11:54:37Z

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 25a277f to master...

pnkfelix · 2021-05-18T15:39:20Z

Performance triage indicates that this PR introduced a 1.7% regression when fully compiling coercions-debug.

I don't think any performance hit was expected. However, it also seems like this may be just noise.

ijackson · 2021-05-18T16:38:13Z

Performance triage indicates that this PR introduced a 1.7% regression when fully compiling coercions-debug.

How strange. Can you perhaps give me a reference to what "coercions-debug" is? I searched rust-lang/rust for it, and asked a general search engine, and I looked for links in the perf report you linked to, but none of those approaches was fruitful.

I don't think any performance hit was expected. However, it also seems like this may be just noise.

Definitely, a performance hit was not expected. I see the result is annotated with a single ? and the text says "measurements known to have high variation are marked with '?'/'??'". The linkified "noise run" seems to have 0 variation in this test.

pnkfelix · 2021-05-18T17:15:08Z

@ijackson yeah we are actually discussing the noise associated with this specific benchmark in Zulip now (zulip archive, zulip live)

The source code to coercions is here: https://github.com/rust-lang/rustc-perf/blob/master/collector/benchmarks/coercions/src/main.rs

It is definitely a micro-benchmark that is not representative of real code.

(And, again, to be clear, We think we are just seeing noise here. Not something actually problematic in your PR.)

ijackson · 2021-05-19T10:17:55Z

The source code to coercions is here: https://github.com/rust-lang/rustc-perf/blob/master/collector/benchmarks/coercions/src/main.rs

It is definitely a micro-benchmark that is not representative of real code.

Thanks for the link. So I see!

(And, again, to be clear, We think we are just seeing noise here. Not something actually problematic in your PR.)

Right, OK, thanks. Happy hunting. (I have bookmarked that paper by Kalibera & Jones since it piqued my curiosity)

Fix `vxworks` Some PRs made the `vxworks` target not build anymore. This PR fixes that: - rust-lang#82973: copy `ExitStatusError` implementation from `unix`. - rust-lang#84716: no `libc::chroot` available on `vxworks`, so for now don't implement `os::unix::fs::chroot`.

rust-highfive assigned kennytm Mar 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2021

ijackson mentioned this pull request Mar 10, 2021

Improve display of Unix wait statuses, notably giving names for signals #82974

Closed

ijackson force-pushed the exitstatuserror branch from 22c4028 to d2ac93c Compare March 10, 2021 23:09

joshtriplett reviewed Mar 16, 2021

View reviewed changes

library/std/src/process.rs Outdated Show resolved Hide resolved

yaahc requested changes Mar 16, 2021

View reviewed changes

library/std/src/process.rs Show resolved Hide resolved

library/std/src/sys/unix/process/process_fuchsia.rs Show resolved Hide resolved

library/std/src/sys/unix/process/process_unix.rs Show resolved Hide resolved

bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2021

yaahc requested changes May 3, 2021

View reviewed changes

library/std/src/process.rs Outdated Show resolved Hide resolved

library/std/src/process.rs Outdated Show resolved Hide resolved

ijackson mentioned this pull request May 4, 2021

Tracking Issue for ExitStatusError #84908

Open

5 tasks

This comment has been minimized.

Sign in to view

yaahc reviewed May 4, 2021

View reviewed changes

library/std/src/process.rs Show resolved Hide resolved

windows: provide NonZeroDWORD

b50c1bb

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson force-pushed the exitstatuserror branch from 9b4a257 to d26ceba Compare May 10, 2021 10:18

yaahc requested changes May 10, 2021

View reviewed changes

library/std/src/os/unix/process.rs Outdated Show resolved Hide resolved

library/std/src/os/unix/process.rs Outdated Show resolved Hide resolved

ijackson and others added 5 commits May 12, 2021 11:12

Fix typo in doc

8832b0a

Co-authored-by: Josh Triplett <josh@joshtriplett.org>

ExitStatusError: Be more verbose in Display impl

bb4ef68

Co-authored-by: Jane Lusby <jlusby@yaah.dev>

impl crate::error::Error for ExitStatusError

4687153

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ExitStatusError: Remove mentions in stable docs

26c782b

We should revert this commit when this is stabilised. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson force-pushed the exitstatuserror branch from fb2db8d to 26c782b Compare May 12, 2021 10:12

ijackson mentioned this pull request May 14, 2021

Fuchsia's target_family should be None, not "unix" #58590

Open

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2021

bors merged commit 25a277f into rust-lang:master May 18, 2021

rustbot added this to the 1.54.0 milestone May 18, 2021

This was referenced May 18, 2021

Implement most of RFC 2930, providing the ReadBuf abstraction #81156

Merged

Add Linux-specific pidfd process extensions (take 2) #81825

Merged

[WIP] Add size-limited command interface, take 2 #82930

Closed

CDirkx mentioned this pull request May 19, 2021

Fix vxworks #85490

Merged

ijackson deleted the exitstatuserror branch August 24, 2021 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide ExitStatusError #82973

Provide ExitStatusError #82973

ijackson commented Mar 10, 2021 •

edited by yaahc

Loading

rust-highfive commented Mar 10, 2021

ijackson commented Mar 10, 2021

bors commented Mar 10, 2021

joshtriplett commented Mar 16, 2021

ijackson commented Mar 16, 2021

joshtriplett commented Mar 16, 2021

yaahc commented Mar 16, 2021 •

edited

Loading

ijackson commented Apr 30, 2021

yaahc commented May 3, 2021

JDuchniewicz commented May 4, 2021 •

edited

Loading

This comment has been minimized.

ijackson commented May 4, 2021

bors commented May 5, 2021

yaahc left a comment

ijackson commented May 12, 2021

ijackson commented May 14, 2021

yaahc commented May 17, 2021

bors commented May 17, 2021

bors commented May 18, 2021

bors commented May 18, 2021

pnkfelix commented May 18, 2021

ijackson commented May 18, 2021

pnkfelix commented May 18, 2021

ijackson commented May 19, 2021

Provide ExitStatusError #82973

Provide ExitStatusError #82973

Conversation

ijackson commented Mar 10, 2021 • edited by yaahc Loading

rust-highfive commented Mar 10, 2021

ijackson commented Mar 10, 2021

bors commented Mar 10, 2021

joshtriplett commented Mar 16, 2021

ijackson commented Mar 16, 2021

joshtriplett commented Mar 16, 2021

yaahc commented Mar 16, 2021 • edited Loading

ijackson commented Apr 30, 2021

yaahc commented May 3, 2021

JDuchniewicz commented May 4, 2021 • edited Loading

This comment has been minimized.

ijackson commented May 4, 2021

bors commented May 5, 2021

yaahc left a comment

Choose a reason for hiding this comment

ijackson commented May 12, 2021

ijackson commented May 14, 2021

yaahc commented May 17, 2021

bors commented May 17, 2021

bors commented May 18, 2021

bors commented May 18, 2021

pnkfelix commented May 18, 2021

ijackson commented May 18, 2021

pnkfelix commented May 18, 2021

ijackson commented May 19, 2021

ijackson commented Mar 10, 2021 •

edited by yaahc

Loading

yaahc commented Mar 16, 2021 •

edited

Loading

JDuchniewicz commented May 4, 2021 •

edited

Loading