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

process: implement raw_handle getter on Child (#3987) #3998

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

passcod
Copy link
Contributor

@passcod passcod commented Jul 27, 2021

Implementation of #3987. /ping @ipetkov

Notes from discussion in issue:

  • The process id returned by Child.id() is not the Windows handle.
  • It is possible to obtain a Handle from a PID, but it is without many access privileges that the handle obtained by spawning the child process has.
  • Windows process handles do not suffer from the PID problem where a stale PID can be recycled:

When a new process is created by the CreateProcess function, handles of the new process and its primary thread are returned. These handles are created with full access rights, and — subject to security access checking — can be used in any of the functions that accept thread or process handles. These handles can be inherited by child processes, depending on the inheritance flag specified when they are created. The handles are valid until closed, even after the process or thread they represent has been terminated.
https://docs.microsoft.com/en-us/windows/win32/procthread/process-handles-and-identifiers
(emphasis mine)

Thus, it should be possible to implement the AsRawHandle trait from std, with the signature:

fn as_raw_handle(&self) -> RawHandle

Implementation notes:

  • I implemented AsRawHandle on both imp::Child and the "public" Child: the latter delegates to the former.
  • Despite the discussion above, there is no way to obtain the raw handle after the child has exited, due to the FusedChild design: once the process has exited, all that remains is FusedChild::Done(ExitStatus) — while the underlying std Child instance could very well be queried for the raw handle safely, it's gone.
  • I am unsure what to do:
    • Child.as_raw_handle() panics if FusedChild is Done, or
    • Write a fn as_raw_handle(&self) -> Option<RawHandle> instead, or
    • Change how FusedChild works.

@ipetkov
Copy link
Member

ipetkov commented Jul 27, 2021

Another way around this would be to:

  • not implement the AsRawHandle trait, but use a cfg'd inherent method on the Child struct
  • make the signature return Optional<RawHandle> and pass the panic decision to the caller (which also mirrors our id method signature even though on windows we can technically get a child handle even when the process has exited)

@ipetkov ipetkov linked an issue Jul 27, 2021 that may be closed by this pull request
@ipetkov ipetkov added A-tokio Area: The main tokio crate M-process Module: tokio/process labels Jul 27, 2021
@ipetkov ipetkov self-requested a review July 27, 2021 16:56
@MikailBag
Copy link
Contributor

MikailBag commented Jul 27, 2021

I think you can obtain a handle right after child creation, and then returned this cached handle in AsRawHandle impl.

@ipetkov
Copy link
Member

ipetkov commented Jul 27, 2021

Yes, though that will require increasing the structure size on Windows even if the feature isn't used (though this isn't an absolute blocker).

Another idea that comes to mind is having flexibility on this with our API stability guarantee. If we chose to go with the fallible API now (i.e. return Option<RawHandle>) and later change our mind to make it infallible (e.g. by caching the handle on creation), we can easily mark the inherent method as deprecated and ask users to use the AsRawHandle trait). If we start with the trait now, we cannot migrate to a fallible API after the fact without introducing potential panics in the old usage

@passcod
Copy link
Contributor Author

passcod commented Jul 27, 2021

Sounds good, I'll change it to be fallible

@passcod passcod force-pushed the process/child/as-raw-handle branch 3 times, most recently from e30a934 to 6172080 Compare July 28, 2021 01:58
@passcod
Copy link
Contributor Author

passcod commented Jul 28, 2021

I named it fn raw_handle(&self) -> Option<RawHandle> so if the trait is implemented later the name won’t conflict (one could write AsRawHandle::as_raw_handle(&child), but, meh).

Copy link
Member

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I named it fn raw_handle(&self) -> Option<RawHandle> so if the trait is implemented later the name won’t conflict

Hadn't thought about this, its a great idea!

Overall this LGTM, thanks for the quick implementation @passcod ! I'm going to mark this as approved by me and kick off the CI build and we can merge when green

tokio/src/process/mod.rs Outdated Show resolved Hide resolved
@ipetkov ipetkov changed the title process: implement AsRawHandle on Child (#3987) process: implement raw_handle getter on Child (#3987) Jul 28, 2021
@ipetkov
Copy link
Member

ipetkov commented Jul 28, 2021

Also quick note, now that we're not implementing the AsRawHandle trait directly I'd recommend updating the commit message for posterity 😄

@passcod passcod force-pushed the process/child/as-raw-handle branch from 3bc3a16 to cff3463 Compare July 28, 2021 05:55
@passcod passcod force-pushed the process/child/as-raw-handle branch from cff3463 to cad1184 Compare July 28, 2021 05:58
@passcod
Copy link
Contributor Author

passcod commented Jul 28, 2021

Done that and also fixed up the last rustfmt issue

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ok to me as well.

@ipetkov ipetkov merged commit f957f7f into tokio-rs:master Jul 28, 2021
@ipetkov
Copy link
Member

ipetkov commented Jul 28, 2021

Thanks again @passcod 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-process Module: tokio/process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add as_raw_handle for process::Child
4 participants