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

API to retrieve high level process's PIDs #1999

Merged
merged 1 commit into from Aug 28, 2018

Conversation

Projects
None yet
2 participants
@toots
Copy link
Contributor

commented Aug 21, 2018

This PR adds an API to retrieve the PID of processes opened with the high-level open_process* functions.

It is particularly useful in situations where a call to close_process* is stuck waiting for a faulty process. Typical use would be to spawn an asynchronous task and send SIGTERM/SIGKILL to that pid if the process is still running.

Additionally, it could be used to communicate with the child process with signals such as SIGHUP and etc.

@toots toots force-pushed the toots:process_pid branch 3 times, most recently from 0eec14e to 625d458 Aug 21, 2018

@@ -1124,28 +1124,38 @@ let find_proc_id fun_name proc =
with Not_found ->
raise(Unix_error(EBADF, fun_name, ""))

let process_in_pid inchan =
find_proc_id "close_process_in" (Process_in inchan)

This comment has been minimized.

Copy link
@nojb

nojb Aug 22, 2018

Contributor

The string literal is displayed in case of error and should correspond to the calling function in each case, so ideally it should be "process_in_pid" when calling process_in_pid, and "close_process_in" when calling close_process_in (same for the other functions).

This comment has been minimized.

Copy link
@toots

toots Aug 22, 2018

Author Contributor

Gotcha.

Changes Outdated
@@ -55,6 +55,9 @@ Working version
Unix.open_process{,_in,_out,_full}, but passing an explicit argv array.
(Nicolás Ojeda Bär, review by Jérémie Dimino, request by Volker Diels-Grabsch)

- GPR#1999: Add Unix.process{,_in,_out,_full}_pid to retrieve opened process's
pid.

This comment has been minimized.

Copy link
@nojb

nojb Aug 23, 2018

Contributor

Please add author and reviewer information.

@nojb

nojb approved these changes Aug 23, 2018

Copy link
Contributor

left a comment

Approving as the proposed addition seems reasonable: the PR just exposes already existing functions, it has clear motivations (e.g. terminate a hung process), cannot be implemented from outside Unix and works for all platforms.

Will merge in a few days if there are no objections.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

Also, please squash the "Fix function name" commit into the first commit.

@toots toots force-pushed the toots:process_pid branch from c7ea67a to f5051da Aug 23, 2018

@toots

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

Awesome. I've added author and reviewer in changes and rebased all the commits. Thank you!

@nojb nojb merged commit db9671f into ocaml:trunk Aug 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@toots toots referenced this pull request Jan 17, 2019

Merged

Fix process_*_pid semantics #2220

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.