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.

Sign up for GitHub

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
Closed

Implemented getpid(p::Process). #24064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57c7a48
Implemented `getpid(p::Process)`.
abalkin Oct 9, 2017
c2be7d5
Make `getpid(p)` throw when p is not running.
abalkin Oct 9, 2017
1626f82
check type of output
stevengj Jul 30, 2018
10138bf
Libc, not Base
stevengj Jul 30, 2018
5e32f42
fix quote
stevengj Aug 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,8 @@ Library improvements
* `getpeername` on a `TCPSocket` returns the address and port of the remote
endpoint of the TCP connection ([#21825]).

* `getpid(::Process)` method ([#24064]).

* `resize!` and `sizehint!` methods no longer over-reserve memory when the
requested array size is more than double of its current size ([#22038]).

Expand Down
11 changes: 11 additions & 0 deletions base/process.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,17 @@ end
pipe_reader(p::Process) = p.out
pipe_writer(p::Process) = p.in

"""
getpid(p::Process) -> Int32
Copy link
Member

@rfourquet rfourquet Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

It seems you are right.


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@abalkin abalkin Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

function Libc.getpid(p::Process)
p.handle != C_NULL || error("process not running")
ccall(:jl_uv_process_pid, Cint, (Ptr{Cvoid},), p.handle)
end

struct ProcessChain <: AbstractPipe
processes::Vector{Process}
in::Redirectable
Expand Down
1 change: 1 addition & 0 deletions src/jl_uv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ void jl_uv_flush(uv_stream_t *stream)
}

// getters and setters
JL_DLLEXPORT int jl_uv_process_pid(uv_process_t *p) { return p->pid; }
JL_DLLEXPORT void *jl_uv_process_data(uv_process_t *p) { return p->data; }
JL_DLLEXPORT void *jl_uv_buf_base(const uv_buf_t *buf) { return buf->base; }
JL_DLLEXPORT size_t jl_uv_buf_len(const uv_buf_t *buf) { return buf->len; }
Expand Down
6 changes: 4 additions & 2 deletions test/spawn.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,12 @@ if valgrind_off
end

# setup_stdio for AbstractPipe
let out = Pipe(), proc = run(pipeline(`$echocmd "Hello World"`, stdout=IOContext(out,stdout)), wait=false)
let out = Pipe(), proc = run(pipeline(`$exename --startup-file=no -e "println(getpid())"`, stdout=IOContext(out,stdout)), wait=false)
pid = getpid(proc)
close(out.in)
@test read(out, String) == "Hello World\n"
@test parse(Int32, read(out, String)) === pid
@test success(proc)
@test_throws ErrorException getpid(proc)
end

# issue #5904
Expand Down