Command improvements for ergonomics and error handling

[ijackson]

Rendered

[Noratrieb]

I agree with the motivation that Command is not very ergonomic, especially around the common "return error containing stderr on error" workflow.
But this RFC is a little too opinionated about stderr in my opinion. It strictly treats it as the "error output", which is just often not true in practice. Even cargo logs all its output to stderr. Therefore, considering any output to stderr an error is a bad idea.
Also, if I read this correctly, there's no way to get stdout when the command failed using these methods except with the existing .output(), but this plans to deprecate that.

[ijackson]

I agree with the motivation that Command is not very ergonomic, especially around the common "return error containing stderr on error" workflow. But this RFC is a little too opinionated about stderr in my opinion. It strictly treats it as the "error output",

Um? That's not true. In my RFC, by default, error output is sent to the Rust program's stderr (inherit).

which is just often not true in practice. Even cargo logs all its output to stderr. Therefore, considering any output to stderr an error is a bad idea.

I think this depends what program you're running. I don't think it is wrong to have the option to treat stderr output as an error, as a non-default mode.

Also, if I read this correctly, there's no way to get stdout when the command failed using these methods except with the existing .output(), but this plans to deprecate that.

No, that's not true either. If you have a SubprocessError, you can call the .stdout_bytes() method on it. (It will only be Some if you called one of the .get_output* methods, obviously; otherwise the stdout went to wherever you configured the Command to send it.)

[Noratrieb]

Ah, I seem to have overlooked that, sorry.

[joshtriplett]

Please specify the behavior if the program has no output. Does this fail in that case, or is the requirement "at most one line"?

[ijackson]

I think it should succeed and give an empty string. Will write that into the RFC.

[joshtriplett]

It seems like this helper would commonly be used for programs for which you expect to parse the output, in which case having to separately check for empty string seems to defeat part of the ergonomic win of using the helper.

But in any case, this isn't a blocker as you've split this helper out into possible future work.

[joshtriplett]

🎉 🎉 🎉

[nagisa]

Would it be possible to make this change without an RFC? Seems somewhat unfortunate to tie this together with the API additions.

[joshtriplett]

This description makes me wonder about the semantics of the other functions in this family. Do they wait for process exit, or for process exit and EOF? (I'm hoping the former, or possibly "process exit and then a non-blocking read saying it would block".)

[ijackson]

This is stated explicitly in the docs for what I am now calling read_stdout_bytes. All of these functions wait for both EOF and process exit. I will add a short xref note to the other two functions. In the real docs the spec text should probably be cut and pasted instead.

[joshtriplett]

In a broad sense, I think the approach of this RFC seems mostly reasonable, and I'd like to see something along these lines.

There are two categories of issues I'd like to raise. I think both are addressable, and I'm hoping it'll be possible to address them and merge the resulting RFC.

One family of issues are largely bikeshed-painting, where the functionality seems perfectly fine but I think the names are unintuitive. For those, I want to be clear that I'm not attached to any particular names, and primarily want to express some desired properties of a name that I think the proposed names don't have. I'd like to align on those properties and then I don't especially care what name we pick that meets those properties.

The other family of issues are those of functionality, where I think the proposal makes a common use case more challenging.

---

First, the bikesheds:

• Bikeshed: We currently have programs using a method called output that retrieves both stdout and stderr, merged. I don't think we can switch from that to methods named get_output* that only return stdout without leading to some confusion. I would propose a name that makes it clear that these only return stdout, such as get_stdout. (Arguably that may mean it needs to be called read_stdout or similar, because get_stdout might imply "get the previously set standard output device"; in that case, we could also have stdout_reader for the reader method to avoid read_stdout_reader.)
• Bikeshed (minor): Perhaps ProcessError rather than SubprocessError? We currently use the term "process" in various places, and not "subprocess". Plus, this error type seems useful for any kind of process.
• Bikeshed: s/ChildOutputRead/ChildOutputReader/, and likewise s/_read/_reader/ for the method returning it. "reader" seems to be the ecosystem convention for "things that implement Read", and Stream is something else entirely. (Regarding the item in the "Drawbacks" section, though, I'm completely in favor of having this function and type.)

---

Next, the semantics:

• Semantic: Please spell out exactly how you're expecting people to retrieve stdout-and-stderr interleaved, as they can currently do with output. If this isn't as simple as "call a single non-deprecated method", I think that needs improving. I think the proposal is that if you route stderr to the same place you route stdout, rather than capturing it in a separate pipe, then you'll get the combined output and stderr won't get treated as a failure? In any case, that needs documenting and needs to be simple. (Common use case: spawning a compiler-like tool.)

• Semantic: We should not encourage developers to fail on non-UTF-8 in the common case. I think get_stdout should return bytes, and we should offer a get_stdout_string that returns a UTF-8 String. Similarly, the error type should have methods stdout and stderr rather than stdout_bytes and stderr_bytes.

• Semantic (exotic): I have some use cases (involving UNIX sockets in datagram mode) for which I cannot keep reading stdout until EOF, and instead I have to stop reading stdout as soon as 1) the process exits and 2) a non-blocking read returns nothing. Would it be possible (either in-std or outside std) to implement a reader with that semantic atop these APIs?

• Semantic: Please document that we need impl Error for ProcessError. Also, that implementation is going to have a little trouble implementing the cause method, unless there's a semantic guarantee that only one of the types of error can happen. This seems like it'd be guaranteed for errors produced by the standard library, but the APIs for building a ProcessError don't statically guarantee that, nor do the accessor methods for ProcessError. If we could maintain that guarantee, such that there's only one chained error, that seems ideal.

• Semantic: I think we should show the command-line arguments in the default case, even though they might be verbose. However, sometimes the command-line arguments might be sensitive. For convenience for such applications, perhaps a hide_args() method that avoids including the args in the error. (We could get more exotic with a .masked_arg() method that supplies and masks one argument to the command, but that's much more complex and I don't think that should be in this RFC. hide_args() seems simple enough though.)

• Semantic: I can imagine the convenience value of the _line variant, but could you give some examples of code that would benefit from it? I'm also wondering if it'd be reasonably convenient to call read_stdout_string and then have a convenience method on String for "fail if not exactly one line", which seems useful in other contexts too.

• Semantic (minor): Please add a note to the "Further necessary APIs for SubprocessError" section that these APIs could be under a separate feature gate and stabilized separately; that'll make it easier to potentially do a partial stabilization.

[afetisov]

I think get_stdout should return bytes,

Shouldn't it be OsString?

However, sometimes the command-line arguments might be sensitive. For convenience for such applications, perhaps a hide_args() method that avoids including the args in the error. 

Sensitive things should not be passed as command line arguments, since those can leave traces in arbitrary places around the OS. For example, they may be included in ps output. We should not encourage half-baked security practices.

[joshtriplett]

Shouldn't it be OsString?

No. On Windows that would be WTF-8, not raw bytes.

[tmccombs]

I agree with the motivation that Command is not very ergonomic, especially around the common "return error containing stderr on error" workflow. But this RFC is a little too opinionated about stderr in my opinion. It strictly treats it as the "error output",

Um? That's not true. In my RFC, by default, error output is sent to the Rust program's stderr (inherit).

But what if you also want to capture stderr?

As @joshtriplett mentioned output currently gives you interleaved stdout/stderr, and if that is deprecated, there isn't a way to get that. Perhaps one way to do that would be to add a new constructor to Stdio that would allow you to redirect stderr to stdout or vice versa?

And should there be functions for getting stdout and stderr as separate Strings or Vec<u8>s? Of course it is possible to implement that using spawn and Child, but that is true of all these other more ergonomic functions as well.

[thomcc]

I think this is well-motivated broadly, however needs some work:

1. I think deprecating Command::output is probably too aggressive, or at least needs a lot more justification.
2. I agree with @joshtriplett in that there's a lot bikeshed-painting to be done about the naming here -- most of these names don't follow the conventions of the ecosystem/stdlib that well.
3. I think the mutable/constructable SubprocessError portion of the API needs more work -- Ideally it could be a lot smaller and have fewer moving pieces. Perhaps it's trying to be too much at once?

[thomcc]

It's not clear to me that Command::output is that bad. It's slightly awkward and requires manually checking errors, but I'm not convinced this is worth deprecating a extremely widely-used function and causing a large amount of ecosystem churn (which many users will respond to with #[allow(deprecated)], due to MSRV+the existing method not being broken).

I think you need to make a much clearer case for why this needs deprecation, and document the downside of ecosystem and educational churn as folks move to the new API(s).

[ijackson]

I think you and Josh are right about this. I have moved the deprecation to Future possibilities.

[thomcc]

Which UTF-8 conversion does this refer to? I think this probably should be pushed down to whatever getter would return something UTF-8 validated rather than having this stored as a field on the error.

If this is stored (and almost certainly if it is not), we should use std::std::Utf8Error rather than a &std::string::FromUtf8Error in order to avoid needing to store the invalid UTF8 twice (and to avoid forcing us to convert eagerly).

[ijackson]

I'm using std::str::FromUtf8Error which doesn't contain or reference the string, but does contain the offset. I think this doesn't suffer from the problem you describe?

[thomcc]

I think too much of this is mutable -- either we should just make these be public fields (we have get_ and set_ methods for them...) or we should just allow constructing one of these from parts.

Additionally it's not clear to me how these interact. Up until now, I had imagined that there was an internal enumeration

enum SubprocessErrorCause {
    Spawn(io::Error),
    Communication(io::Error),
    // Actually, I don't think we want this one, but anyway
    Utf8(str::Utf8Error),
    // ...
}

but this API has individual setters for each one, which implies it either is not that way, or they have perhaps-confusing interactions.

Does it make any sense for there to be more than one error here anway? Which would gets reported in std::error::Error::source?

[ijackson]

I have made this a transparent struct. Making it opaque was indeed confusing.

[thomcc]

I think we should instead ask users who need a Cloneable SubprocessError to use Arc<SubprocessError>, rather than imposing that requirement on the internals.

[ijackson]

Fair enough, I have changed this.

[joshtriplett]

This feels a little awkward to use: if you call this to filter out some exit statuses, you lose the rest of the error, which means you don't have stdout_bytes anymore. What code pattern would you suggest for code that wants to run diff, check the exit code for 0/1 to determine if there was a difference, bail on >1, and capture the patch output?

[joshtriplett]

I wonder if this (straw proposal) might be easier to use:

/// If the process is only considered to have errored due
/// to its exit code, calls the provided filter, returning
/// `Ok(Self)` if it returns `true`, or `Err(Self)` if it
/// returns `false`.
pub fn permit_statuses(self, f: impl FnOnce(ExitStatus) -> bool) -> Result<ProcessError, ProcessError>

This would make it relatively straightforward to say "allow 0 or 1, reject everything else". (Still mildly annoying due to code() returning Option, but doable.)

[ijackson]

I quite like this idea. I wasn't sure people would like the notion of returning Ok(ProcessError), but if you think that's OK it seems OK to me.

mildly annoying due to code() returning Option

I wondered whether the closure should receive ExitCode (so would only be called if the process exited rather than was killed). But then you can't use this to ignore (say) SIGPIPE.

I do kind of hate booleans. Would impl FnOnce(ExitStatus) -> Result<(), ()> be too awful?

[joshtriplett]

Nit (emphatically not a blocker): your commit message "Rename to ChildOutputStream" seems inaccurate (specifically the "to"), as it renames ChildOutputStream to ChildOutputReader.

[joshtriplett]

Can you give an example where multiple different things could simultaneously go wrong? It seems like spawn_error, communication_error, and utf8_error should be mutually exclusive.

The only things that seem like they could simultaneously go wrong would be "exit status was nonzero" and one of the errors processing output. And I think in that case, exit status should be printed by ProcessError, and the other error should be in cause.

[ijackson]

I think the following can all occur in the same execution:

• The application calls .stderr(piped) and .read_stdout()
• Spawn succeeds
• The subprocess produces some invalid-UTF output
• Then, something in the kernel goes very badly awry (or, some other thing in our Rust program wrecks our fd) and read() on one of the pipes fails
• The subprocess prints some stderr (which the application has said to treat as an error)
• The subprocess then crashes.

We must report at least all of: the error from read; the stderr output (which might well be very helpful for diagnosis); and the process's exit status.

I think we should eagerly do a unicode validity check whenever we captured the stdout, and record the unicode conversion error in the ProcessError. That way a caller who calls your .permit_status() and gets Ok can know that the stdout output is UTF-8; conversely, if the command unexpectedly printed non-UTF-8, the caller can conveniently report the command line etc.

Or to put it another way, we shouldn't "short circuit" some checks and report only a subset of the errors.

[joshtriplett]

Suggested change

	This is not a good idea, because command line arguments are generally public on Unix.
	This is not a good idea in portable software, because command line arguments are often public on Unix targets.

[joshtriplett]

Suggested change

	constructed outside std, for example by async frameworks.
	constructed outside std, such as by async frameworks or other
	code launching processes and handling errors from them.

[joshtriplett]

I've nominated this for the next libs-api meeting. I anticipate proposing FCP on it, unless folks raise any other issues before then.

[ijackson]

I had been thinking about this, and in particular the implementation, and felt that communication_error: Option<io:Error> was suboptimal, because it makes handling of subprocess communication errors less correct (the kind() isn't very useful). See the reasoning I have left in the comment there.

I'm not 100% sure whether this is better than my previous proposal. One option woudl be to leave this one way or the other for now, and let the implementor decide.

[joshtriplett]

Could you clarify how stdout_reader makes avoiding this deadlock easier?

Is this just that you need to interleave .write calls to the process stdin with .read calls to the reader, rather than calling .write_all and then reading all the output at once?

[ijackson]

The deadlocks that stdout_reader avoids completely are ones ike: waiting for the command's stdout EOF, and child termination, in the wrong order; waiting for stderr EOF (which can sometimes block forever for complicated reasons).

It helps only a little with deadlocks resulting from piping both into and out of the same Command. (I think it may still help in those use cases, but it doesn't help with "you wrote too much to the command's input while it's trying to write output to you".)