Make process exit status more flexible to allow for handling when a child is terminated by signal

[miselin]

The UvProcess exit callback is called with a zero exit status and non-zero termination signal when a child is terminated by a signal.

If a parent checks only the exit status (for example, only checks the return value from wait()), it may believe the process completed successfully when it actually failed.

Helpers for common use-cases are in std::rt::io::process.

Should resolve #10062.

[huonw]

Maybe this should return an enum like:

enum ProcessExit {
   ExitOk, // status == 0, could be merged into ExitStatus.
   ExitStatus(int),
   ExitSignal(int)
}

[miselin]

Working on the enum solution - it is significantly nicer.

This would of course break any existing code that uses std::rt::io::process as the form of a result has changed. However, being able to determine that a child process terminated due to a signal (and did not therefore complete successfully) is a huge plus.

[alexcrichton]

It's fine to break existing code, but I think that there should definitely be some tests accompanying this change. It's fine to only run the tests on some platforms (getting a forking/shell tests working on both windows/android is annoying), but this should not get regressed on.

[miselin]

I have implemented the enum and added a few helpers to std::rt::io::process to avoid having to do a match on the enum everywhere.

@alexcrichton I agree, so I added a test to run-pass/rtio-processes.rs that runs a bash instance that simply sends a SIGHUP to itself and confirms that we get a termination signal that matches SIHGUP (1). This will run on unixish but not Android.

[huonw]

Would it be possible to write a test that invokes itself with arguments that will raise a signal, e.g.

fn main() {
    let args = os::args();
    if args.len() >= 2 && args[1] == "signal" {
        // called like `./testname signal`, so raise a signal...
    } else {
        // work out the path to this executable.
        let mut self_path = os::self_exe_path().unwrap();
        self_path.push(args[0]);
        let status = run::process_status(format!("{}", self_path.display()), 
                                         [~"signal"]);
        assert_eq!(status, ExitSignal(/* whatever */));
    }
}

(This might be unreasonably complicated for a test.)

[huonw]

Can these all be methods? i.e.

impl ProcessExit {
     fn is_success(&self) -> bool { ... }

     fn maybe_get_exit_code(&self) -> Option<int> { ... }

     // etc
}

[alexcrichton]

Hm ok reader the uv code, it looks like exec errors are reported in the callback (which is a little unfortunate). I also think that the current handling of the error codes is pretty wrong, and I think that the callback should get modified slightly.

I think that the uv ExitCallback should take a ProcessExit and a watcher explicitly, and the conversion should look like:

match term_signal {
  0 => match exit_status {
    n if n < 0 => ExitExecError(UvError(n)),
    n => ExitStatus(n),
  },
  n => ExitSignal(n),
}

I'm not entirely certain if you can have a clean negative exit status, but I that this will correctly propagate the uv error. Note that ExitExecError is an explicit UvError (or maybe it's an IoError), not an integer (because this error is based off errno). Also, I think that we should have a test for something like executing a non-existent program and make sure that this gets reported as the correct ExitExecError. Right now I think that it's a nonzero exit status which is not correct.

[miselin]

We actually have a raw UvError passed in to the exit callback from libuv, so we don't need to create one from exit_status.

Will work on fixing up these things - the utility functions for example should've been methods from the very beginning.

Just seeking some clarification @alexcrichton when you say the ExitCallback should take a ProcessExit, are you referring explicitly to the type ExitCallback, or to the members of the UvProcess struct?

There's a test in rtio-processes.rs for a program which does not exist, I'll look at fixing that up to check errno rather than the exit status.

[alexcrichton]

Right now the function in uv/process.rs which calls the exit callback uses incorrect logic to figure out what the UvError is to return. The above logic I was talking about should replace this, and yes I think that the ExitCallback should be modified to take a ProcessExit

[miselin]

Updated with input from review and rebased onto current master with the libuv crate move. No local regressions with a make check.

Still not totally sure on the UvError semantics that should be used (not currently certain how to pull in UvError from librustuv to libstd). My preference would be to expose the errorno field on the libuv process type to Rust. We could then use that field as the first thing to match on, as if that is non-zero we have hit an error of some description in executing the task. However that also seems to be delving a bit deeper into the internals of libuv and its data types than may be wise.

I am also uncertain about the updated code to handle problems running cc and friends in link.rs (and the rustpkg calls of the same type). The main thing is what will happen there if the user overrides the program to use (if possible?) and overrides it to something that does not exist?

@huonw - RE the test that invokes itself. Sure, I can look into doing that. Is there any advantage to doing it that way over the current test, which sends itself a SIGHUP via bash?

[huonw]

Is there any advantage to doing it that way over the current test, which sends itself a SIGHUP via bash?

It is more platform independent than calling out to sh; i.e. it should work on any platform that supports running external processes, whereas the sh one only works when sh is around.

[alexcrichton]

This seems to be a pretty common switch, and it looks like the case of a fork error is commonly forgotten as well. I think that a good way to handle this might be to have a to_str() or fmt::Default implementation for ProcessExit which becomes one of:

• error code: 100
• signal: 10
• exec error: failed to allocate memory

And then error messages like this could be format!("failure produced {}", status)

[alexcrichton]

This is really awesome, thanks so much for this excellent work! I think the only real remaining bump is ExitExecError(int) vs ExitExecError(IoError). The unfortunate part of the latter is that this enum becomes move-by-default because of the contained Option<~str>. I don't think that this will cause problems, but you might end up turning up something (hopefully not!).

Also, were you able to figure out what happened if you spawned a process of a nonexistent program? I would expect that to return an ExitExecError, and if so we should have a test for that.

[miselin]

Working on these now.

@alexcrichton had a quick look and the pattern matching against an IoError requires a move - I really haven't looked too closely yet though while I do the formatter and such.

@huonw fair point! I have no idea what happens when you segfault on Windows with respect to exit codes or "signals" - and I don't have a Windows machine handy. I could do up a test with this PR that works on unixy systems, and see what it does when it hits CI on Windows?

[miselin]

@alexcrichton the latest commit has everything bar the IoError change. This includes modifying the existing test in rtio-processes.rs that runs a non-existent file to not only check for failure, but to also check spawn_error.

@huonw I have added a new test that uses more or less the code you suggested, and checks for termination by signal. I have specifically avoided checking the exact value of the signal to avoid having to deal with system specifics (eg, SIGBUS vs SIGSEGV vs Windows), resorting to only is_some() as the confirmation.

[huonw]

I have a feeling this isn't correct: doesn't this attempt to compile the #[test] in this file (i.e. all zero of them :P ) into a testsuite, and should just be removed?

[miselin]

Oops!

[alexcrichton]

Isn't args[0] equivalent to self_path? It seems like this push is simply overriding what was previously there.

[miselin]

With the latest commit, run::tests::test_finish_with_output_twice is the only failure in the full testsuite (on my system).

This is because I have not yet made ProcessExit and IoError cloneable, and therefore we have no way of cloning the status to allow multiple calls of finish_and_output.

I feel like this code is a lot hackier as a result of trying to make this work and I don't like it as much, but I've put it up as an RFC to make sure I'm going about things the right way.

[alexcrichton]

This should probably still say linking with

[miselin]

Oops! Missed that in the diff before I pushed. Cheers.

[miselin]

Pushed current work.

@alexcrichton just waiting now on word about libuv (and watching the related issue over there).

[alexcrichton]

I'm upgrading libuv as part of #10321, after that lands this should be ready for a rebase.

[alexcrichton]

Aha! I've finally landed 10321, so this should be ready for a rebase now.

[miselin]

@alexcrichton rebased and should be good to go (the builders might disagree, though I've done tests).

[alexcrichton]

I don't think that this function exists any more

[eddyb]

This is UB and has remained unfixed on master, the address should be 1 or something else, not NULL.
Might work on windows too then.