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

Unix.getpid returns wrong result #4034

Closed
vicuna opened this issue May 29, 2006 · 19 comments
Closed

Unix.getpid returns wrong result #4034

vicuna opened this issue May 29, 2006 · 19 comments

Comments

@vicuna
Copy link

vicuna commented May 29, 2006

Original bug ID: 4034
Reporter: Christoph Bauer
Status: acknowledged (set by @xavierleroy on 2006-06-10T09:11:56Z)
Resolution: open
Priority: normal
Severity: feature
OS: Windows
Version: 3.09.2
Category: otherlibs
Tags: patch
Monitored by: @nojb @dra27 @alainfrisch

Bug description

Unix.getpid returns a wrong (non-existing) processid.
To reproduce:

ocaml unix.cma
# Unix.getpid ();;

A working implementation of getpid.c could be

#include <mlvalues.h>
#include "unixsupport.h"

CAMLprim value unix_getpid(value unit)
{
  return Val_int( getpid() );
}

File attachments

@vicuna
Copy link
Author

vicuna commented May 29, 2006

Comment author: Christoph Bauer

Important note: the platform is windows (mingw-version in my case).

@vicuna
Copy link
Author

vicuna commented Jun 8, 2006

Comment author: @xavierleroy

There are no strict equivalent to Unix's process IDs in the Win32 API,
so in what sense is the returned PID "wrong"??

Caml uses process handles as closest equivalents of process IDs for the
Unix.create_process* and Unix.waitpid and Unix.getpid functions. Returning
anything else from Unix.getpid or Unix.create_process* would break Unix.waitpid.

@vicuna
Copy link
Author

vicuna commented Jun 9, 2006

Comment author: Christoph Bauer

There is a function GetCurrentProcessID. The task manager
shows a column with a pid. This pid could be used by an external
program to kill a process or even to run cygwins gdb with the option
--pid=...

winwait.c could make use of OpenProcess, which takes the
pid and returns the handle.

@vicuna
Copy link
Author

vicuna commented Jun 10, 2006

Comment author: @xavierleroy

I keep that as a feature wish.

@vicuna
Copy link
Author

vicuna commented Aug 3, 2006

Comment author: Christoph Bauer

I attached an patch that seems to work:

  • Unix.getpid() returns a pid like the taskmanager
  • create_process + winwait works correct (at least in my test)
  • there is no open process handle.

My tests are done with the mingw version. I found the API
description on MSDN.

In the original implementation closes never the hProcess-Handle
from the PROCESS_INFORMATION struct pi. MSDN states this should
be done. My patch closes this handle directly after create_process,
so this problems is solved, too.

@vicuna
Copy link
Author

vicuna commented Aug 3, 2006

Comment author: Christoph Bauer

Please apply the patch in the directory `ocaml-3.09.2/otherlibs/win32unix'.

@vicuna
Copy link
Author

vicuna commented Jan 22, 2007

Comment author: Christoph Bauer

I tested the patch now a long time and had never a problem with it.

This entry could be counted as bug because of the process handle leak under windows...

@vicuna
Copy link
Author

vicuna commented Jan 30, 2007

Comment author: @xavierleroy

I tried the patch but runs into errors returned by OpenProcess.
I believe the problem is this: if Unix.waitpid is called for a process that has already terminated, its processID is invalid (Windows doesn't keep the process around if its handles are closed) and there is no way to recover its handle and therefore its exit code. Try for instance the following example:

  let ic = Unix.open_process_in "dir c:\\" in
  begin try
    while true do 
      print_string (input_line ic); print_newline()
    done
  with End_of_file -> ()
  end;
  ignore (Unix.close_process_in ic)

Don't you get an error on close_process_in?

@vicuna
Copy link
Author

vicuna commented Feb 1, 2007

Comment author: Christoph Bauer

I guess you are right. With your example I get the error sometimes. So a Unix.sleep and now I get the error more reliably:

let ic = Unix.open_process_in "dir c:\\" in
  begin try
    while true do
      print_string (input_line ic); print_newline()
    done
  with End_of_file -> ()
  end;
  Unix.sleep 1;
  ignore (Unix.close_process_in ic);;

There are two possible solutions. The simple one:
GetLastError() returns ERROR_INVALID_PARAMETER. This could be catched
and a zero exit code could be returned. The main drawback is that the return
code nonsense. (Another drawback for my approach is
that windows theoretical could reuse the given pid.)

The second solution could be a internal table that stores for each pid the process information. Then the job of waitpid would be to free the internal handle. The table could be realized as a simple list or a simple hash table.

I try to implement the second approach.

@vicuna
Copy link
Author

vicuna commented Feb 1, 2007

Comment author: Christoph Bauer

The new patch implements a pid table of PROCESS_INFORMATION. (binary search,
dynamic storage allocation). The code for table management is in two new files pidtable.h and pidtable.c.

@vicuna
Copy link
Author

vicuna commented Mar 16, 2007

Comment author: Christoph Bauer

There's a stupid bug in patch 2b. I'll prepare a new one.

@vicuna
Copy link
Author

vicuna commented Jan 22, 2008

Comment author: Christoph Bauer

The patch 2e was quite good. 2f is a patch against 3.10.1.

@vicuna
Copy link
Author

vicuna commented Dec 16, 2011

Comment author: Christoph Bauer

patch against 3.12.1. compiles with msvc.

@vicuna
Copy link
Author

vicuna commented May 20, 2014

Comment author: Christoph Bauer

patch against 4.01.0 uploaded: ocaml-getpid-4.01.0a.patch

@vicuna
Copy link
Author

vicuna commented Jul 25, 2016

Comment author: gabelevi

(It's very possible that I'm missing some historical information about Windows. I did notice that some functions, like GetCurrentProcessID, were introduced with Windows XP, so maybe Windows didn't used to have a notion of Process IDs).

It does appear that Windows has the concept of Process IDs, in addition to Process Handles. I think it would be better if the various functions that return references to a process (like Unix.getpid and Unix.create_process) and the various functions that take a reference to a process (like Unix.waitpid and Unix.kill) used Process IDs instead of Process Handles.

I have code that I am porting to Windows. While the create_process/waitpid pattern still works, there are other patterns that break. I need to manually translate Process Handles to Process IDs when I want to output the "pid" to stdout, a log file, etc. Also, I need to manually translate Process IDs to Process Handles when I read the "pid" from stdin, a file, etc.

Process IDs are portable across processes in a way that Process Handles are not. So while Process Handles work well enough as an opaque pseudo-pid, I think Process IDs would work just as well and would prevent unnecessary translation when outputting and inputting pids.

(For reference, here is the change I had to make to get some code working on Windows. Some of the code is responsible for logging pids to a file in case they need to be manually terminated later, and has to translate Process Handles to Process IDs. The code that reads these Process IDs needs to translate them back into Process Handles to terminate those processes. The third affected piece of code just reports to stdout the pid of a created process. facebook/flow@276bb7f)

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 18, 2020
@dra27 dra27 removed the Stale label Jun 23, 2020
@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Oct 18, 2021
@xavierleroy
Copy link
Contributor

As I wrote in 2007, The problem with using process ID instead of handles is:

if Unix.waitpid is called for a process that has already terminated, its processID is invalid (Windows doesn't keep the process around if its handles are closed) and there is no way to recover its handle and therefore its exit code.

In other words: the current implementation using handles is correct and the one using process IDs is not. So I'll close this issue. If anyone sees a workaround, please reopen.

@alainfrisch
Copy link
Contributor

A related remark only : I think it would seem coherent with the treatment of file descriptors in the Unix library to represent process descriptors with a dedicated abstract type which would be implemented as the integer representing the pid on Unix, and as the handle on Windows. We would expose a function to get the pid out of such value (using GetProcessId under Windows). And perhaps also a function to get the process descriptor from the pid (again the identity on Unix, and through OpenProcess on Windows).

(If we make such changes to Unix, and to keep backward compatibility, one would need to "fork" the waitpid and functions that create processes. This could be the occasion to also allow passing extra (sometimes OS-specific) flags when creating processes, as we do when opening files.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants