Opened 6 years ago

Closed 5 months ago

Last modified 5 months ago

#2233 closed task (fixed)

Overhaul System.Process

Reported by: simonmar Owned by: simonmar
Priority: normal Milestone:
Component: libraries/process Version: 6.8.2
Keywords: Cc: Deewiant, anton.nik@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

Overhaul of System.Process

From the patch:

Tue Apr 22 15:02:16 PDT 2008  Simon Marlow <simonmarhaskell@gmail.com>
  * Overhall System.Process
     
   - fix #1780: pipes created by runInteractiveProcess are set
     close-on-exec by default
   
   - add a new, more general, form of process creation: createProcess
     Each of stdin, stdout and stderr may individually be taken
     from existing Handles or attached to new pipes.  Also it
     has a nicer API.
   
   - add readProcess from Don Stewart's newpopen package.  This
     function behaves like C's popen().
  
   - Move System.Cmd.{system,rawSystem} into System.Process.  Later
     we can depecate System.Cmd.
   
   - Don't use O_NONBLOCK for pipes, as it can confuse the process
     attached to the pipe (requires a fix to GHC.Handle in the base
     package).
  
   - move the tests from the GHC testsuite into the package itself,
     and add a couple more
  
   - bump the version to 2.0

Haddock for the new API is here: http://darcs.haskell.org/~simonmar/process/System-Process.html

Patch is attached. So far I've only modified the Unix parts of the code, the Windows parts still need to be updated.

Discussion period: 4 weeks (20 May)

Attachments (8)

process-2.0.patch (38.5 KB) - added by simonmar 6 years ago.
darcs patch to implement the proposed changes (Unix only so far)
0001-API-cleanup-with-new-functions-and-old-soft-deprecat.patch (4.5 KB) - added by duncan 5 months ago.
API cleanup with new functions and old "soft" deprecated
0002-Reorder-code-sections-to-be-rather-clearer.patch (22.1 KB) - added by duncan 5 months ago.
Reorder code sections to be rather clearer
0003-Add-TODO-notes-on-what-should-be-marked-DEPRECATED-l.patch (3.4 KB) - added by duncan 5 months ago.
Add TODO notes on what should be marked DEPRECATED later
0004-Document-spawnProcess-and-spawnCommand.patch (1.2 KB) - added by duncan 5 months ago.
Document spawnProcess and spawnCommand
0001-All-new-sync-process-functions-now-terminate-on-an-e.patch (7.8 KB) - added by duncan 5 months ago.
All new sync process functions now terminate on an exception
0002-Improve-the-code-for-ignoring-EPIPE.patch (2.4 KB) - added by duncan 5 months ago.
Improve the code for ignoring EPIPE
0003-Be-even-more-careful-with-the-threads-that-consume-o.patch (4.5 KB) - added by duncan 5 months ago.
Be even more careful with the threads that consume output

Download all attachments as: .zip

Change History (38)

Changed 6 years ago by simonmar

darcs patch to implement the proposed changes (Unix only so far)

comment:1 Changed 6 years ago by duncan

Further to the changes already made we discussed some ideas on #ghc today:

Deprecate:

  • runCommand
  • runProcess
  • runInteractiveCommand
  • runInteractiveCommand

The rationale is that these are all more limited than the new createProcess and yet none of them are really convenient. It's better to have fewer variations if they can all be expressed easily using createProcess. It makes the API much simpler.

Also we'd not add system or rawSystem to System.Process.

That would leave just:

  • createProcess
  • readProcess
  • readProcessWithExitCode

Then add the following:

  • callProcess :: FilePath -> [String] -> IO ()
  • callCommand :: String -> IO ()


These would be synchronous like the current system and rawSystem. The difference is they would throw IOErrors on failure rather than returning the ExitCode which is so easily ignored.

These are of course only convenience functions. If someone wants the exit code it should be easy to do it via createProcess and waitForProcess. We need to make sure that is indeed the case.

We'd also add async versions of the above:

  • spawnProcess :: FilePath -> [String] -> IO ProcessHandle
  • spawnCommand :: String -> IO ProcessHandle

that do not wait for the process to finish and return the ProcessHandle?. Again these should be easy instances of createProcess. The docs should probably say as much so it's clear how to make minor variations.

We also discussed how it should be safe to GC the ProcessHandle and not end up with zombie processes on unix systems. On Windows it's actually easy because you can close the process handle which means you don't get the exit status of the process. On unix we have to collect the exit status of every child process (actually you can ignore all of them, but you cannot ignore them selectively).

The point is that with a convenient spawnProcess it's tempting to ignore the ProcessHandle result and never bother calling waitForProcess on it. We do want to support that. At the moment doing that would leave zombie processes.

We discussed a mechanism to allow GC'ing ProcessHandles that does not leave zombies. It'd probably involve keeping a Map PID (MVar ExitCode) and embedding a MVar ExitCode in the ProcessHandle. Then when we get notified that a child process terminated we would store the exit status in the MVar. Then waitForProcess would just wait on that MVar ExitCode.

The one thing we have left that we cannot express with createProcess is the behavior with respect to ^C handling. For some processes we want to delegate ^C handling to that child process (eg imagine calling ghci). For others we want to handle ^C 'normally'. For details see #2301.

comment:2 Changed 6 years ago by Deewiant

  • Cc Deewiant added

comment:3 Changed 6 years ago by simonmar

Another round of changes has been posted for comment. Haddock docs in the same location: http://darcs.haskell.org/~simonmar/process/System-Process.html, and the patch message is as follows:

  * More System.Process overhaul
  
  New functions:
  
    callProcess :: FilePath -> [String] -> IO ()
    callCommand :: String -> IO ()
  
    spawnProcess :: FilePath -> [String] -> IO ProcessHandle
    spawnCommand :: String -> IO ProcessHandle
  
  Changes:
  
    - system and rawSystem have been removed from System.Process again.
      (they were only there temporarily after the last round of changes,
      now callCommand and callProcess replace them respectively).
  
  On Unix systems we now use SIGCHLD to detect process completion
  instead of calling waitpid().  This has several advantages:
  
    - much cheaper: no extra OS threads to do the waiting
    - doesn't require -threaded to get non-blocking waitForProcess
    - waitForProcess can be interrupted
    - no zombie process left around (only relevant on Unix)
  
  However, it relies on the new signal API (see separate proposal).  And
  these advantages aren't available on Windows (yet).

comment:4 Changed 6 years ago by simonmar

  • Architecture changed from Unknown to Unknown/Multiple

comment:5 Changed 6 years ago by simonmar

  • Operating System changed from Unknown to Unknown/Multiple

comment:6 Changed 4 years ago by igloo

  • Owner set to simonmar
  • Type of failure set to None/Unknown

The original patch has been applied, and the second "round of changes" is 2 years old. Simon, can I suggest closing this ticket and, if appropriate, making a new proposal for the other changes?

comment:7 Changed 4 years ago by simonmar

We might as well keep this open, rather than start a new ticket, I think.

comment:8 Changed 4 years ago by lelf

  • Cc anton.nik@… added

comment:9 Changed 3 years ago by igloo

  • Type changed from proposal to task

comment:10 Changed 3 years ago by igloo

  • Milestone changed from Not GHC to 7.4.1

comment:11 Changed 2 years ago by simonmar

  • Milestone changed from 7.4.1 to _|_

comment:12 Changed 20 months ago by benmachine

Now that it's been another 2 years, the haddock link above is dead. Does anyone know what "the new signal API" referred to? Is it worth keeping this bug open?

comment:13 Changed 20 months ago by simonmar

The new signal API is #2451. That ran into problems IIRC.

We probably still want this API change, but probably not the signal changes, so I'll keep the ticket open.

Changed 5 months ago by duncan

API cleanup with new functions and old "soft" deprecated

Changed 5 months ago by duncan

Reorder code sections to be rather clearer

Changed 5 months ago by duncan

Add TODO notes on what should be marked DEPRECATED later

Changed 5 months ago by duncan

Document spawnProcess and spawnCommand

comment:14 Changed 5 months ago by duncan

  • Status changed from new to patch

comment:15 Changed 5 months ago by duncan

Note, these patches depend on the ones from #2301

comment:16 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In d53196e313023ce92a5181074b7bb2ab7d35b5f3/process:

API cleanup with new functions and old "soft" deprecated

Add callProcess, callCommand, spawnProcess, spawnCommand as per the
design in #2233 (but not relying on any of the SIGCHLD stuff).

Move the various pre-createProcess functions to a section at the bottom
in the Haddock docs. Don't yet mark anything as deprecated. That can
come later.

Authored-by: Duncan Coutts <duncan@well-typed.com>
Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:17 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In a2c3294223bf1f5a1887b4f1bb6e1534699f4eba/process:

Reorder code sections to be rather clearer

And move the deprecated things (as per #2233) to the end, out of the way.

Authored-by: Duncan Coutts <duncan@well-typed.com>
Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:18 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In 0faf513f7e3667246d40767d25d0871b49d95851/process:

Document spawnProcess and spawnCommand (#2233)

Authored-by: Duncan Coutts <duncan@well-typed.com>
Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:19 Changed 5 months ago by simonmar

Looks great, thanks for doing this!

Changed 5 months ago by duncan

All new sync process functions now terminate on an exception

Changed 5 months ago by duncan

Improve the code for ignoring EPIPE

Changed 5 months ago by duncan

Be even more careful with the threads that consume output

comment:20 Changed 5 months ago by duncan

A few follow-up patches. The last one needs rather careful review. It's all rather subtle.

comment:21 follow-up: Changed 5 months ago by simonmar

Heh, clearly we need to pull the async package into the core libraries. But the patches look ok to me.

comment:22 in reply to: ↑ 21 Changed 5 months ago by hvr

Replying to simonmar:

Heh, clearly we need to pull the async package into the core libraries. But the patches look ok to me.

...that would pull in stm as well, wouldn't it? :-)

Last edited 5 months ago by hvr (previous) (diff)

comment:23 Changed 5 months ago by duncan

hvr noticed that a test was failing. I noticed that the ignoreSigPipe should be cover the hFlush as well (my fault).

comment:24 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In b601209456e13ca3d08faffb7e6771bcb8c1b1b4/process:

All new sync process functions now terminate on an exception (#2233)

Now all the functions that call a process synchronously have the same
behaviour. Previously just readProcess, readProcessWithExitCode did
this, now callProcess and callCommand do too.

If a thread running one of these functions gets an exception, including
async exceptions (such as from timeout or killThread), then the
external process gets terminated.

Introduce a helper function to implement this behaviour. Currently it
is not exposed to users, but that could be changed easily.

Authored-by: Duncan Coutts <duncan@well-typed.com>
Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:25 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In 79ae975aab5866de38e1a01aca7ac5bd9cde2285/process:

Improve the code for ignoring EPIPE (#2233)

Factor it out into an ignoreSigPipe util, and use it in both
readProcess and readProcessWithExitCode.

Authored-by: Duncan Coutts <duncan@well-typed.com>
Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:26 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In d95a67012ea30b02144d193254d947cdf5400a9d/process:

Be even more careful with the threads that consume output (#2233)

There is a potential deadlock with withCreateProcess in the case that
there's an exception: cleanupProcess will try to hClose the various
handles, but if another thread holds the Handle lock then that hClose
will block.

Takano Akio fixed the main case of this (in patch
32223a9ab174c22e939c81e24b6f48223c7cb020) by terminating the process
(before closing the handles) This works because terminating the process
will eventually cause those other threads to finish and release the
Handle lock, so we can hClose.

However on Unix terminateProcess is not guaranteed to terminate the
process since it uses SIGTERM, which can be handled or ignored. So we
have to separately guarantee that the handles can be hClosed, and the
simplest way to do this is to ensure that the thread reading from the
handles get killed in the case there's an exception.

So we change forkWait to withForkWait that will kill off the thread if
the body gets an exception.

Authored-by: Duncan Coutts <duncan@well-typed.com>
Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:27 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In 228297ec53e9ee7a1a6a3c5964ca7e89a6474c9b/process:

Drop redundant hFlush & add ignoreSigPipe to hClose

This is a follow-up to 79ae975aab5866de38e1a01aca7ac5bd9cde2285
(addressing #2233)

Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:28 Changed 5 months ago by hvr

  • Resolution set to fixed
  • Status changed from patch to closed

comment:29 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In 776a260be009cc27cfeacf298934d544c4e87408/process:

Make `cleanupProcess` resistant to SIGPIPE

Otherwise, `cleanupProcess` may exit prematurely if trying to flush out
data to the process' stdin handle even though the endpoint has already
vanished, and fail to complete the cleanup process.

See also 228297ec53e9ee7a1a6a3c5964ca7e89a6474c9b and #2233.

Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

comment:30 Changed 5 months ago by Herbert Valerio Riedel <hvr@…>

In edda1be660d07b461da75b2dfacc3d5820b4d084/process:

Add Haddock note to `call{Command,Process}` wrt execptions (#2233)

This also tweaks Haddock comments & markup in "System.Process" while at it.

Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>
Note: See TracTickets for help on using tickets.