os/exec: .Run() should not return as error on non-zero status on *NIX-like platforms

[johnnybubonic]

What version of Go are you using (go version)?

$ go version
go version go1.17.6 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/[REDACTED]/.cache/go-build"
GOENV="/home/[REDACTED]/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/[REDACTED]/go/pkg/mod"
GONOPROXY="[REDACTED],[REDACTED]/*,github.com/[REDACTED]/*"
GONOSUMDB="[REDACTED],[REDACTED]/*,github.com/[REDACTED]/*"
GOOS="linux"
GOPATH="/opt/dev/go"
GOPRIVATE="[REDACTED],[REDACTED]/*,github.com/[REDACTED]/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.6"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/[REDACTED]/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1498898721=/tmp/go-build -gno-record-gcc-switches"

What did you do?

N/A; bug in design.

exec.Cmd.Run() currently returns an error on non-zero exit status.

This is not only poor design logic as many commands return non-zero even upon success, but it hampers access to getting the status code of a command- requiring .Wait() to be invoked instead.

A careful note is that it's common assumption that POSIX or some other standard defines 0 as success and n > 0 as failure. To stress, this is erroneous - it is convention, not standard.

An immediate example that comes to mind is sendmail's EX_TEMPFAIL, status code 75. This indicates that a message was deferred, not failed, and will be retried (or the user should retry, depending on your MTA spooling or lack thereof).

What did you expect to see?

N/A

What did you see instead?

N/A

Proposed Resolution

I propose the following.

Obviously changing e.g. exec.Run() execution with a non-zero exit code flagging an error is not the proper way forward, as that's a significant API interface change that likely many people are depending on.

Instead, there should be "non-erroring" methods added, e.g. .RunNonZero() and .WaitNonZero() (I'm not crazy about the names, but...) that will return the *os.ProcessState (nil if error NOT related to command exit code) and error (NOT returned if command has non-zero exit code).

[ianlancetaylor]

The result of the exec.(*Cmd).Run method is the same as the result of the exec.(*Cmd).Wait method. Either way, you can retrieve the wait status via err.(*exec.ExitError).Sys().(syscall.WaitStatus).ExitStatus(). I don't think there is anything to change here.

[johnnybubonic]

The process state return is tertiary to the actual issue of non-zero exit statuses being treated as golang errors.

[seankhliao]

Even in your example, a non zero exit code indicates a partial success/failure, deviating from a complete success, and potentially requiring extra actions. Reporting it as an error seems reasonable; in go, errors are values, and it's expected to inspect and handle them as necessary.

[ianlancetaylor]

Adding new methods like RunNonZero and WaitNonZero will just present exactly the same information in a slightly different way. You can do that yourself with wrappers around the methods. It doesn't need to be in the standard library.

[johnnybubonic]

By this logic, why not return an error if unmarshaling JSON encounters a key named error or errors? This, also, is a convention with a high amount of prevalence. Do you understand what I'm saying here? It satisfies the exact state you describe:

a non zero exit code indicates a partial success/failure

The error interface encapsulating (and thus making assumptions about environment and implementation) responses sent by interfaces external to golang code is erroneous. error should be reserved for golang errors, not inferred by assumptions about the external interface that the code is interacting with.

[ianlancetaylor]

@johnnybubonic It seems to me that you are basically making a style argument. The same information is available either way. The os/exec package has picked a style. We would need a very good reason to change that style at this point. A good reason would be "I can't write this kind of code." An argument about the intended meaning of the error interface is not a good argument. Maybe the os/exec package should have been written differently, but it wasn't, and we aren't going to change it now. Sorry.

[mpx]

Go 1.12 added (*ProcessState).ExitCode which significantly simplifies retrieving the exit status:

func exitCode(err error) int {
    if e, ok := err.(interface{ExitCode() int}); ok {
        return e.ExitCode()
    }   
    return -1
}

Using something like exitCode makes it easy to interpret exit status however you want without any further API changes.

Absent a pre-existing API, I agree an argument could be made the processes that return an exit status do exit "successfully". However, there is an API and adding further complexity doesn't seem like an improvement.

Perhaps it might be useful to add a func ExitCode(err error) int helper function to os/exec with some documentation to simplify using the exit status and make it more easily discoverable?

[johnnybubonic]

Go 1.12 added (*ProcessState).ExitCode which significantly simplifies retrieving the exit status:
(SNIP)

Missed this, thanks. This does at least make things a little less cumbersome/a little more clean and readable.

(SNIP)
Perhaps it might be useful to add a func ExitCode(err error) int helper function to os/exec with some documentation to make it more easily discoverable?

I think this is a good and fair approach. I do have my doubts that any PR/patch I submitted would be accepted, as it seems to be something endemic to the Golang core team (no offense intended, of course) - I have a CL in go-review that's seen no human activity since November (when I submitted it), so if it's going to be implemented, I suspect it'd have to be by someone who's already had commits accepted.

[cherrymui]

For API changes (including adding new API), it needs to go through the proposal process, see https://golang.org/s/proposal . Would you like to make this a proposal, by editing the title and the first message to reflect your latest proposed API? Thanks.

[ianlancetaylor]

@johnnybubonic Sorry your CL has not received any attention. It's fine to ping a CL.

[gopherbot]

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)