Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#4504 closed bug (fixed)

"awaitSignal Nothing" does not block thread with -threaded

Reported by: adept Owned by:
Priority: normal Milestone: 7.0.2
Component: Runtime System Version: 7.0.1
Keywords: Cc: kazu@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Consider the following code:

module Main where

import System.Posix.Signals
import Control.Concurrent

main :: IO ()
main = do
  awaitSignal Nothing >> yield

According to the System.Posix.Signals haddock, "awaitSignal Nothing" should call pause(2), causing main thread to block until some signal has arrived. This is indeed what happens when this code is compiled by GHC 7.0.1 like so: "ghc --make awaitSignal.hs".

However, when compiled with -threaded, this code exits immediately when run. I think this is a bug in -threaded runtime.

I've tested with GHC 6.12.1, and this behavior is present there as well. Maybe I'm missing something non-obvious here?

My environment: x86, Debian sid, GHC 6.12.1 from Debian packages, GHC 7.0.1 from binary packages from GHC site.

Attachments (4)

4504.bundle (9.3 KB) - added by adept 7 years ago.
Masking scheduler timer signal under threaded RTS
4504-1.bundle (9.4 KB) - added by adept 7 years ago.
Excluded check for THREADED_RTS, import rtsSupportsBoundThreads from Control.Concurrent
export-the-value-of-the-signal-used-by-scheduler-__4504_.dpatch (16.9 KB) - added by adept 7 years ago.
clarify-behavior-of-_awaitsignal-nothing__-export-signalset-that-includes-all-signals-reserved-by-rts-__4504_.dpatch (9.1 KB) - added by adept 7 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 7 years ago by adept

Debian sid, GHC 6.12.3 from the binary package: same behavior (exits with "-threaded").

Versions of unix package:

6.12.1: 2.4.0.0
6.12.3: 2.4.0.2
7.0.1:  2.4.1.0

comment:2 Changed 7 years ago by kazu-yamamoto

Cc: kazu@… added

comment:3 Changed 7 years ago by igloo

Milestone: 7.2.1

Thanks for the report.

Looks like it's seeing internal SIGVTALRMs.

comment:4 Changed 7 years ago by adept

Owner: set to adept

Indeed it does. Running with +RTS -V0 gets the "proper" behavior.

comment:5 Changed 7 years ago by adept

Status: newpatch

Attached patch should fix this. Please review.

Changed 7 years ago by adept

Attachment: 4504.bundle added

Masking scheduler timer signal under threaded RTS

comment:6 Changed 7 years ago by simonmar

These CPP conditionals won't work in the .hsc file, I think, because the symbols aren't defined there. Also, the package is not build multiple times for threaded/unthreaded, so it's not meaningful to check for THREADED_RTS.

+#if defined(USE_TIMER_CREATE)
+      virtualTimerExpired
+#elif defined(HAVE_SETITIMER)
+#  if defined(THREADED_RTS) || !defined(HAVE_SETITIMER_VIRTUAL)
+      realTimeAlarm
+#  else
+      virtualTimerExpired
+#  endif
+#endif

comment:7 Changed 7 years ago by adept

Well, apart from THREADED_RTS everything else will be available via Rts.h -> MachDeps.h -> ghcautoconf.h, is it not?

Contents of the dist-install/build/System/Posix/Signals.hs support my theory: note that in the absence of needed defines, the whole #if .. #endif block would evaluate to empty. However, on my notebook, it evaluates to the (proper) value of virtualTimerExpired.

However, I agree that at the very least, check for defined(THREADED_RTS) should be removed. Modified patch will be attached after this comment.

Changed 7 years ago by adept

Attachment: 4504-1.bundle added

Excluded check for THREADED_RTS, import rtsSupportsBoundThreads from Control.Concurrent

comment:8 Changed 7 years ago by simonmar

Are we really sure we should fix it this way? Perhaps instead we should document that the RTS uses SIGVTALRM or SIGALRM, and the caller should include those in the signal set unless they really know what they're doing.

comment:9 Changed 7 years ago by simonmar

Oh, a better alternative might be this:

-- | A set of signals reserved for use by the implementation.  In GHC, this will normally
-- include either `sigVTALRM` or `sigALRM`.
reservedSignals :: SignalSet

And the documentation for awaitSignals would suggest using awaitSignals (Just reservedSignals)

This could be implemented using an RTS API like

int getTimerSignal(void);

to avoid all the #ifdef stuff, and keep the choice about which signal is reserved in just one place.

comment:10 Changed 7 years ago by adept

Please note that awaitSignals (Just reservedSignals) would use just reservedSignals as signal mask, instead of adding them to the current process mask.

Let's look at the problem from this perspective: suppose that we set up a bunch of signal handlers and now we want to block until the signal arrives. Right now documentation says to use awaitSignal Nothing.

Unfortunately, awaitSignal Nothing is completely useless with "-threaded".

So, in order to write robust code that would work either way we would have to do something like this (and add this example to the documentation of "awaitSignal"):

{-# LANGUAGE CPP #-}
import Control.Concurrent (rtsSupportsBoundThreads)
import System.Posix.Signals

blockTillSignal = do
  sigset <- getSignalMask
  let sigset' =
        case rtsSupportsBoundThreads of
          False -> sigset
          True -> unionSignalSets sigset reservedSignals -- i'm making this up, no function like this right now.
  awaitSignal sigset'

Now, as you can see, this has large overlap with awaitSignal code. I think that this is a job for the library to hide away exactly this sort of complexity.

I might have a case of tunnel vision, but it seems like awaitSignal Nothing has a very limited use right now, unless one writes the code and is 100% sure that it would be used without "-threaded", which is simply not right.

comment:11 Changed 7 years ago by adept

Or, let me put it this way: awaitSignal should provide a convenient interface to sigsuspend, right?

AFAIK, in C land the most common use cases for sigsuspend is to set up some signal handlers and then either:

  1. Block until any of the signals arrive (using empty signal mask)
  1. Block until one of the subset of the handled signals arrive (using non-empty signal mask)

For the latter, we have awaitSignal (Just someMask). For former, we have nothing. Rather we have a "honey trap" - if the programmer uses awaitSignal Nothing, his code will behave strangely with "-threaded" without any indication of what went wrong.

Hence I propose to restore the utility of awaitSignal Nothing via masking the scheduler signal away, more or less like the proposed patch does.

On the other hand, we should export reservedSignals like you proposed so that programmers may check when setting signal handlers if they are likely to step on RTS toes.

How does this sound?

comment:12 Changed 7 years ago by adept

Ok. Following the discussion on IRC I've clarified the behavior of awaitSignal Nothing and exported the value of the timer signal from RTS. I hope that I did this properly - this is my "first export", so please check carefully :)

I verified and

blockSignals reservedSignals
awaitSignal Nothing

behaves as expected and as advertised with -threaded and without.

comment:13 Changed 7 years ago by simonmar

Resolution: fixed
Status: patchclosed

Applied, thanks:

ghc patch:

Wed Dec  8 10:37:55 PST 2010  Dmitry Astapov <dastapov@gmail.com>
  * Export the value of the signal used by scheduler (#4504)

libraries/unix patch:

Wed Dec  8 18:38:49 GMT 2010  Dmitry Astapov <dastapov@gmail.com>
  * Clarify behavior of "awaitSignal Nothing", export SignalSet that includes all signals reserved by RTS (#4504)

comment:14 Changed 7 years ago by simonmar

one more patch:

Fri Dec 10 01:00:45 PST 2010  Simon Marlow <marlowsd@gmail.com>
  * Fix Windows build: move rtsTimerSignal to the POSIX-only section

comment:15 Changed 7 years ago by kazu-yamamoto

Why are these patch applied only to GHC HEAD? Why don't you apply these to ghc-7.0 branch?

comment:16 in reply to:  15 Changed 7 years ago by simonmar

Replying to kazu-yamamoto:

Why are these patch applied only to GHC HEAD? Why don't you apply these to ghc-7.0 branch?

It's an API addition, and we don't normally change APIs on the stable branch. If this is important to you we could make an exception, though.

comment:17 Changed 7 years ago by kazu-yamamoto

If this patch does not break anything, please include to 7.0.2. According to adept, this patch is necessary to run the "c10k" package well.

comment:18 Changed 7 years ago by simonmar

Milestone: 7.2.17.0.2
Status: closedmerge

Ian: let's merge this into 7.0.2.

comment:19 Changed 7 years ago by igloo

Status: mergeclosed

All 3 merged

comment:20 Changed 6 years ago by kazu-yamamoto

Owner: adept deleted
Resolution: fixed
Status: closednew

This bug appears again in GHC 7.0.3. I tested the code above both on Linux and Mac. awaitSignal does not block in both cases with -threaded.

comment:21 Changed 6 years ago by kazu-yamamoto

It seemds to me that two patches out of 3 are not merged into GHC 7.0.3 nor GHC 7.0.4rc1. Is it possible to merge them to 7.0.4rc2?

comment:22 Changed 6 years ago by igloo

Resolution: fixed
Status: newclosed

All the patches looks merged to me.

If I understand correctly, the program needs to be changed to

module Main where

import System.Posix.Signals
import Control.Concurrent

main :: IO ()
main = do
  blockSignals reservedSignals
  awaitSignal Nothing >> yield

and that behaves correctly.

comment:23 Changed 6 years ago by kazu-yamamoto

Ian,

Thank you for your reply and sorry for my misunderstanding. I comfirmed that adding "blockSignals reservedSignals" fixes this.

Note: See TracTickets for help on using tickets.