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 std::process::id #44971

Closed
tmccombs opened this Issue Oct 2, 2017 · 11 comments

Comments

Projects
None yet
8 participants
@tmccombs
Copy link
Contributor

tmccombs commented Oct 2, 2017

The standard library provides a way to get the process id of a child process with std::process::Child::id(), but doesn't have any way to get the current processes id (the equivalent of getpid).

It is possible to do this with libc::getpid, but it seems to me that this should be something that is part of the standard library, especially since it already has cross-platform support for process ids of child processes.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Oct 3, 2017

Seems reasonable to add!

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Oct 4, 2017

GetCurrentProcessId on Windows.

tmccombs added a commit to tmccombs/rust that referenced this issue Oct 6, 2017

tmccombs added a commit to tmccombs/rust that referenced this issue Oct 6, 2017

tmccombs added a commit to tmccombs/rust that referenced this issue Oct 6, 2017

kennytm added a commit to kennytm/rust that referenced this issue Oct 25, 2017

@bors bors closed this in #45059 Oct 26, 2017

@sfackler sfackler reopened this Nov 19, 2017

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Nov 19, 2017

Reopening as the tracking issue.

@EdSchouten

This comment has been minimized.

Copy link
Contributor

EdSchouten commented Dec 28, 2017

While porting Rust over to CloudABI, a sandboxed UNIX-like runtime environment, I ran into the issue that there is no way for me to implement this function. As we're aiming to use CloudABI in distributed settings, there is no traditional 16/32-bit process identifier. Instead, processes may be identified by a UUID. This is useful, as integer process identifiers can easily collide when dealing with many instances of a job (due to the birthday paradox).

As this function was only introduced recently, would it still be possible to alter it to return an opaque, but displayable data type instead? That said, almost all of the other functions provided by this module are also broken on CloudABI. The traditional fork()/execve() model doesn't lend well to sandboxing. Maybe it makes more sense to disable std::process on CloudABI entirely.

@tmccombs

This comment has been minimized.

Copy link
Contributor Author

tmccombs commented Dec 28, 2017

I'm opposed a completely opaque data type, because that limits the usefulness. For example, that makes it difficult to use in C (or dbus) that require a PID. However, I think it would be alright if the it was a semi-opaque type with OS-specific API's to get the underlying representation (in std::os::unix, etc). However, that would break consistency with std::process::Child::id, which is already stabilized. I'm not sure how you would deal with that either.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 29, 2017

As @tmccombs has mentioned, the ship has already sailed here due to Child::id. More generally, though. I'm not sure it makes sense to avoid supporting getpid of all things because some platform decided not to implement it.

@EdSchouten

This comment has been minimized.

Copy link
Contributor

EdSchouten commented Dec 29, 2017

One thing that would be low-hanging fruit would be to at least have some kind of type alias for this. On UNIX/Windows, it could be defined as i32. On CloudABI, it could be a string. That way, existing code that uses i32 continues to build on existing platforms, whereas less conventional platforms can choose to implement this function differently.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 17, 2018

Given this:

That said, almost all of the other functions provided by this module are also broken on CloudABI. The traditional fork()/execve() model doesn't lend well to sandboxing. Maybe it makes more sense to disable std::process on CloudABI entirely.

and this:

the ship has already sailed here due to Child::id

I’m inclined to stabilize std::process::id as-is. (Returning u32.)

@rfcbot fcp merge

Hopefully the portability lint can eventually help a CloudABI port to "cleanly" implement this differently, or not at all.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 17, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@SimonSapin SimonSapin changed the title getpid function Tracking issue for std::process::id Mar 17, 2018

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 19, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 29, 2018

The final comment period is now complete.

bors pushed a commit that referenced this issue Apr 2, 2018

bors added a commit that referenced this issue Apr 2, 2018

Auto merge of #49574 - tmccombs:stabilize-getpid, r=sfackler
Stabilize `std::process::id()`

Fixes #44971

@bors bors closed this in #49574 Apr 2, 2018

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Apr 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.