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

Implemented `getpid(p::Process)`. #24064

Closed
wants to merge 5 commits into from

Conversation

8 participants
@abalkin
Copy link
Contributor

commented Oct 9, 2017

Closes #4752.

@abalkin abalkin force-pushed the abalkin:issue-4752 branch from 09fa282 to 9156fb4 Oct 9, 2017

@vtjnash

vtjnash approved these changes Oct 9, 2017

Get the process ID of a running process. Returns `-1` if the
process is not running.
"""

This comment has been minimized.

Copy link
@rfourquet

rfourquet Oct 9, 2017

Contributor

What about alternatives when the process is not running, like throwing, or returning nothing ?

This comment has been minimized.

Copy link
@abalkin

abalkin Oct 9, 2017

Author Contributor

Returning nothing would violate type stability. Throwing is an option, but I feel returning -1is more convenient. There was also a suggestion to save the pid so that it can be inspected after the process has ended. I don't think this is a good idea because pids can be reused and having getpid(p) return a potentially unrelated pid is error prone. (E.g. you don't want to accidentally kill an unrelated process.)

Also, as @vtjnash mentioned before, we can take guidance from nodejs where child pid is accessible via a property which does not throw.

This comment has been minimized.

Copy link
@abalkin

abalkin Oct 9, 2017

Author Contributor

I ran an experiment with node, and it looks like they cache the pid:

> const { spawn } = require('child_process');
> x = spawn('sleep',  ['1']);
> x.exitCode
0
> x._handle
null
> x.pid
45660

I still don't think this is a good idea for the reasons I mentioned above.

This comment has been minimized.

Copy link
@vtjnash

vtjnash Oct 9, 2017

Member

I think this is essentially the same question as #18145. But upon reflection, I think an error would be better here. In some places, -1 is used as a special value to mean "all processes". So kill(getpid(p)) could accidentally cause the system to reboot (surprise!) with this choice of sentinel value.

This comment has been minimized.

Copy link
@abalkin

abalkin Oct 9, 2017

Author Contributor

Fair enough. I'll make the change. Just to make sure: caching the pid is off the table.

This comment has been minimized.

Copy link
@rfourquet

rfourquet Oct 9, 2017

Contributor

nothing may make it type unstable, but this seems to be the new way of returning what was a Nullable (ie. returning Union{T, Void} or Union{Some{T},Void}), and getpid is a function for which I would tend to return a Nullable.

@@ -335,6 +335,20 @@ end
pipe_reader(p::Process) = p.out
pipe_writer(p::Process) = p.in

"""
getpid(p::Process) -> Int32

This comment has been minimized.

Copy link
@rfourquet

rfourquet Oct 9, 2017

Contributor

Why not return Int?
EDIT: oh I see, getpid() returns Int32 already, but still...

This comment has been minimized.

Copy link
@abalkin

abalkin Oct 9, 2017

Author Contributor

Right. I just copied that line from the Base.getpid() doc. The actual type is Cint, but I think that is the same as Int32 on all the supported platforms.

This comment has been minimized.

Copy link
@vtjnash

vtjnash Oct 9, 2017

Member

Yes, this was the right choice (matching getpid())

This comment has been minimized.

Copy link
@rfourquet

rfourquet Oct 9, 2017

Contributor

What about changing that to Int for both methods in another PR? for example, one method of kill (with ClusterManager) accepts a pid as Int.

This comment has been minimized.

Copy link
@abalkin

abalkin Oct 9, 2017

Author Contributor

@rfourquet - isn't that a different ID? I thought kill(::ClusterManager, ..) takes the worker id, not the OS process id.

This comment has been minimized.

Copy link
@rfourquet

rfourquet Oct 9, 2017

Contributor

It seems you are right.

@abalkin abalkin force-pushed the abalkin:issue-4752 branch from 2c5226d to 311b202 Oct 9, 2017

@Keno

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

This seems to have fallen through the cracks, but seems fine to me. @vtjnash ?

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

Somewhat tangential to this, but I think that we should use Int for process IDs.

@stevengj

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

Bump

@stevengj stevengj force-pushed the abalkin:issue-4752 branch from 311b202 to 1626f82 Jul 30, 2018

@stevengj

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Rebased.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Our getpid is actually Libc.getpid. Should we add a method to that (even though such a signature doesn't exist in libc)?

@iamed2

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

Our getpid is actually Libc.getpid. Should we add a method to that (even though such a signature doesn't exist in libc)?

I think it would make sense to have the Libc getpid extend Base.getpid. If there are further uses for getpid that don't use Libc (e.g., some package implements some other process management system) it'll look weird to have to extend it from there again.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Well, we don't really have a way to do that exactly. Base.getpid and Libc.getpid are either the same function or two functions.

@iamed2

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

Oh, I meant make them the same function but put it in the Base namespace rather than Libc

@stevengj

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

@IAMED, other packages can already extend it via Base.pid(...) = ..., no? So it's not clear to me how it matters in practice if it's defined first in Base or Libc.

@iamed2

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

Oh you're right, my bad, that's something good for me to remember in the future. Sorry to muddy the waters.

stevengj added some commits Jul 30, 2018

@vtjnash

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

Thanks for the PR! Sorry it took us such a long time to land.

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.