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

Fix process_*_pid semantics #2220

Merged
merged 1 commit into from Jan 29, 2019

Conversation

Projects
None yet
3 participants
@toots
Copy link
Contributor

commented Jan 17, 2019

This PR fixes an oversight in PR #1999. When fetching an opened process' PID, the process should not be removed from the list of running processes.

@toots toots force-pushed the toots:fix-find_proc_id branch from 0edbf0a to 162bbe4 Jan 17, 2019

@gasche

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

Instead of passing a remove:bool parameter to find_proc_id, why not have a separate function remove_proc_id that you call from close_* functions after find_proc_id? I think the API would be cleaner that way.

@alainfrisch alainfrisch added this to the 4.08 milestone Jan 17, 2019

@alainfrisch alainfrisch added the bug label Jan 17, 2019

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

I second @gasche's suggestion. Also, could you add a non-regression test for the bug?

@toots toots force-pushed the toots:fix-find_proc_id branch from 162bbe4 to 6d543f6 Jan 17, 2019

@toots

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

@gasche: done
@alainfrisch: what kind of non-regression test are you looking for? The pid/proc hashtable is pretty much encapsulated within the unix module. I could launch a process, get pid twice and see that it doesn't fail but that seems a lot for a test.

@toots toots force-pushed the toots:fix-find_proc_id branch from 6d543f6 to f500058 Jan 17, 2019

@gasche

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

If I understand correctly, the error came as follow: the function find_proc_id existed before #1999, and it had almost the right semantics except for the fact that it removed the pid from the table (a bad choice for a function of this name); you didn't notice it when you reused it in #1999, hence the bug. Now you noticed it and you are about to fix it; a priori, not only your calls to find_proc_id must be reviewed, but also pre-#1999 uses of the function, to make sure their semantics is preserved (with a better API).

Now: in threads/unix.ml, there seems to be two definitions of find_proc_id and, for example, two occurrences of let close_process_in. Why is that? It seems that you only edited the first copy, is that really what should be done?

(I haven't looked in enough details to understand the cause of duplication. It seems fairly suspicious and probably should be removed?)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

I could launch a process, get pid twice and see that it doesn't fail

Yes, this seems reasonable. Independently of the non-regression aspect, this would also serve as a unit test for those new functions, which is a good idea.

@toots toots force-pushed the toots:fix-find_proc_id branch from f500058 to 4e56833 Jan 20, 2019

@toots

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

Good catch on duplicate code in otherlibs/threads/unix.ml, I've removed it. I also added a test as discussed with @alainfrisch.

I see that 4.08 has been branched off. It would be timely if this PR could also make it there before the release. @damiendoligez, should I push another PR against that branch?

@gasche: the full diff for both #1999 and #2220 for a full review is:

--- a/otherlibs/threads/unix.ml
+++ b/otherlibs/threads/unix.ml
@@ -1118,71 +1118,51 @@ let open_process_full cmd =

 let find_proc_id fun_name proc =
   try
-    let pid = Hashtbl.find popen_processes proc in
-    Hashtbl.remove popen_processes proc;
-    pid
+    Hashtbl.find popen_processes proc
   with Not_found ->
     raise(Unix_error(EBADF, fun_name, ""))

+let remove_proc_id proc =
+  Hashtbl.remove popen_processes proc
+
+let process_in_pid inchan =
+  find_proc_id "process_in_pid" (Process_in inchan)
+let process_out_pid outchan =
+  find_proc_id "process_out_pid" (Process_out outchan)
+let process_pid (inchan, outchan) =
+  find_proc_id "process_pid" (Process(inchan, outchan))
+let process_full_pid (inchan, outchan, errchan) =
+  find_proc_id "process_full_pid"
+    (Process_full(inchan, outchan, errchan))
+
 let close_process_in inchan =
-  let pid = find_proc_id "close_process_in" (Process_in inchan) in
+  let proc = Process_in inchan in
+  let pid = find_proc_id "close_process_in" proc in
+  remove_proc_id proc;
   close_in inchan;
   snd(waitpid_non_intr pid)

 let close_process_out outchan =
-  let pid = find_proc_id "close_process_out" (Process_out outchan) in
+  let proc = Process_out outchan in
+  let pid = find_proc_id "close_process_out" proc in
+  remove_proc_id proc;
   (* The application may have closed [outchan] already to signal
      end-of-input to the process.  *)
   begin try close_out outchan with Sys_error _ -> () end;
   snd(waitpid_non_intr pid)

 let close_process (inchan, outchan) =
-  let pid = find_proc_id "close_process" (Process(inchan, outchan)) in
-  close_in inchan;
-  begin try close_out outchan with Sys_error _ -> () end;
-  snd(waitpid_non_intr pid)
-
-let close_process_full (inchan, outchan, errchan) =
-  let pid =
-    find_proc_id "close_process_full"
-                 (Process_full(inchan, outchan, errchan)) in
-  close_in inchan;
-  begin try close_out outchan with Sys_error _ -> () end;
-  close_in errchan;
-  snd(waitpid_non_intr pid)
-
-let find_proc_id fun_name proc =
-  try
-    let pid = Hashtbl.find popen_processes proc in
-    Hashtbl.remove popen_processes proc;
-    pid
-  with Not_found ->
-    raise(Unix_error(EBADF, fun_name, ""))
-
-let rec waitpid_non_intr pid =
-  try waitpid [] pid
-  with Unix_error (EINTR, _, _) -> waitpid_non_intr pid
-
-let close_process_in inchan =
-  let pid = find_proc_id "close_process_in" (Process_in inchan) in
-  close_in inchan;
-  snd(waitpid_non_intr pid)
-
-let close_process_out outchan =
-  let pid = find_proc_id "close_process_out" (Process_out outchan) in
-  close_out outchan;
-  snd(waitpid_non_intr pid)
-
-let close_process (inchan, outchan) =
-  let pid = find_proc_id "close_process" (Process(inchan, outchan)) in
+  let proc = Process(inchan, outchan) in
+  let pid = find_proc_id "close_process" proc in
+  remove_proc_id proc;
   close_in inchan;
   begin try close_out outchan with Sys_error _ -> () end;
   snd(waitpid_non_intr pid)

 let close_process_full (inchan, outchan, errchan) =
-  let pid =
-    find_proc_id "close_process_full"
-                 (Process_full(inchan, outchan, errchan)) in
+  let proc = Process_full(inchan, outchan, errchan) in
+  let pid = find_proc_id "close_process_full" proc in
+  remove_proc_id proc;
   close_in inchan;
   begin try close_out outchan with Sys_error _ -> () end;
   close_in errchan;
diff --git a/otherlibs/unix/unix.ml b/otherlibs/unix/unix.ml
index dd1d770ff..caf9d95f3 100644
--- a/otherlibs/unix/unix.ml
+++ b/otherlibs/unix/unix.ml
@@ -1108,34 +1108,51 @@ let open_process_full cmd =

 let find_proc_id fun_name proc =
   try
-    let pid = Hashtbl.find popen_processes proc in
-    Hashtbl.remove popen_processes proc;
-    pid
+    Hashtbl.find popen_processes proc
   with Not_found ->
     raise(Unix_error(EBADF, fun_name, ""))

+let remove_proc_id proc =
+  Hashtbl.remove popen_processes proc
+
+let process_in_pid inchan =
+  find_proc_id "process_in_pid" (Process_in inchan)
+let process_out_pid outchan =
+  find_proc_id "process_out_pid" (Process_out outchan)
+let process_pid (inchan, outchan) =
+  find_proc_id "process_pid" (Process(inchan, outchan))
+let process_full_pid (inchan, outchan, errchan) =
+  find_proc_id "process_full_pid"
+    (Process_full(inchan, outchan, errchan))
+
 let close_process_in inchan =
-  let pid = find_proc_id "close_process_in" (Process_in inchan) in
+  let proc = Process_in inchan in
+  let pid = find_proc_id "close_process_in" proc in
+  remove_proc_id proc;
   close_in inchan;
   snd(waitpid_non_intr pid)

 let close_process_out outchan =
-  let pid = find_proc_id "close_process_out" (Process_out outchan) in
+  let proc = Process_out outchan in
+  let pid = find_proc_id "close_process_out" proc in
+  remove_proc_id proc;
   (* The application may have closed [outchan] already to signal
      end-of-input to the process.  *)
   begin try close_out outchan with Sys_error _ -> () end;
   snd(waitpid_non_intr pid)

 let close_process (inchan, outchan) =
-  let pid = find_proc_id "close_process" (Process(inchan, outchan)) in
+  let proc = Process(inchan, outchan) in
+  let pid = find_proc_id "close_process" proc in
+  remove_proc_id proc;
   close_in inchan;
   begin try close_out outchan with Sys_error _ -> () end;
   snd(waitpid_non_intr pid)

 let close_process_full (inchan, outchan, errchan) =
-  let pid =
-    find_proc_id "close_process_full"
-                 (Process_full(inchan, outchan, errchan)) in
+  let proc = Process_full(inchan, outchan, errchan) in
+  let pid = find_proc_id "close_process_full" proc in
+  remove_proc_id proc;
   close_in inchan;
   begin try close_out outchan with Sys_error _ -> () end;
   close_in errchan;
diff --git a/otherlibs/unix/unix.mli b/otherlibs/unix/unix.mli
index 78d60df70..78e8386dd 100644
--- a/otherlibs/unix/unix.mli
+++ b/otherlibs/unix/unix.mli
@@ -843,6 +843,30 @@ val open_process_args_full :

     @since 4.08.0 *)

+val process_in_pid : in_channel -> int
+(** Return the pid of a process opened via {!Unix.open_process_in} or
+   {!Unix.open_process_args_in}.
+
+    @since 4.08.0 *)
+
+val process_out_pid : out_channel -> int
+(** Return the pid of a process opened via {!Unix.open_process_out} or
+   {!Unix.open_process_args_out}.
+
+    @since 4.08.0 *)
+
+val process_pid : in_channel * out_channel -> int
+(** Return the pid of a process opened via {!Unix.open_process} or
+   {!Unix.open_process_args}.
+
+    @since 4.08.0 *)
+
+val process_full_pid : in_channel * out_channel * in_channel -> int
+(** Return the pid of a process opened via {!Unix.open_process_full} or
+   {!Unix.open_process_args_full}.
+
+    @since 4.08.0 *)
+
 val close_process_in : in_channel -> process_status
 (** Close channels opened by {!Unix.open_process_in},
    wait for the associated command to terminate,
diff --git a/otherlibs/win32unix/unix.ml b/otherlibs/win32unix/unix.ml
index a2d8dde19..3105b6da9 100644
--- a/otherlibs/win32unix/unix.ml
+++ b/otherlibs/win32unix/unix.ml
@@ -1000,31 +1000,48 @@ let open_process_full cmd =

 let find_proc_id fun_name proc =
   try
-    let pid = Hashtbl.find popen_processes proc in
-    Hashtbl.remove popen_processes proc;
-    pid
+    Hashtbl.find popen_processes proc
   with Not_found ->
     raise(Unix_error(EBADF, fun_name, ""))

+let remove_proc_id proc =
+  Hashtbl.remove popen_processes proc
+
+let process_in_pid inchan =
+  find_proc_id "process_in_pid" (Process_in inchan)
+let process_out_pid outchan =
+  find_proc_id "process_out_pid" (Process_out outchan)
+let process_pid (inchan, outchan) =
+  find_proc_id "process_pid" (Process(inchan, outchan))
+let process_full_pid (inchan, outchan, errchan) =
+  find_proc_id "process_full_pid"
+    (Process_full(inchan, outchan, errchan))
+
 let close_process_in inchan =
-  let pid = find_proc_id "close_process_in" (Process_in inchan) in
+  let proc = Process_in inchan in
+  let pid = find_proc_id "close_process_in" proc in
+  remove_proc_id proc;
   close_in inchan;
   snd(waitpid [] pid)

 let close_process_out outchan =
-  let pid = find_proc_id "close_process_out" (Process_out outchan) in
+  let proc = Process_out outchan in
+  let pid = find_proc_id "close_process_out" proc in
+  remove_proc_id proc;
   close_out outchan;
   snd(waitpid [] pid)

 let close_process (inchan, outchan) =
-  let pid = find_proc_id "close_process" (Process(inchan, outchan)) in
+  let proc = Process(inchan, outchan) in
+  let pid = find_proc_id "close_process" proc in
+  remove_proc_id proc;
   close_in inchan; close_out outchan;
   snd(waitpid [] pid)

 let close_process_full (inchan, outchan, errchan) =
-  let pid =
-    find_proc_id "close_process_full"
-                 (Process_full(inchan, outchan, errchan)) in
+  let proc = Process_full(inchan, outchan, errchan) in
+  let pid = find_proc_id "close_process_full" proc in
+  remove_proc_id proc;
   close_in inchan; close_out outchan; close_in errchan;
   snd(waitpid [] pid)
Fix process_*_pid semantics: do not remove process from list of running
processes when fetching its pid. Remove duplicate code in
otherlibs/threads/unix.ml
@gasche

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

I believe that the change is correct, and tracked down the moment where the duplication was introduced. Removing it changes the behavior of the code slightly (it makes close_process_out ignore close_out exceptions), but I believe it is correct.

Minor nitpicking: I guess I would have used an auxiliary find_and_remove_proc_id function to make the diff smaller, but the current version seems fine to me.

@gasche

gasche approved these changes Jan 21, 2019

@toots

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Any update on this one? Would be nice to get it merged in time for the 4.08 release.. 🙂

@gasche gasche merged commit 2956845 into ocaml:trunk Jan 29, 2019

2 checks passed

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

gasche added a commit that referenced this pull request Jan 29, 2019

Merge pull request #2220 from toots/fix-find_proc_id
Fix process_*_pid semantics

(cherry picked from commit 2956845)
@gasche

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

Thanks for the ping! Merged, and cherry-picked in 4.08 ( 809a402 ).

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.