Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: add ProcessState.ExitCode #26539

Closed
benhoyt opened this issue Jul 22, 2018 · 17 comments
Closed

os: add ProcessState.ExitCode #26539

benhoyt opened this issue Jul 22, 2018 · 17 comments

Comments

@benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Jul 22, 2018

After a bit of discussion on this golang-nuts thread, I propose that we add an ExitStatus() int method to os.ProcessState so that it's significantly simpler to get the exit status of a finished process, and doesn't require the syscall package.

Motivation (some of this copied from the thread): I struggled to get the exit status integer of a command executed with os/exec. I followed the documentation through ExitError and ProcessState, but could only find the ProcessState.Success() boolean. After searching Google+StackOverflow I found you can get it, but it requires importing syscall and converting to a syscall type -- pretty klunky.

Here's roughly the code snippet that I'm currently using:

err = cmd.Wait()
if err != nil {
    if exitErr, ok := err.(*exec.ExitError); ok {
        if status, ok := exitErr.Sys().(syscall.WaitStatus); ok {
            return status.ExitStatus()
        }
    }
    return -1
}
return 0

Which is problematic because of all the boilerplate, but also because syscall is platform-specific, and fetching an exit code works on all major platforms (Unix/Linux/macOS/Windows). Even on Plan 9 ExitStatus() is implemented (though admittedly it's a bit of a hack as it just checks to see whether an error message was returned). So this would work for all major systems, and on Plan 9 would just act like the above syscall function that already exists, returning 0 on success or 1 on error.

os.ProcessState already has a Success() bool method, so this proposal would add ExitStatus() int alongside that. It would return the exit status integer (and possibly be documented to return -1 if the process hasn't finished). This would enable the above code to be something like this:

err = cmd.Wait()
if err != nil {
    if exitErr, ok := err.(*exec.ExitError); ok {
        return exitErr.ExitStatus() // ExitError embeds ProcessState
    }
    return -1
}
return 0

Or actually just this:

_ = cmd.Wait()
return cmd.ProcessState.ExitStatus()

There are various reasons for needing the exit code value:

  • To log it in a structured way
  • To switch on the exit code when running a utility that returns different well-defined codes for different kinds of errors that you need to detect
  • To display it in (say) a bold font in a UI control
  • To return it from a system() function when implementing a scripting language (my particular case)

This exists in other languages, for example Python's subprocess has "returncode" and Java's exec has exitValue().

For what it's worth, @ianlancetaylor said (on the linked golang-nuts thread) that this "seems reasonable to me".

@gopherbot gopherbot added this to the Proposal milestone Jul 22, 2018
@gopherbot gopherbot added the Proposal label Jul 22, 2018
@agnivade agnivade changed the title proposal: add ExitStatus() method to os.ProcessState proposal: os: add ExitStatus() method to ProcessState Jul 23, 2018
@wgliang

This comment has been minimized.

Copy link
Contributor

@wgliang wgliang commented Jul 23, 2018

If there were no one working on this, I can help add it.:)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 23, 2018

Change https://golang.org/cl/125443 mentions this issue: os: add ExitStatus() method to ProcessState

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Jul 23, 2018

@wgliang - The proposal is still in a discussion phase, and not yet accepted. Once it is accepted, you are most welcome to work on it. But I see you have already sent a CL. :)

@bcmills

This comment has been minimized.

@bradfitz bradfitz changed the title proposal: os: add ExitStatus() method to ProcessState proposal: os: add ExitStatus method to ProcessState Jul 23, 2018
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 23, 2018

SGTM. I have vague memories of discussion of this earlier and recall that Plan 9 not having integer exit codes being part of the reason we don't have this.

But if Windows & Unix both have them, I see no reason not to add it. But if there are Plan 9 related objections, perhaps that could be addressed in naming the method something else, possibly more specific. But current name is fine.

Your CL would need docs.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jul 23, 2018

Kind of unfortunate that os.Exit takes 'code int' while syscall.WaitStatus's method is ExitStatus (not ExitCode). I usually think of the "status" as the code with extra random bits I don't want mixed in (like the C int that is the argument of all the macros in the wait(2) man page). Also StatusCode in HTTP is the int while Status is the string.

But given that os.Exit exists and takes an int, I see no portability reason to refuse to return that int in a portable way. (Not worried about Plan 9 here.)

Let's call it ProcessState.ExitCode (not ExitStatus) and return -1 (not undefined behavior) if Exited() == false.

@rsc rsc closed this Jul 23, 2018
@rsc rsc reopened this Jul 23, 2018
@rsc rsc modified the milestones: Proposal, Go1.12 Jul 23, 2018
@rsc rsc changed the title proposal: os: add ExitStatus method to ProcessState os: add ExitStatus method to ProcessState Jul 23, 2018
@rsc rsc changed the title os: add ExitStatus method to ProcessState os: add ProcessState.ExitCode Jul 23, 2018
@wgliang

This comment has been minimized.

Copy link
Contributor

@wgliang wgliang commented Jul 24, 2018

I've updated the PR, PTAL.:)

@mpx

This comment has been minimized.

Copy link
Contributor

@mpx mpx commented Jul 24, 2018

Perhaps ExitCode() should return an error to advise that the process doesn't have an exit code? Eg:

  • the process hasn't terminated yet (not started, failed to start, still running or suspended)
  • the process terminated with a signal

This feels like one of those situations where an inline error is likely to result in a surprising unchecked outcome. Especially since a terminated process doesn't necessary have an exit code (Eg, exit via signal).

Signals are very system specific, so I don't think there is any need for a similar ExitSignal() method. That probably should be unpacked the usual way.

@benhoyt

This comment has been minimized.

Copy link
Contributor Author

@benhoyt benhoyt commented Jul 24, 2018

@mpx I would much prefer it be clearly documented to return -1 in these cases per @rsc . Like with calling Success(), it's up to the caller to only call it after a Wait() or the process has otherwise finished.

I don't think it will result in a surprising error, since zero is the only "success" status, so folks will be doing things like if exitStatus != 0 { handleError(exitStatus) } and -1 is not zero, so it'll all work as expected.

As for signals, I don't know enough about them to comment. Python does them as -N where N is the POSIX signal number (docs), but that feels a bit overloaded. I'd prefer just -1 in these cases.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 24, 2018

Like with calling Success(), it's up to the caller to only call it after a Wait() or the process has otherwise finished.

Experience with context.Context suggests that users will generally not respect that sort of invariant (see #19856).

I would not be at all surprised if we end up with users confused by unexpected -1 return-values. On the other hand, the point of the proposal is to reduce boilerplate, and returning (code int, ok bool) would add some of that boilerplate back. I would suggest that we panic if the process has not yet exited, but that's bad for almost exactly the same reason as returning -1: if the code path is untested, we're going to get some failure mode one way or another.

If we had some sort of run-time reporting for failed assertions without panicking (a proposal I intend to make soon), we could report the error and return -1. So my preference would be to formally define ExitCode before Exited to be a user error but return a single value (-1) anyway for now.

@mpx

This comment has been minimized.

Copy link
Contributor

@mpx mpx commented Jul 25, 2018

@benhoyt Even if the code guarantees Wait has been called you should still check for -1 since the process could have terminated via a signal. That is usually interesting separate from a normal exit.

I don't think using (int, error) adds much boilerplate compared to using an inline -1 to describe "unknown failure". Eg:

if c := p.ExitCode(); c == -1 {
  return errors.new("exit via signal, or broken invariant")
} else if c != wantCode {
  return fmt.Errorf("bad exit code %d", c)
}

vs

if c, err := p.ExitCode(); err != nil {
  return err
} else if c != wantCode {
  return fmt.Errorf("bad exit code %d", c)
}

In general, using err has a couple of advantages:

  • Go developers are familiar with the convention are less likely to ignore odd failures without prior consideration.
  • In the failure case the caller can understand what really when wrong (SIGSEGV, etc..)

If we want to reduce boilerplate further, I tend to use a helper function like:

// MatchExitCode returns nil when the *exec.ExitError matches one of the provided codes.
// Otherwise an error describing the exit reason is returned.
func MatchExitCode(err error, want ...int) error

err := exec.Command("foo").Run()
if e := MatchExitCode(err, 5); e != nil {
  return e
}

Using a helper function like MatchExitCode doesn't require any additions to the exist methods and can support all platforms. There is no need to check for other failure modes, but the caller can still log exactly what went wrong.

@benhoyt

This comment has been minimized.

Copy link
Contributor Author

@benhoyt benhoyt commented Jul 25, 2018

I was thinking the normal use case would like more like this:

if err := cmd.Wait(); err != nil {
    switch cmd.ExitCode() {
    case 1:
        logger.Errorf("big bad error")
    case 2:
        logger.Errorf("some other error")
    default:
        logger.Errorf("error %d running program", cmd.ExitCode())
    }
    return
}
logger.Infof("did the thing successfully")

Or maybe even:

_ = cmd.Wait()
switch cmd.ExitCode() {
case 0:
    logger.Infof("did the thing succesfully")
case 1:
    logger.Errorf("big bad error")
case 2:
    logger.Errorf("some other error")
default:
    logger.Errorf("error %d running program", cmd.ExitCode())
}

In either case you'd have some fallback for "unexpected error" which would include the -1. Or if you wanted to check for -1 specially, you'd be welcome to.

@mpx

This comment has been minimized.

Copy link
Contributor

@mpx mpx commented Jul 25, 2018

Using the (int, error) return with if gives roughly the same number of conditionals and also gracefully handles other failure modes with details (signals, broken invariants, Plan9 error messages):

_ = cmd.Wait()
if c, err := cmd.ExitCode(); err != nil {
   return err
} else if c == 1 {
   return errors.New("foo failure")
} else if c == 2 {
   return errors.New("bad failure")
} else if c != goodCode {
  return fmt.Errorf("unknown exit code %d", c)
}
return nil

// Or with a helper function (probably my preference):

waitErr := cmd.Wait()
if c, err := exec.ExitCode(waitErr); err != nil {
...

If converting exit codes to error messages is a common pattern, it's easy enough to abstract via a helper function/map which would be simpler again.

I'm not convinced an API returning -1 is really any simpler than (int, error), but it does lose information that is helpful when debugging a failure. Also, returning an error would work better with Plan9 with it's error messages.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 25, 2018

I would not be at all surprised if we end up with users confused by unexpected -1 return-values.

At least they'll see -1 and know they did something wrong, as opposed to returning 0 in that case.

Let's just proceed with the simple signature approved during the proposal review.

@mpx

This comment has been minimized.

Copy link
Contributor

@mpx mpx commented Jul 25, 2018

The caller might not have done anything wrong - the process might have terminated via a signal. They also wouldn't know what went wrong without the boilerplate that this API attempts to avoid. (int, error) could return -1 on error as well if that is a concern, although it's unlikely to be used due to the standard Go conventions.

A helper function (like func exec.ExitCode(error) (int, error)) is conceptually simpler since it takes the result of an execution (via Wait) instead of using the hopefully terminated ProcessState. This:

  • reduces the chance of misusing the API
  • is generally more useful with all the failure states represented (signal, exit message, etc...). No need for further boilerplate.

It would be trivial to wrap the more general function for anyone who wants to hide all failures behind an inline error. I suspect most developers wouldn't want to do that.

I understand that everyone has their preferences, so just for your consideration. In either case, making the exit code easier to obtain is a step forward.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 25, 2018

Let's just proceed with the simple signature approved during the proposal review.

Agreed. To be clear, I'm not suggesting that we change the signature or the behavior from what was approved: only that we tell users “don't call ExitCode on a running process” instead of “it's fine to call ExitCode on a running process”.

@gopherbot gopherbot closed this in be94dac Aug 28, 2018
@tbehling

This comment has been minimized.

Copy link

@tbehling tbehling commented Aug 9, 2019

For anyone who finds this in a search like I did, this capability was implemented as the following in Go 1.12: https://golang.org/pkg/os/#ProcessState.ExitCode . As described in the first comment, this removes the need to cast the error twice, and the dependency on syscall.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.