Opened 7 years ago

Last modified 12 months ago

#4162 patch bug

GHC API messes up signal handlers

Reported by: jcpetruzza Owned by:
Priority: low Milestone:
Component: GHC API Version: 6.12.3
Keywords: Cc: chowells79@…, hsyl20
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): phab:D2633
Wiki Page:

Description

A side-effect of using the runGhc function is that some signal handlers are modified and not restored afterwards (see function initGhcMonad). In particular, the handler for SIGINT installed by ghc throws an exception to a thread stored in a global variable, which initially corresponds to the thread from which runGhc was run.

This is a particularly problematic for programs that wish to run ghc "in the background" on its own thread. For example, consider this code:

import qualified GHC
import qualified MonadUtils as GHC
import qualified GHC.Paths as GHC

import Control.Concurrent ( forkIO, threadDelay )

main = do
   putStrLn "waiting for 5 seconds..."
   threadDelay $ 5 * 1000 * 1000
   putStrLn "starting..."
   forkIO $ GHC.runGhc (Just GHC.libdir) (GHC.liftIO $ putStrLn "hello")
   putStrLn "waiting for 10 seconds"
   threadDelay $ 10 * 1000 * 1000
   putStrLn "exiting after final wait"

One can interrupt this program with Ctrl-C during the first five seconds of execution only.

It is not clear to me how one can safely workaround this problem. For instance, one could manually restore the program's original handlers at the beginning of execution, that is, transform:

runGhc action

into something like this:

runGhc $ (liftIO restoreProgramHandlers >> action)

but I don't know if this is safe (i.e., what happens if ghc is run without its own handlers installed).

Change History (18)

comment:1 Changed 7 years ago by carlhowells

If anyone with knowledge of the GHC api could chime in, I'd greatly appreciate it. I'm waiting on an update to the hint package, which is waiting on a response to this.

This comes down to two things, fundamentally:

1) It's a bug that the GHC api changes signal handlers, and doesn't change them back when it's finished. The GHC api probably shouldn't even touch signal handlers. The handler it's installing is part of GHCi, not the GHC api. Hence I believe GHCi code should be installing it, not the GHC api.

2) Given the current behavior, is it a safe workaround to restore the previous signal handlers inside the action runGhc is performing, or should restoring them be delayed until runGhc returns?

An answer to the latter is what's required for forward progress on hint. The former is less urgent, but should be considered a bug to some degree in the GHC api.

comment:2 Changed 7 years ago by carlhowells

Cc: chowells79@… added

comment:3 in reply to:  1 Changed 7 years ago by simonmar

Milestone: 6.14.1

Replying to carlhowells:

1) It's a bug that the GHC api changes signal handlers, and doesn't change them back when it's finished. The GHC api probably shouldn't even touch signal handlers. The handler it's installing is part of GHCi, not the GHC api. Hence I believe GHCi code should be installing it, not the GHC api.

I sympathise with that view. However, it's not entirely straightforward to change, because when GHC executes an expression interactively, it starts a new thread, so Ctrl-C exceptions have to be directed to the correct thread. There would probably need to be some callback mechanism from the GHC API back to the client to inform the client that interrupts need to be directed to a new thread.

I agree we should do this, so let's leave the ticket open.

2) Given the current behavior, is it a safe workaround to restore the previous signal handlers inside the action runGhc is performing, or should restoring them be delayed until runGhc returns?

restoring the signal handler should be fine. Note we also catch SIGHUP, SIGTERM, and SIGQUIT, to ensure that GHC cleans up temporary files if any of these are caught.

An answer to the latter is what's required for forward progress on hint. The former is less urgent, but should be considered a bug to some degree in the GHC api.

comment:4 Changed 7 years ago by igloo

Milestone: 7.0.17.0.2

comment:5 Changed 7 years ago by igloo

Milestone: 7.0.27.2.1

comment:6 Changed 6 years ago by igloo

Milestone: 7.2.17.4.1

comment:7 Changed 6 years ago by igloo

Milestone: 7.4.17.6.1
Priority: normallow

comment:8 Changed 5 years ago by igloo

Milestone: 7.6.17.6.2

comment:9 Changed 3 years ago by thoughtpolice

Milestone: 7.6.27.10.1

Moving to 7.10.1.

comment:10 Changed 3 years ago by literon

difficulty: Unknown

I would appreciate if this were addressed. In particular, I use GHC API for typechecking only and don't intend to evaluate expressions. I would handle killing the GHC thread myself if needed so.

At minimum, a documentation comment should be added to runGhc that it installs signal handlers without restoring them to spare people from surprises.

Note that non-responsiveness to Ctrl-C can be worked around by using runGHC $ installHandler sigINT Default Nothing >> ....

comment:11 Changed 3 years ago by thoughtpolice

Milestone: 7.10.17.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

comment:12 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:13 Changed 21 months ago by thomie

Milestone: 8.0.1

comment:14 Changed 12 months ago by hsyl20

Cc: hsyl20 added
Differential Rev(s): D2633
Status: newpatch

comment:15 Changed 12 months ago by hsyl20

Differential Rev(s): D2633phab:D2633

Note that my patch in comment:14 doesn't make runGhc thread-safe if several runGhc are executed concurrently. E.g., in the following case:

runGhc (1) install handlers
runGhc (2) install handlers
runGhc (1) uninstall handlers
runGhc (2) uninstall handlers -- reinstall handlers installed by `runGhc (1)`
                              -- overwriting initial handlers

comment:16 Changed 12 months ago by simonmar

Good point, maybe use a reference count instead?

comment:17 Changed 12 months ago by hsyl20

Yes, I have updated the patch in Phab:D2633

comment:18 Changed 12 months ago by Ben Gamari <ben@…>

In 8a5960ad/ghc:

Uninstall signal handlers

GHC installs signal handlers in runGhc/runGhcT to handle ^C but it
never uninstalls them.
It can be an issue, especially when using GHC as a library.

Test Plan: validate

Reviewers: bgamari, erikd, austin, simonmar

Reviewed By: bgamari, simonmar

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D2633

GHC Trac Issues: #4162
Note: See TracTickets for help on using tickets.