Opened 10 years ago

Closed 9 years ago

#3994 closed feature request (fixed)

Add support for creating and interrupting process groups

Reported by: hamish Owned by: simonmar
Priority: high Milestone: 7.2.1
Component: libraries/process Version: 6.12.1
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

This patch introduces the following to System.Process

new_group

This Bool field is added to CreateProcess. If it is set to True the child process will be created as the lead process in a new process group.

  • Unix - calls setpgid in both the parent and the child process after fork.
  • Win32 - calls CreateProcess with the CREATE_NEW_PROCESS_GROUP set. I also had it unset CREATE_NO_WINDOW because this seems to prevent the child attaching to the parents console (and therefor stops interuptProcessGroup from working).

interruptProcessGroup

This function can be used interrupt a running process group.

  • Unix - calls signalProcessGroup to send sigINT
  • Win32 - If the process ID is known it calls generateConsoleCtrlEvent to send cTRL_BREAK_EVENT

Compatibility

Backward Compatibility

CreateProcess has a new field new_group which may need to be added to some existing code. If set to False the current behaviour is preserved. As far as I know this is the only change that could break existing code.

Linker Errors

I have renamed the C functions it used in order to prevent linker errors when using this new version with the existing process package.

Win32 Only

On Win32 the process handle is not the same as the process ID and there is no reliable way to convert from one to the other. I have added an interface that allows access to the process ID, but does not change the behaviour of the existing functions.

PINFO

Replaces PHANDLE in the ProcessHandle type.

type PINFO = (PHANDLE, Maybe Word32) -- Handle and Process ID

mkProcessHandleWithPID

Like mkProcessHandle but allows you to specify the processes ID as well. This function is now used in the run* functions and createProcess, so the ProcessHandle they return will contain a process ID.

withProcessInfo and withProcessInfo_

Like withProcessHandle functions but gives you access to the PINFO (not just the PHANDLE it contains).

Attachments (1)

ProcessGroups2.patch (20.1 KB) - added by hamish 10 years ago.
Second revision of the patch.

Download all attachments as: .zip

Change History (20)

Changed 10 years ago by hamish

Attachment: ProcessGroups2.patch added

Second revision of the patch.

comment:2 Changed 10 years ago by igloo

If I understand correctly, we will have

terminateProcess :: ProcessHandle -> IO ()

which sends SIGTERM, and

interruptProcessGroup :: ProcessHandle -> IO ()

which sends SIGINT.

Shouldn't we have

signalProcess      :: CInt -> ProcessHandle -> IO ()
signalProcessGroup :: CInt -> ProcessHandle -> IO ()

(with terminateProcess for backwards compatibility)?

comment:3 Changed 10 years ago by hamish

Perhaps signalProcess and signalProcessGroup should be added to System.Process.Internals for unix systems, but sadly windows can only send a SIGINT like event and only to a process group, so they would not be portable.

I think terminateProcess and interruptProcessGroup make it clear to the user what is portable (terminating any process and interrupting a process group).

comment:4 Changed 10 years ago by igloo

Milestone: Not GHC

comment:5 Changed 10 years ago by hamish

Component: Compilerlibraries/process

I would like to move this forward if possible.

The patch I submitted here six weeks ago includes the changes Simon requested on haskell-cafe. So that just leaves the question of wether we should add signalProcess and signalProcessGroup for unix developers.

I don't think adding unix only signalProcess and signalProcessGroup functions to this package is a good idea. I think developers expect the functions in this package to be portable.

If we add them then there is a danger developers on unix systems will use these non portable functions instead of the portable functions (inadvertently breaking compilation on win32). If they need to send different signal types they can using the withProcessHandle_ and the unix package signal functions (which makes it clear that they are no longer writing portable code).

comment:6 Changed 10 years ago by simonmar

Milestone: Not GHC6.14.1
Priority: normalhigh

I need to properly review the patch. The API change looks fine to me, but the internal changes weren't obviously right when I just skimmed though the patch. Also there are some typos in the Haddocks. I'll make sure we get it done before 6.14.1 though.

comment:7 Changed 10 years ago by igloo

Owner: set to simonmar

comment:8 Changed 10 years ago by igloo

Status: newpatch

comment:9 Changed 9 years ago by igloo

Milestone: 7.0.17.0.2

comment:10 Changed 9 years ago by igloo

Milestone: 7.0.27.0.3
Type: proposalfeature request

comment:11 Changed 9 years ago by Favonia

Recently I have implemented similar functionality for my own project. While I am more than glad to see people discussing about this, I have some observations/ideas different from the current patch.

  • I suggest another name interruptProcessGroupOf to clearly distinguish process groups from processes. Actually, the proposed patch might fail if the child changed its process group. A more reliable solution for POSIX might look up the current process group first. (See the following points for the problem of my approach with the current library.) There should be some ways to find out the root process in Windows? (Sorry that I do not program in Windows.) If this is indeed possible, then we can interrupt a group even if it is not created with the new flag.
  • Also I prefer create_group over new_group for consistency with (createProcessGroup). This is just a personal opinion, though.
  • (A little off-topic:) For checking the process group in POSIX, I recommend re-implementing System.Posix.Process.getProcessGroup by the more standardized getpgid instead of getpgrp. The standardized version make it possible to query the process group of a process, and is more consistent with setProcessGroup. (This is not entirely the library's fault. BSD/System V/POSIX have different sets of APIs. In my opinion we should have a full set of POSIX API:
    getProcessGroupIDOf :: ProcessID -> IO ProcessGroupID
    setProcessGroupIDOf :: ProcessID -> ProcessGroupID -> IO ()
    joinProcessGroupID :: ProcessGroupID -> IO () -- a special case
    recruitProcessID :: ProcessID -> IO () -- another special case
    getCurrentProcessGroupID :: IO ProcessGroupID -- maybe a shorter name?
    setCurrentProcessGroupID :: ProcessID -> IO ()
    
  • Perhaps we need to take care of the situation that we are interrupting ourself. Maybe we should temporarily block the signals while playing around processes, at least on POSIX.
  • I am not sure if adding the suffix 2 is a good idea. Certainly in some cases this is unavoidable, but ideally we should only rely on the version number. Personally I feel the high-level process handling library is far from complete, and we might need suffixes 3,4,... in the future.
  • I can see the patch is using something like Data.Bits to handle close_fds and the new flag. I think this is a good move, as it seems efficient, ready for more flags, and unrelated to the interface. (Please correct me if I am wrong.) However, defining the constants in the header might make the program more clearer. Especially this part:
    ((if mb_close_fds then 1 else 0).|.(if mb_new_group then 2 else 0))
    
  • Also I am not convinced that we should use ProcessInfo to hold process ID. MSDN says the process ID can always be retrieved by using system call GetProcessId. The old handler should work. I am trying to minimize the changes we need to implement the functionality.
  • Another somewhat-related interesting idea is to implement suspension. This is more complicated, though. I think a more useful usage is to have suspendProcessGroupOf and continueProcessGroupOf. This should be trivial for POSIX systems by using SIGTSTP and SIGCONT. As for Windows, from MSDN documents I have learned that we can find out all attached processes in a console progress group, and then suspend threads in the processes.

comment:12 Changed 9 years ago by hamish

interruptProcessGroupOf and create_group sound good. I'm not to fussed about what goes on in System.Posix.Process (as you said it is kind of off topic for this change).

Are there any situations you would want to interrupt your own process group without blocking signals?

I am happy to change or remove the suffix. How is this problem normally handled? If a user of say ghc 6.12.3 upgrades process are they expected to upgrade everything that uses the old process package?

The patch already has this in the header runProcess.h ...

#define RUN_PROCESS_IN_CLOSE_FDS 0x1
#define RUN_PROCESS_IN_NEW_GROUP 0x2

We should move these to a simpler header (processFlags.h or something) so it can be included in both runProcess.h and Internals.hs.

I did try with getProcessID first, but I could not make it work. MSDN says

The handle must have the PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION access right.

but then it also says

The handle returned by the CreateProcess function has PROCESS_ALL_ACCESS access to the process object.

Suspension would be cool, but should we make that another issue? I am not convinced it will be easy to do this on Win32 (I would be happy to proved other wrong with some sample code though).

comment:13 in reply to:  12 Changed 9 years ago by Favonia

Replying to hamish:

interruptProcessGroupOf and create_group sound good. I'm not to fussed about what goes on in System.Posix.Process (as you said it is kind of off topic for this change).

Well, if you want to know the process group id of a process, then you need getpgid! Let me rephrase my point again: I think childGrp <- getpgid(child) and than killpg(childGrp) is better than killpg(child) directly.

Are there any situations you would want to interrupt your own process group without blocking signals?

I was worried about SIGINT, FFI and user-defined signal handlers, but this post says I worried too much. :)

I am happy to change or remove the suffix. How is this problem normally handled? If a user of say ghc 6.12.3 upgrades process are they expected to upgrade everything that uses the old process package?

As far as I understand multiple versions can co-exist in Hackage system. However we need a significant version bump. Some suggestions are made in PVP.

The patch already has this in the header runProcess.h ...

#define RUN_PROCESS_IN_CLOSE_FDS 0x1
#define RUN_PROCESS_IN_NEW_GROUP 0x2

We should move these to a simpler header (processFlags.h or something) so it can be included in both runProcess.h and Internals.hs.

Yes, that is what I meant. Even without an extra file, I think it would be nice to have named constants in Internals.hs.

I did try with getProcessID first, but I could not make it work. MSDN says

The handle must have the PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION access right.

but then it also says

The handle returned by the CreateProcess function has PROCESS_ALL_ACCESS access to the process object.

This sounds like a bug to me. Doesn't PROCESS_ALL_ACCESS imply all other rights? Maybe some Windows expert can save us.

Suspension would be cool, but should we make that another issue? I am not convinced it will be easy to do this on Win32 (I would be happy to proved other wrong with some sample code though).

After knowing the issues about CreateProcess I lost all my confidence. Yes let's do it in another thread, or maybe in a Wiki page titled System.Process Reformation.

comment:14 Changed 9 years ago by Favonia

FYI: A related ticket #2301 contains some ideas about delegating Ctrl-C to subprocesses.

comment:15 Changed 9 years ago by hamish

I tried GetProcessId again and it works. Not sure what happened when I tried it before, but I think I might have messed up the calling convention by not defining _WINNT_VER before including windows.h (I noticed this time when I was testing GetProcessId in runProcess.c that I got a warning that it was GetProcessId's sig was not defined if I did not define _WINNT_VER).

Anyway I stuck getProcessId into Win32 package where it belongs and it works fine.

I have updated the patch with this and your other suggestions and the new version is here https://github.com/hamishmack/packages-process/tree/Ticket-3994-GetProcessId and https://github.com/hamishmack/packages-Win32/tree/Ticket-3994-GetProcessId

I also added a test case that makes sure "sleep 10" can be interrupted.

The flags are in a separate header so they can easily be #included in both the .hs and the .c (runProcess.h did not work when #included in the .hs)

comment:16 in reply to:  15 Changed 9 years ago by Favonia

I quickly reviewed the code. For the new function I might prefer

            OpenHandle h -> do
                gh <- getProcessGroupIDOf h
                signalProcessGroup sigINT gh
                return p_

instead of

            OpenHandle h -> do
                signalProcessGroup sigINT h
                return p_

if possible. The difference, as pointed out in my previous comments, is that this will work even if create_group=False or the child has changed its group. The technical problem is that getProcessGroupIDOf (wrapper of getpgid) does not exist in Posix package now! I will try to write a wrapper... hopefully things will be integrated into System.Posix.Process.

PS: Thanks for your hard work. I wish I knew this ticket so that I didn't have to implement this by myself. :)

comment:17 in reply to:  15 ; Changed 9 years ago by simonmar

Replying to hamish:

I tried GetProcessId again and it works. Not sure what happened when I tried it before, but I think I might have messed up the calling convention by not defining _WINNT_VER before including windows.h (I noticed this time when I was testing GetProcessId in runProcess.c that I got a warning that it was GetProcessId's sig was not defined if I did not define _WINNT_VER).

Anyway I stuck getProcessId into Win32 package where it belongs and it works fine.

I have updated the patch with this and your other suggestions and the new version is here https://github.com/hamishmack/packages-process/tree/Ticket-3994-GetProcessId and https://github.com/hamishmack/packages-Win32/tree/Ticket-3994-GetProcessId

I also added a test case that makes sure "sleep 10" can be interrupted.

The flags are in a separate header so they can easily be #included in both the .hs and the .c (runProcess.h did not work when #included in the .hs)

I took a quick look, and the changes look fine - much easier to review this version without the changes to ProcessHandle. When you've reached a fixed point with the patch I'll push it in.

comment:18 in reply to:  17 Changed 9 years ago by Favonia

Replying to simonmar:

I took a quick look, and the changes look fine - much easier to review this version without the changes to ProcessHandle. When you've reached a fixed point with the patch I'll push it in.

I believe we have reached a fixed point. My last comment does not make sense unless #5167 is done.

comment:19 Changed 9 years ago by simonmar

Resolution: fixed
Status: patchclosed

Committed, thanks!

Note: See TracTickets for help on using tickets.