Expose process windows_process_extensions_main_thread_handle on Windows

[nico-abram]

I did not find any tests in https://github.com/rust-lang/rust/blob/7d3e03666a93bd2b0f78b3933f9305832af771a5/library/std/src/sys/windows/process/tests.rs that actually launch processes, so I haven't added tests for this. I ran the following locally, to check that it works as expected:

#![feature(windows_process_extensions_main_thread_handle)]

fn main() {
    use std::os::windows::process::{ChildExt, CommandExt};
    const CREATE_SUSPENDED: u32 = 0x00000004;

    let proc = std::process::Command::new("cmd")
        .args(["/C", "echo hello"])
        .creation_flags(CREATE_SUSPENDED)
        .spawn()
        .unwrap();

    extern "system" {
        fn ResumeThread(_: *mut std::ffi::c_void) -> u32;
    }
    unsafe {
        ResumeThread(proc.main_thread_handle());
    }

    let output = proc.wait_with_output().unwrap();
    let str_output = std::str::from_utf8(&output.stdout[..]).unwrap();
    println!("{}", str_output);
}

Without the feature attribute it wouldn't compile, and commenting the ResumeThread line makes it hang forever, showing that it works.

Tracking issue #96723

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

[retep998]

Why not expose the thread handle instead of the thread id?
We expose the process handle directly instead of the process id, so why is the main thread different?

[ChrisDenton]

Why not expose the thread handle instead of the thread id?

Agreed, although are there any downsides is holding a thread handle open?

[nico-abram]

I didn't realize there was precedent for exposing the process handle directly, just saw the id() method and missed AsRawHandle. You're right that would be better. Modified the patch to use handles instead of ids

[ChrisDenton]

Btw, there are tests that run processes in src\process\tests.rs. I think the distinction is that src\sys\windows\process\tests.rs is more for testing functions internal to sys\windows while src\process\tests.rs tests the std APIs. But don't quote me on that 😉.

[nico-abram]

Added a test

[ChrisDenton]

Ok, so my concern was that keeping the thread handle open is an insta-stable change so I wanted to be sure this wouldn't cause breakage. However, I think I've satisfied myself that closing it at the same time as the process handle won't be an issue for existing code. The worst that can happen is some kernel data structures are needlessly kept around in the event that a child process exits its main thread but the process continues running other threads (which would itself be a very unusual situation).

My only other concern is that perhaps new APIs should be making use of the (currently unstable) BorrowedHandle<'_>, assuming it will be stabilized sometime soon-ish. But that could be added as a possibility on the tracking issue.

[ChrisDenton]

I'll ask @rust-lang/libs-api if they're ok with this.

[retep998]

The worst that can happen is some kernel data structures are needlessly kept around in the event that a child process exits its main thread but the process continues running other threads (which would itself be a very unusual situation).

Can this even happen? As far as I can tell, when the main thread exits Windows will exit the process anyway.

[m-ou-se]

My only other concern is that perhaps new APIs should be making use of the (currently unstable) BorrowedHandle<'_>, assuming it will be stabilized sometime soon-ish. But that could be added as a possibility on the tracking issue.

Might be good to do that right away. Then we get some usage of BorrowedHandle before stabilizing it, to see if it works well. :)

[ChrisDenton]

Can this even happen? As far as I can tell, when the main thread exits Windows will exit the process anyway.

It's possible! In Win32, a process only exits when all threads have exited or ExitProcess is called (which terminates all other threads),

[ChrisDenton]

Might be good to do that right away. Then we get some usage of BorrowedHandle before stabilizing it, to see if it works well. :)

Good point. @nico-abram could you update the implementation to return BorrowedHandle

[nico-abram]

Rebased on master and changed the API to use BorrowedHandle

[ChrisDenton]

Looks good! Just a few small points.

[ChrisDenton]

Great, thanks! If you could squish the commits then I think this will be ready to be merged.

[nico-abram]

Squished

[ChrisDenton]

@bors r+

[bors]

📌 Commit 5368ea7 has been approved by ChrisDenton

[oli-obk]

@bors rollup