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

Tracking Issue for windows_process_extensions_main_thread_handle #96723

Open
1 of 3 tasks
nico-abram opened this issue May 5, 2022 · 3 comments
Open
1 of 3 tasks

Tracking Issue for windows_process_extensions_main_thread_handle #96723

nico-abram opened this issue May 5, 2022 · 3 comments
Labels
A-process Area: std::process and std::env C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@nico-abram
Copy link
Contributor

nico-abram commented May 5, 2022

Feature gate: #![feature(windows_process_extensions_main_thread_handle)]

This is a tracking issue for a new main_thread_handle(&self) function on a new ChildExt trait for windows.
The winapi function used to create a process on windows returns a structure that has both a main thread handle and a process handle. std currently discards the thread handle, but that handle can be useful when resuming a process that was created as suspended since that is usually done with ResumeThread. Finding that thread from the process handle can be a bit convoluted, so it'd be nice if std exposed it on a windows-specific extension trait.

Public API

// std::process

pub trait ChildExt: Sealed {
    #[unstable(feature = "windows_process_extensions_main_thread_handle", issue = "96723")]
    fn main_thread_handle(&self) -> RawHandle;
}

Steps / History

@nico-abram nico-abram added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 5, 2022
@nico-abram nico-abram changed the title Tracking Issue for windows_process_extensions_thread_id Tracking Issue for windows_process_extensions_main_thread_handle May 5, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this issue May 10, 2022
Expose process windows_process_extensions_main_thread_handle on Windows

~~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:
```rs
#![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.

Trakcing issue rust-lang#96723
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Expose process windows_process_extensions_main_thread_handle on Windows

~~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:
```rs
#![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.

Trakcing issue rust-lang/rust#96723
@TYPEmber
Copy link

TYPEmber commented Jan 20, 2023

how about make main_thread_handle as Option<Handle>?

Except creating a child process, it is difficult for us to get the main_thread_handle of another process. If we make this item Option, we could extend struct Process's use case. Such as building a process manager, which holds some process's handle without main_thread_handle.

@nico-abram
Copy link
Contributor Author

I don't think the Process struct defined in library/std/src/sys/windows/process.rs is part of the public API. And the fields are private, and only used for std::process::Child. As far as I understand, if/when APIs which deal with non-child processes are added to std the change you mentioned can be done internally, and it should be tranparent to code outside std.

@workingjubilee workingjubilee added the A-process Area: std::process and std::env label Jul 22, 2023
@madsmtm
Copy link
Contributor

madsmtm commented Dec 27, 2023

Windows documentation generally describe this as the "primary" thread (see e.g. here), so a consideration would be to rename this method to reflect that?

Then again, there is precedence for the name "main thread" on macOS (and a few other BSDs), and in Rust's own documentation, so maybe the current name is fine.

facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Feb 8, 2024
Summary: Change `resume_process` implementation to get main thread handle through [unstable api](rust-lang/rust#96723) in std::process::Child and not use `CreateToolhelp32Snapshot`, because iteration over treads cause performance issue.

Reviewed By: stepancheg

Differential Revision: D53472748

fbshipit-source-id: 2589ade412363cf96f5fcb2f896517d4650d0235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-process Area: std::process and std::env C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants