Opened 6 years ago

Closed 5 months ago

#2301 closed bug (fixed)

Proper handling of SIGINT/SIGQUIT

Reported by: duncan Owned by:
Priority: high Milestone: 7.8.1
Component: libraries/process Version: 6.12.3
Keywords: Cc: bos@…, tibbe, pho@…, v.dijk.bas@…, hvr
Operating System: POSIX Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets: #3994,#5766,#7229,#4274,#3318,#1619

Description

This guide http://www.cons.org/cracauer/sigint.html suggests a couple things that we should do that we do not currently do.

It comes in two parts.

  1. how we respond to our own calling process when we terminate due to ^C
  2. how we respond to process that we call terminating due to ^C

The first part is easier. If we decide that we want to terminate in response to a ^C signal then it is important that our calling process knows that. We must kill ourselves using SIGINT and not just terminate via exit() otherwise our calling process must assume that we decided to ignore the ^C. We should use killpid(getpid(),SIGINT).

Of course we may well want to catch ^C and do cleanup work by unwinding all exception handlers etc. It is therefore important to distinguish ^C from other exceptions so that when the ^C exception propagates to the top level exception handler we have enough information to know to use killpid(getpid(),SIGINT) rather than just exit(1). So we should add an Interrupted case to the Exception type.

How we respond to ^C while running sub-processes is more subtle. If we are delegating interaction with the user via the controlling terminal then we should also delegate the decision about how to respond to ^C. Some child processes will also want to take cleanup action on ^C or ignore it completely (eg ghci) so it is not right for us the parent process to catch ^C and decide what to do. So where we are delegating the decision we should ignore ^C.

When we wait for a child process to terminate and discover that it exited due to SIGINT then at that moment we should respond in the same way as if we ourselves had received a ^C. A sensible default might be to send the Interrupted exception to every thread in the system, or at least to the main thread. A haskell program can always change the response to ^C by using installHandler (eg for an interactive REPL program like ghci).

Attachments (3)

0001-Implement-delegated-control-C-handling-on-Unix.patch (24.6 KB) - added by duncan 5 months ago.
Implement delegated control-C handling on Unix
0002-Add-tests-for-the-delegated-control-C-handling.patch (4.2 KB) - added by duncan 5 months ago.
Add tests for the delegated control-C handling
0003-Rename-runGenProcess_-and-leave-a-deprecated-stub.patch (5.5 KB) - added by duncan 5 months ago.
Rename runGenProcess_ and leave a deprecated stub

Download all attachments as: .zip

Change History (46)

comment:1 Changed 6 years ago by igloo

  • Difficulty set to Unknown
  • Milestone set to 6.10.1

comment:2 follow-up: Changed 6 years ago by simonmar

  • Component changed from libraries/base to libraries/process

I've done the first part of this:

Wed Jul  9 09:49:16 BST 2008  Simon Marlow <marlowsd@gmail.com>
  * FIX part of #2301, and #1619
  
  2301: Control-C now causes the new exception (AsyncException
  UserInterrupt) to be raised in the main thread.  The signal handler
  is set up by GHC.TopHandler.runMainIO, and can be overriden in the
  usual way by installing a new signal handler.  The advantage is that
  now all programs will get a chance to clean up on ^C.
  
  When UserInterrupt is caught by the topmost handler, we now exit the
  program via kill(getpid(),SIGINT), which tells the parent process that
  we exited as a result of ^C, so the parent can take appropriate action
  (it might want to exit too, for example).
  
  One subtlety is that we have to use a weak reference to the ThreadId
  for the main thread, so that the signal handler doesn't prevent the
  main thread from being subject to deadlock detection.
  
  1619: we now ignore SIGPIPE by default.  Although POSIX says that a
  SIGPIPE should terminate the process by default, I wonder if this
  decision was made because many C applications failed to check the exit
  code from write().  In Haskell a failed write due to a closed pipe
  will generate an exception anyway, so the main difference is that we
  now get a useful error message instead of silent program termination.
  See #1619 for more discussion.

comment:3 follow-up: Changed 6 years ago by duncan

So the next part is how we handle ^C when we are running foreground processes. I think the behaviour suggested in the above description is right but lets try to flesh it out a bit more and suggest how it could be implemented.

Some terminology: technically all processes we launch are in the foreground. Putting a process into the background has to be done explicitly. However that does not means that logically we consider all processes to be in the foreground. We often launch processes and do not expect them to communicate with the user via the console or we may connect them via pipes and not let them use the console at all.

So we only want to delegate^C handling to some processes. Delegating ^C handling is a property of a process which we set when we launch it.

When we get a ^C there are two cases:

  • we are running one or more processes to which we consider we are delegating ^C handling
  • we are running no such processes

The second case is easy and is addressed by the patch above. We throw an exception to the main thread which will typically propagate and terminate the program.

In the first case we do not want to throw any exception immediately. However when processes to which we were delegating ^C handling terminate due to ^C we should at that point throw an exception to the main thread.

So how could we implement this?

Delegated ^C handling becomes another property of a process we launch. We keep a global count of the number of such processes we're running. We use that count to decide if we should throw an exception to the main thread when we receive SIGINT or not. We will also need to record in the map of launched processes if we were delegating ^C handling or not. This means changing from a Map PID (MVar ExitCode) to a Map PID (Bool, MVar ExitCode). When the SIGCHILD handler runs for a PID it will look it up in the map, if we were delegating C handling for that process and it terminated due to ^C then it should throw an exception to the main thread, exactly as the ^C handler would do when there were no delegated ^C processes running. It should also set the MVar ExitCode since the main thread may well be ignoring ^C exceptions and want to continue as normal.

So in both cases, ^C happens as an async exception in the main thread. The difference is whether it happens when the ^C is first received or when a process to which we are delegating ^C handling terminates due to ^C.

So the CreateProcess record will need an extra field to indicate if we're delegating ^C handling. That will make it possible to implement the old system and rawSystem functions in terms of createProcess.

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 Multiple to Unknown/Multiple

comment:6 Changed 5 years ago by igloo

  • Milestone changed from 6.10.1 to 6.12 branch

comment:7 Changed 5 years ago by igloo

  • Milestone changed from 6.12 branch to 6.12.1

comment:8 Changed 5 years ago by duncan

See #3318. There is clearly demand for running foreground processes with delegated Control-C handling.

comment:9 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by diego

Replying to simonmar:


1619: we now ignore SIGPIPE by default. Although POSIX says that a
SIGPIPE should terminate the process by default, I wonder if this
decision was made because many C applications failed to check the exit
code from write(). In Haskell a failed write due to a closed pipe
will generate an exception anyway, so the main difference is that we
now get a useful error message instead of silent program termination.
See #1619 for more discussion.

And when describing exec POSIX says:

Signals set to the default action (SIG_DFL) in the calling process image shall be set to the default action in the new process image.
Except for SIGCHLD, signals set to be ignored (SIG_IGN) by the calling process image shall be set to be ignored by the new process image.
Signals set to be caught by the calling process image shall be set to the default action in the new process image (see <signal.h>).

Thus, to avoid breaking applications that rely on the the default handler of SIGPIPE the RTS should restore the handler before starting new processes.

I can't say how many applications are affected by this nowadays. Simple applications may rely on default bahavior of signals. But it seems to be a minor issue.

comment:10 Changed 4 years ago by igloo

  • Milestone changed from 6.12.1 to 6.14.1
  • Type of failure set to None/Unknown

comment:11 Changed 4 years ago by bos

  • Cc bos@… added

comment:12 Changed 4 years ago by tibbe

  • Cc tibbe added

comment:13 Changed 4 years ago by PHO

  • Cc pho@… added

comment:14 in reply to: ↑ 9 Changed 4 years ago by phunge0

Replying to diego:

And when describing exec POSIX says:

Signals set to the default action (SIG_DFL) in the calling process image shall be set to the default action in the new process image.
Except for SIGCHLD, signals set to be ignored (SIG_IGN) by the calling process image shall be set to be ignored by the new process image.
Signals set to be caught by the calling process image shall be set to the default action in the new process image (see <signal.h>).

Thus, to avoid breaking applications that rely on the the default handler of SIGPIPE the RTS should restore the handler before starting new processes.

I can't say how many applications are affected by this nowadays. Simple applications may rely on default bahavior of signals. But it seems to be a minor issue.

I think this detail needs fixing too, here's a patch as an RFC: http://www.haskell.org/pipermail/glasgow-haskell-users/2010-August/019082.html

comment:15 Changed 4 years ago by igloo

  • Status changed from new to patch

comment:16 Changed 3 years ago by igloo

  • Milestone changed from 7.0.1 to 7.0.2

comment:17 Changed 3 years ago by igloo

  • Status changed from patch to new

Something equivalent to the patch was applied (#4274).

It might be useful if someone were to close this ticket and open another one, describing just what remains to be done.

comment:18 in reply to: ↑ 3 Changed 3 years ago by duncan

Note that due to this problem, Cabal is currently handling the call of ./configure incorrectly. Previously we used rawSystem which uses syncProcess to do the ctl-C handling. Now, because we need to pass some environment vars to ./configure we have to use the combination of runProcess + waitForProcess which means we are not getting the correct ctl-C handling.

The CreateProcess record should be extended with a Bool flag to indicate running in the foreground with delegated ctl-C handling as described above.

Note that an alternative to throwing Interrupted to the main thread is just to raise Interrupted as a synchronous exception, but that has the problem that by default forkIO (almost) silently discards exceptions rather than propagating them, so the Interrupted may never reach the main thread as intended.

comment:19 Changed 3 years ago by mcandre

  • Architecture changed from Unknown/Multiple to x86
  • Operating System changed from Unknown/Multiple to MacOS X
  • Type of failure changed from None/Unknown to Incorrect result at runtime
  • Version changed from 6.8.2 to 6.12.3

I can pass control to a handler for SIGINT, but exitFailure does nothing to end my program.

Code:

#!/usr/bin/env runhaskell

-- Press Control+C to quit.
--
-- Andrew Pennebaker
-- 7 Feb 2011
--
-- Based on Per Magnus Therning's "Signals in Haskell"
-- http://therning.org/magnus/archives/285

module SigIntTest where

import System.Posix.Signals
import Control.Monad (forever)
import System.Posix (sleep)
import System.Exit (exitFailure)

main :: IO ()
main = do
	mapM (\sig -> installHandler sig (Catch $ exitFailure) Nothing) [sigINT, sigHUP, sigABRT, sigTERM]

	forever $ do
		putStrLn "Repeating"
		sleep 1

Trace:

$ ./siginttest.hs 
Repeating
Repeating
Repeating
^Csiginttest.hs: ExitFailure 1
Repeating
Repeating
^Csiginttest.hs: ExitFailure 1
Repeating
Repeating
Repeating
...

Specs:

GHC 6.12.3

Mac OS X 10.6.6

MacBook Pro 5,1

comment:20 Changed 3 years ago by simonmar

  • Architecture changed from x86 to Unknown/Multiple
  • Operating System changed from MacOS X to Unknown/Multiple

@mcandre: this is the documented behaviour. From System.Exit.exitWith:

-- Note: in GHC, 'exitWith' should be called from the main program
-- thread in order to exit the process.  When called from another
-- thread, 'exitWith' will throw an 'ExitException' as normal, but the
-- exception will not cause the process itself to exit.

This ticket is about something different: the behaviour of SIGINT/SIGQUIT when there are child processes started by the Haskell program.

comment:21 follow-up: Changed 3 years ago by mcandre

@simonmar: Thanks for clearing that up. Could you supply the correct code to cause the main process to quit for SIGINT?

P.S. I didn't expect to use trac like Stack Overflow; I just thought this was a bug.

comment:22 in reply to: ↑ 21 Changed 3 years ago by simonmar

Replying to mcandre:

@simonmar: Thanks for clearing that up. Could you supply the correct code to cause the main process to quit for SIGINT?

It happens by default - if you don't install a handler, the main thread gets the UserInterrupt exception for SIGINT. If you want to do it with an explicit handler, you need to get the ThreadId for the main thread and use throwTo to send it an exception.

P.S. I didn't expect to use trac like Stack Overflow; I just thought this was a bug.

Yes - the mailing list would be a better place for this.

comment:23 Changed 3 years ago by mcandre

Incidentally, this works for me:

#!/usr/bin/env runhaskell

-- Press Control+C to quit.
--
-- Andrew Pennebaker
-- 7 Feb 2011
--
-- Based on Per Magnus Therning's "Signals in Haskell"
-- http://therning.org/magnus/archives/285
--
-- Based on Rosetta Code
-- http://rosettacode.org/wiki/Handle_a_signal#Haskell

-- Prevents Prelude shadowing other imports
{-# LANGUAGE NoImplicitPrelude #-}

module SigIntTest where

import Prelude hiding (catch)
import Control.Exception (catch, throwIO, AsyncException(UserInterrupt))
import Control.Monad (forever, when)
import System.Posix (sleep)
import System.Exit (exitFailure)

main :: IO ()
main = do
	catch (forever $ do
		putStrLn "Repeating"
		sleep 1)
		(\e -> when (e == UserInterrupt)
			exitFailure)

comment:24 Changed 3 years ago by igloo

  • Milestone changed from 7.0.2 to 7.2.1

comment:25 Changed 3 years ago by Favonia

Ticket #3994 seems to be related to this one.

comment:26 Changed 3 years ago by igloo

  • Milestone changed from 7.2.1 to 7.4.1

comment:27 Changed 2 years ago by simonmar

See also #5766

comment:28 Changed 2 years ago by basvandijk

  • Cc v.dijk.bas@… added

comment:29 Changed 2 years ago by igloo

  • Milestone changed from 7.4.1 to 7.6.1

comment:30 Changed 20 months ago by benmachine

It's worth mentioning that due to #7229 it's pretty hard for clients to implement the right behaviour for subprocesses themselves.

comment:31 Changed 19 months ago by igloo

  • Milestone changed from 7.6.1 to 7.6.2

comment:32 Changed 15 months ago by morabbin

comment:33 Changed 5 months ago by hvr

  • Cc hvr added
  • Milestone changed from 7.6.2 to 7.8.1
  • Operating System changed from Unknown/Multiple to POSIX
  • Priority changed from normal to high

comment:34 Changed 5 months ago by duncan

I have a patch to fix this.

It adds a new delegate_ctlc :: Bool to the CreateProcess options record for delegating control C handling. If you turn that on, then createProcess will make the parent process ignore SIGINT until waitForProcess returns (or getProcessExitCode returns non-Nothing).

Then waitForProcess (or getProcessExitCode) will throw UserInterrupt if we were delegating, and the process did exit due to SIGINT. This should implement the "WCE" protocol ok.

It should correctly handle the case of there being multiple concurrent processes to which we're delegating (unlike the current system/rawSystem which will end up with the wrong signal handlers installed).

comment:35 Changed 5 months ago by simonmar

If waitForProcess is called by a forkIO'd thread, then UserInterrupt doesn't cause the process to die with SIGINT. Normally a SIGINT causes the main thread to receive UserInterrupt, and the top handler for the main thread turns this into a SIGINT again.

What should happen? Maybe this: if a UserInterrupt in a child thread were to throw a SIGINT at the current process, then it would be caught by the handler, causing a UserInterrupt to be thrown to the main thread, which would eventually throw a SIGINT again to kill the process.

comment:36 Changed 5 months ago by duncan

Yes, I did think about whether it should throw the UserInterrupt sync locally or async to the main thread. As you say, the concern is that people do not properly propagate their exceptions from forkIO threads (which I still maintain is a deficiency in our thread system). On the other hand it gives people an opportunity to catch the user interrupt locally if that's what they want to do.

If it were not for the annoying problem of forkIO swallowing our exceptions, then I'd say that throwing it locally is really the right thing to do, because by delegating ^C handling to the child process, we really are turning an async exception (being interrupted by a signal) into a sync exception (waiting on process exit, which exits due to a signal).

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

Changed 5 months ago by duncan

Implement delegated control-C handling on Unix

Changed 5 months ago by duncan

Add tests for the delegated control-C handling

comment:37 Changed 5 months ago by duncan

  • Status changed from new to patch

Changed 5 months ago by duncan

Rename runGenProcess_ and leave a deprecated stub

comment:38 Changed 5 months ago by duncan

Simon: BTW, I didn't mean I'm dead set against changing it, I think it's rather unclear which is better.

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

In a0467f3ee892f5843c1ba98bcb9630e1e5b4863b/process:

Implement delegated control-C handling on Unix (#2301)

This is a generalisation of the SIGINT-ignoring that system and
rawSystem do, to allow it to be used via the general createProcess.

For the gory details of SIGINT handling, see
http://www.cons.org/cracauer/sigint.html

We implement the 'WCE' method described there.

That important feature was only available to system and rawSystem
(mirroring the C system() behaviour). These functions are very limited
and indeed deprecated, so we need this feature in general. In particular
projects like Cabal are suffering because they cannot do this properly
(or need horrible workarounds copy and pasting much of System.Process
and using System.Process.Internals).

The feature is available now via a new delegate_ctlc flag in the
CreateProcess options record. The use of signal handlers is still a
little hairy, but probably better than before (for situations where
there were multiple concurrent calls to system/rawSystem).

One thing to note is that waitForProcess and getProcessExitCode can now
throw the UserInterrupt exception.

This is all documented in the haddock docs (both a short description and
also the excruciating details).

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

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

In 1b1f18b89d23094f4411be534c493a0da8aeadb9/process:

Add tests for the delegated control-C handling (#2301)

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

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

In 3d8f9bb7f84c1fbb5efa62dd46daae44110ddb18/process:

Rename runGenProcess_ and leave a deprecated stub

At least Cabal was using runGenProcess_, and the previous patches
addressing #2301 changed its type already. So this adds a deprecated
stub with the original type and the real function gets to have a less
odd name.

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

comment:42 Changed 5 months ago by simonmar

I think you're absolutely right to throw UserInterrupt locally. The only question is what to do if it propagates to the top of a forkIO thread. I think it would be good to note in the docs that UserInterrupt only causes the process to die if it propagates to the top of the main thread, so if this is a child thread you need to arrange to propagate it to the parent, say by using async instead of raw forkIO.

comment:43 Changed 5 months ago by hvr

  • Resolution set to fixed
  • Status changed from patch to closed
Note: See TracTickets for help on using tickets.