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

Open
vicuna opened this issue May 29, 2006 · 15 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

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

This comment has been minimized.

Copy link
Author

commented May 29, 2006

Comment author: Christoph Bauer

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

@vicuna

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

commented Jun 10, 2006

Comment author: @xavierleroy

I keep that as a feature wish.

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 3, 2006

Comment author: Christoph Bauer

I attached an patch that seems to work:

o Unix.getpid() returns a pid like the taskmanager
o create_process + winwait works correct (at least in my test)
o 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

This comment has been minimized.

Copy link
Author

commented Aug 3, 2006

Comment author: Christoph Bauer

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

@vicuna

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

commented Mar 16, 2007

Comment author: Christoph Bauer

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 22, 2008

Comment author: Christoph Bauer

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 16, 2011

Comment author: Christoph Bauer

patch against 3.12.1. compiles with msvc.

@vicuna

This comment has been minimized.

Copy link
Author

commented May 20, 2014

Comment author: Christoph Bauer

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

@vicuna

This comment has been minimized.

Copy link
Author

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)

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.