Feat(process): Implement Command::spawn_with

[PaulDance]

Motivation

#7238: under Windows, the spawn sometimes has to be configured in a specific way in order to achieve a specific behavior or to stay compatible with some other API. In our case, it is about spawning child processes as PPL when the parent (Rust) process currently runs as PPL.

In the standard library, it is currently supported through the nightly-only windows_process_extensions_raw_attribute feature. It was previously implemented by configuring the Command object using its raw_attribute extension method and then the regular spawn added the attributes on the Windows process when appropriate. This meant that simply creating and configuring a std::process::Command object was sufficient to spawn Tokio processes since there is impl From<std::process:Command> for tokio::process::Command.

However, the unstable API has recently been changed and the attribute setting must now be explicitly performed at spawn time through a separate spawning method: Command::spawn_with_attributes. This means that the previous solution does not work anymore.

Solution

Since nightly APIs cannot currently be consumed from Tokio in any way, including when put behind some sort of cfg gate such as tokio_unstable, this proposes to add a broader API that enables customizing what is used in order to actually perform the synchronous process spawn while letting the rest stay as is.

This makes for a relatively small change and straightforward API addition. In particular, it allows keeping the nightly-only code outside of Tokio and more generally any kind of desired spawn customization, although not strictly necessary for the initial use case.

However, it could be considered too vague for such a niche use case while it could be replaced with a more specific spawn_with_attributes when the upstream feature is stabilized, even though it could take a long time, but this is the best comprise found for now. This is why the linked issue is not marked here as to be closed: it circumvents the problem at hand, but does not more directly solve it.

Testing

No tests covering the new API have been added. Indeed, the standard library's side has landed a bit too recently (rust-lang/rust#123604, 1.85) it seems compared to the nightly distribution seen in CI so the Windows-specific part could be tested, while there does not seem to be nightly-only tests yet either. Also, considering the current spawn API has been internally rewritten in terms similar to spawn_with's, the current tests should already cover this adequately.

[Darksonn]

cc @ipetkov

[ipetkov]

Overall the changes seem sensible! Just have a few minor comments around cutting down unnecessary monomorphization

[PaulDance]

Something like this?

[Darksonn]

That's weird. The FreeBSD 64-bit run failed with this:

failures:

---- runtime::tests::queue::stress1 stdout ----

thread 'runtime::tests::queue::stress1' panicked at tokio/src/runtime/tests/queue.rs:214:9:
assertion `left == right` failed
  left: 499997
 right: 500000
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    runtime::tests::queue::stress1

test result: FAILED. 131 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.27s

This is not a known spurious failure. I'm filing a bug ...

[PaulDance]

Yeah, I had the same reaction 😅

I triggered a new pipeline since I could not find a way to restart the Cirrus job through GH, so we will see.

It ended up succeeding, so it does seem like a spurious failure indeed.

[Darksonn]

LGTM. Just one nit.

[Darksonn]

I noticed that github will set the resulting commit to be authored by [email protected], which does not match the email in the SoB tag you have included. Is this something you wish to update before I merge the PR?

[PaulDance]

Ah yes, I didn't know I had this inconsistency. I now disabled this proxying, so it should be better?

[Darksonn]

Now it wants to use your hotmail rather than the harfanglab.fr address you put in the SoB.

[PaulDance]

Eh, whatever, fine by me 🤷

[Darksonn]

Thanks for the PR!

[PaulDance]

Many thanks!

[piscisaureus]

This is very useful, thank you!
🙏 for this to become stable.

[Darksonn]

This was added as unstable as a temporary measure until stdlib stabilizes spawn_with_attributes. Did you have a different use-case in mind?

[Pistonight]

@Darksonn I had a different use case for this: I wanted to take the stdout/stderr of a child and pipe it to another command like the example here: https://doc.rust-lang.org/std/process/struct.Stdio.html#impl-From%3CChildStdout%3E-for-Stdio

(Maybe there's another way to do it other than my approach below, please let me know)

I could take it from tokio::process::ChildStdout, but that involves converting it from sync to async and back to sync, which seems unnecessary:

#[tokio::main]
async fn main() {
    let hello = Command::new("echo")
        .arg("Hello, world!")
        .stdout(Stdio::piped())
        .spawn() // converted to nonblocking io here
        .expect("failed echo command");
    
    let std_io: Result<Stdio, _> = hello.stdout.unwrap().try_into(); // extra try_into() and unwrap() compared to the std example
    
    let reverse = Command::new("rev")
        .stdin(std_io.unwrap())
        .output().await.unwrap();
    // works, but not ideal
    assert_eq!(reverse.stdout, b"!dlrow ,olleH\n");
}

What I really want to do:

    let mut std_io: Option<Stdio> = None;
    let hello = Command::new("echo")
        .arg("Hello, world!")
        .stdout(Stdio::piped())
        .spawn_with(|cmd| {
            let mut child = cmd.spawn()?;
            std_io = child.stdout.take().map(Stdio::from); // take control of the stdout handle
          // ^ does not compile due to being inside Fn closure
            Ok(child)
        })
        .expect("failed echo command");
    
    let reverse = Command::new("rev")
        .stdin(std_io.take().unwrap()) // put it in directly without converting it to non-blocking
        .output().await.unwrap();
    
    assert_eq!(reverse.stdout, b"!dlrow ,olleH\n");

Note that this doesn't compile and currently has to be worked around with RefCell unless the closure is upgraded to FnMut or FnOnce, but it works once I do that

[Darksonn]

If you have change requests, then please file a new issue.

As for upgrading the closure to FnOnce, I would be happy to see a PR for that.