Add interrupt function to std::process::Child

[JonathanWoollett-Light]

Proposal

Problem statement

With running a binary it is common their will be artefacts which should be removed on ending execution (e.g. a unix socket). When developing such a binary, integration tests will need to run the binary and then clean-up afterwards.

At the moment it is not clear how to do this, this should be a convenient and clear.

Motivation, use-cases

At the moment you might do this like:

let child = std::process::Command("/path/to/my/binary").spawn().unwrap();
unsafe {
    libc::kill(child.id() as i32, libc::SIGINT);
}
let exit = child.wait().unwrap();
assert_eq!(exit,Some(0));

Yet if you are in the rarer circumstance where you don't trust the running application and/or need to short-circuit it, it is significantly easier to do:

let child = std::process::Command("/path/to/my/binary").spawn().unwrap();
child.kill().unwrap();
let exit = child.wait().unwrap();
assert_eq!(exit,Some(0));

I would forward that really SIGINT should be the more common use case than SIGKILL so it is odd that we have a convenience method for SIGKILL but not SIGINT.

Solution sketches

The simple solution would be to add an interrupt function to std::process::Child that behaves simiarlly to the kill function except sends the SIGINT signal.

let child = std::process::Command("/path/to/my/binary").spawn().unwrap();
child.interrupt().unwrap();
let exit = child.wait().unwrap();
assert_eq!(exit,Some(0));

Notably in the above example it is not guaranteed that the program will exit, especially if it captures the signal then hangs, this may lead to code where you want to interrupt the process then after a given timeout kill the process:

let child = std::process::Command("/path/to/my/binary").spawn().unwrap();
child.interrupt().unwrap();
let now = std::time::Instant::now();
let exit_status = loop {
    if now.elapsed() >= std::time::Duration::from_millis(500) {
        break None;
    }
    match child.try_wait() {
        Ok(Some(status)) => break Some(status),
        Ok(None) => {},
        Err(err) => panic!(err)
    }
}.unwrap_or_else(|| {
    child.kill().unwrap();
    child.wait().unwrap()
});
assert_eq!(exit,Some(0));

This case may warrant a interrupt_timeout function, such that this code could look like:

let child = std::process::Command("/path/to/my/binary").spawn().unwrap();
child.interrupt_timeout(std::time::Duration::from_millis(500)).unwrap();
let exit_status = child.wait().unwrap();
assert_eq!(exit,Some(0));

Links and related work

I've already made a partial PR on this: rust-lang/rust#101387

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

[ChrisDenton]

I'll note that Windows does not have a direct equivalent to this. Therefore it may need to be a Unix extension.

[programmerjake]

the windows equivalent would probably be something like GenerateConsoleCtrlEvent (allows simulating typing ctrl+c at a console) or sending a WM_CLOSE message.

[ChrisDenton]

For sure, it can be done but it isn't exactly equivalent. As you say, the GenerateConsoleCtrlEvent is for console applications but works by sending the crtl-c event to all programs in the group (not just a target process). This may even include your own process. For GUI applications, it's necessary to enumerate all top level windows of the process to send the WM_CLOSE message.

So this is rather more complex than the Unix case and a somewhat more niche feature.

[the8472]

If we're doing a unix-specific thing then I think a general signal-sending thing would be better than specifically interrupt. And if we do that it should be on a trait that can later be implemented on other things, e.g. PidFd. Maybe unix::ProcessExt (process instead of child).

[joshtriplett]

I agree: since this needs to be a UNIX extension anyway, let's support sending signals.

That said, signal numbers are not portable, so we'll need to define signal constants for the common subset that works on every UNIX platform. man 7 signal has a table of signal names and corresponding standards.

[the8472]

#[non_exhaustive]
enum Signal {
  Term,
  Kill,
  Interrupt,
  // a bunch more...
  PlatformSpecific(u32)
}

[joshtriplett]

PlatformSpecific is great for input, but causes problems for output: if we add a new variant, matching PlatformSpecific can fail. I would suggest an accessor function instead.

[thomcc]

We already have places in std::os::unix where we use i32 for signals, for example https://doc.rust-lang.org/std/os/unix/process/trait.ExitStatusExt.html#tymethod.signal. #88 proposed some more std::os::unix signal-using functionality, and used i32 there too.

I somewhat feel that any new API here should follow suit (rather than something type-safe/high-level like an enum or wrapper), otherwise it will feel weird and unfortunate for these existing APIs not to use it.

That is, IMO the drawback of inconsistency (specifically, inconsistency in a way that makes the existing APIs feel bad) is not worth the benefit of using something higher level for this. (I guess I could be convinced that input vs output makes a big difference here, though).

(This is relevant to #88 as well, which is using i32 for signals).

[the8472]

I somewhat feel that any new API here should follow suit (rather than something type-safe/high-level like an enum or wrapper), otherwise it will feel weird and unfortunate for these existing APIs not to use it.

"weird and unfortunate" suggests that the precedent is bad and perhaps shouldn't be followed? Plus we can offer conversion methods between a higher-level enum and the i32 values.

[JonathanWoollett-Light]

I think it is non-ideal to use a i32 over an enum here.

I can't see a better implementation than:

enum Signal {
    Term,
    Kill,
    Interrupt,
    // ...
    Custom(u32)
}
impl From<Signal> for i32 {
    fn from(signal: Signal) -> Self {
        match signal {
            Signal::Term => 15,
            Signal::Kill => 9,
            Signal::Interrupt => 2,
            // ...
            Signal::Custom(x) => x as i32,
        }
    }
}
fn signal(sig: Signal) {
    raw_signal(i32::from(sig))
}
fn raw_signal(sig: i32) {
    // ...
}

The unfortunate thing is, the more we push for the largest amount of compatibility we quickly regress back to an implementation which is near identical to libc::kill(pid: pidt_t, sig: c_int).

[thomcc]

"weird and unfortunate" suggests that the precedent is bad and perhaps shouldn't be followed? Plus we can offer conversion methods between a higher-level enum and the i32 values.

If we have a high level representation for signals, then yes, it would feel unfortunate that we don't use it consistently. I don't think that is enough to justify having one, though.

[thomcc]

IOW, if we do add a higher level signal type, I think we should be prepared to deprecate the existing functions which work with signals and replace them with versions that use it.

I'm not a particular fan of that, but I don't think these are so widely used that there would be much churn.

[jkugelman]

It's worth noting that SIGINT is the signal sent when a user presses Ctrl+C. Programs that kill other programs conventionally send SIGTERM.

[Amanieu]

We discussed this in the libs meeting and think this would be good to add. Some notes on the specific design points:

• This should be a unix-specific extension trait.
• This can just use the name send_signal, there is no need use a generic name like "interrupt".
• This should just take an i32 for the signal number. An enum is probably not justified here.

[RalfJung]

This should just take an i32 for the signal number. An enum is probably not justified here.

So if I want to send a SIGINT, how do I do that? This sounds like I pretty much have to also depend on libc to learn the signal numbers for the current target? Hard-coding a number certainly shouldn't be encouraged.