Hi,

this patch  allows to return values across blocked signals (using
IO(Maybe) for instance).

This patch has been originally written while solving issue332.  A better
solution has been found since, but Ganesh mentioned on IRC that it might
be a good idea to integrate it nonetheless.

I believe that this patch is correct after reading a dozen of times the
specification of block/unblock, but I might still be wrong.  I have also
been told that the semantics of block/unblock would change in future
GHCs but I have not investigated this issue (more details welcomed).

Best,
Gabriel

1 patch for repository http://darcs.net:

Thu Nov  4 12:45:45 CET 2010  Gabriel Kerneis <kerneis@pps.jussieu.fr>
  * Generalize withSignalsBlocked and withGutsOf

Attachments

• generalize-withsignalsblocked-and-withgutsof.dpatch
• unnamed

Screening this at kerneis' request.

I definitely think this is a good idea (I need it for rebase, and had my 
own uglier implementation there), and I also think his implementation is 
correct, but I'd like someone else's eyes on it. We should also consider 
the implications of the introduction of mask/unmask in GHC 7.0 
(http://hackage.haskell.org/trac/ghc/ticket/1036).

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-1b8ee4c11170d931caf6cd632aafaeb6331e40a4

I'm probably not the best person to review this, but I'll give it a go 
tomorrow morning and (more likely) run crying to grown-up.

Sorry, I'm afraid I don't understand enough stuff to review this patch
effectively. :-(

I'll just lay all my little bits and pieces on the table and hope that
they can be swept into some coherent whole.

Gabriel gave me some helpful context yesterday:

  The context is that you want to be able to "restore to the original
  state" after you block some threads, that it you want to block the
  threads but when you come back, you don't want to unblock
  unconditionally because the threads may already have been blocked.

Furthermore, he told me that there will be no change to block/unblock;
they are just deprecated in GHC 7 and we'll have to use mask/unmask
instead.  That's a bit of a relief from a patch review standpoint.

General confusion 1: unblock function vs unblocking
----------------------------------------------------------------------
I'm not entirely sure I understand, though.  I think part of my
confusion stems from the existence of a function called 'unblock'
as opposed to the general effect of disabling exceptions.  So
when we say 'unblock' here, we just mean "disable"

As a sanity check, if we ignore the existence of the unblock function,
does that mean that the block behaviour of

  1. disables asynchronous exceptions
  2. job
  3. re-enables asynchronous exceptions

completely disregards whether or not asynchronous exceptions are already
blocked on the current thread?  Instead of keeping track of blockedness
counts or something, it just flips the switch on and off?

I guess so because the http://hackage.haskell.org/trac/ghc/ticket/1036
talks about "perhaps making block/unblock count nesting". It's kind of
funny because the "withFoo" style of block implies that I don't have to
worry about this...

General confusion 2: what is this function for?
----------------------------------------------------------------------
I have some silly/ignorant questions about terminology and what not.

Just taking the "before" version of this code.

> -withSignalsBlocked :: IO () -> IO ()
> -withSignalsBlocked job = (block job) `catchSignal` couldnt_do
> -    where couldnt_do s | s == sigINT = oops "interrupt"
> -                       | s ==  sigHUP = oops "HUP"
> -                       | s ==  sigABRT = oops "ABRT"
> -                       | s ==  sigALRM = oops "ALRM"
> -                       | s ==  sigTERM = oops "TERM"
> -                       | s ==  sigPIPE = return ()
> -                       | otherwise = oops "unknown signal"
> -          oops s = hPutStrLn stderr $ "Couldn't handle " ++ s ++
> -                   " since darcs was in a sensitive job."

If I'm reading my textbook right [1], this function is a bit of a
misnomer in two ways.

First, it's not literally blocking signals in the sense of "don't
deliver these signals yet"; it's actually accepting the signals and
dropping them, notifying the user that it has done so.  

Second, this isn't just about signals.  I was puzzling over the role of
the 'block' in the (block job).  It occurred to me that actually the
reason we disable asynchronous exceptions is not related to the dropping
signals on the floor task; it's just because Darcs is in a sensitive job
and you really don't want it interrupted at all.  So really the function
is just a general goAwayImBusy

[1] friend loaned me his Advanced Programming in the Unix Environment

Generalize withSignalsBlocked and withGutsOf
--------------------------------------------
> ] hunk ./src/Darcs/Repository/Internal.hs 660
> -withGutsOf :: Repository p C(r u t) -> IO () -> IO ()
> +withGutsOf :: Repository p C(r u t) -> IO a -> IO a

No surprises here

> -withSignalsBlocked :: IO () -> IO ()
> -withSignalsBlocked job = (block job) `catchSignal` couldnt_do

> +withSignalsBlocked :: IO a -> IO a
> +withSignalsBlocked job = block (job >>= \r ->
> +                           unblock(return r `catchSignal` couldnt_do r))

Things I think I do understand:

A. wanting to return r, and having to do it from the handler too

B. pushing the signal handler into
      >>= \r -> return r `catchSignal` couldnt_do r
   just to get r in scope of the handler

Three things I don't really understand:

1. Re B: why is it OK to go from
      catchSignal job couldnt_do
   to
      job >>= \r -> catchSignal ...
   Will signals still be caught even if job isn't wrapped by
   the catch? Why? Is it some sort of laziness-related thing?

2. Why do we wrap (return r `catchSignal` couldnt_do r) with an
   unblock?  It's not for the sake of couldnt_do as far as I can tell,
   since catch puts an implicit block around the exception handler.

   So my only guess is that for some reason you want there to be some
   gap where asynchronous exceptions are enabled but you are still
   catching/dropping signals.
   
   In other words, BEFORE:

     catch
      (1. disable exceptions
       2. job
       3. enable exceptions
       4. [GAP!])

   WRONG AFTER

     1. disable exceptions
     2. catch job (only signals raised from this thread are caught)
     3. enable exceptions

   BETTER AFTER

     1. disable exceptions
     2a. job
     2b. enable exceptions
     2c. [GAP!]
     2d. catch return
     2e. (disable exceptions)
     3. (enable exceptions)

3. What's the link between this code and what Gabriel said about
   only selectively re-enabling exceptions if they were already
   enabled before hand?  If my understanding in 'General confusion 1'
   is correct, then doesn't the block still brutally re-enable
   exceptions after we're done anyway?
   
   Does the finesse come from the unblock I was asking about in 2?  That
   only seems to guarantee that we have explicitly asked for exceptions
   to be disable during block before go and disable them

The rest of this code doesn't need much comment, just that want to
return the job result even if a signal was caught/dropped

> -    where couldnt_do s | s == sigINT = oops "interrupt"
> -                       | s ==  sigHUP = oops "HUP"
> -                       | s ==  sigABRT = oops "ABRT"
> -                       | s ==  sigALRM = oops "ALRM"
> -                       | s ==  sigTERM = oops "TERM"
> -                       | s ==  sigPIPE = return ()
> -                       | otherwise = oops "unknown signal"
> -          oops s = hPutStrLn stderr $ "Couldn't handle " ++ s ++
> -                   " since darcs was in a sensitive job."

> +    where couldnt_do r s | s == sigINT = oops "interrupt" r
> +                         | s ==  sigHUP = oops "HUP" r
> +                         | s ==  sigABRT = oops "ABRT" r
> +                         | s ==  sigALRM = oops "ALRM" r
> +                         | s ==  sigTERM = oops "TERM" r
> +                         | s ==  sigPIPE = return r
> +                         | otherwise = oops "unknown signal" r
> +          oops s r = do hPutStrLn stderr $ "Couldn't handle " ++ s ++
> +                          " since darcs was in a sensitive job."
> +                        return r

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

On Thu, Dec 23, 2010 at 12:29:40 +0000, Eric Kow wrote:
> I'm not entirely sure I understand, though.  I think part of my
> confusion stems from the existence of a function called 'unblock'
> as opposed to the general effect of disabling exceptions.  So
> when we say 'unblock' here, we just mean "disable"

Gah, standard Erician polarity inversion.
s/disable/enable/

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Unassigning me so somebody else who knows more signal handling can pick 
this up.

Eric,

Let me first state clearly what this patch is about:
- This patch is about getting a more general type for the
  withSignalsBlocked (and the related withGutsOf) function.

That's all.  In particular :
- This patch is *not* about changing the semantics, behaviour, or
  fixing a bug in this function.  
- This patch is also *not* about switching to the "mask" primitive of
  GHC 7 (although understanding what it does makes it obvious what
  should be changed to use mask).

(But, as it turns out, there is a bug in this patch. More on that below.)

Why?
----

- Having a more general type is a good thing because it means the
  function composes better: currently, you cannot run any function job
  through withSignalsBlocked and get a result back; the result is
  (arbitrarily, because of programming style) restricted to IO ().
- Focusing on this piece of code paves the road for the up-comming
  change to the mask primitive of GHC 7 (block/unblock are *deprecated*
  in GHC 7).

What is a signal?
-----------------

There is a lot to say about signals, I'll focus on what is relevant from
a Haskell point-of-view.  I might be wrong sometimes, but here is what I
understand about them anyway.

A signal is an asynchronous interrupt.  This means that you could be
interrupted anywhere in your code.

As a result, there are very few things you can do when a signal occurs.
You cannot, for instance, call functions because you have no guarantee
about the state of your stack, heap, and resources in general.  In fact,
what you do generally boils down to two strategies:
- print an error message and exit(), or
- set a flag variable to 1, and make sure that you check its value
  regularly to notice when signals happen.
(Of course, you must be very careful because it's easy to get caught in
a race-condition and other dark-corners of signal handling.)

Signals in Haskell
-------------------

I am not aware of GHC's internals, but I suspect it follows the second
strategy.  Anyway, as a Haskell programmer, you do not need to worry
about those gorry details: signals are exposed as some kind of
exceptions, and you can handle them as such.

Just like signals, those exceptions might be raised at any time.  They
are called "asynchronous":
http://www.haskell.org/ghc/docs/7.0-latest/html/libraries/base-4.3.0.0/Control-Exception.html#10

It is, of course, impossible to add exception handlers everywhere in
your code.  Instead, you chose one strategy out of two, as above:
- accept that signal might occurr and kill your application at any point
  (this is okay as long as do not perform critical operations), or
- ignore signals during your critical operations, and check afterwards
  which signals happened, and if you want to do something about them.

Ignoring signals is done with the "block" primitive:
   block job
will compute job, record any signal happening during the computation,
and raise the corresponding exceptions once job has returned.  You have
the guarantee that job has not been interrupted (hence no critical
operation is aborted too early), and you get either the result of job or
an asynchronous exception (that might be caught somewhere else).

What about withSignalsBlocked?
------------------------------

If you want to catch those signals, you do:
   (block job) `catch` ...
but this comes at a cost:  the two branches of "catch" must have the
same type, so what you return cannot depend on the result of job.

This is exactly what withSignalsBlocked does: it computes an IO (), and
then catches any signal and prints a (warning) message.  Then it goes on
(signals are ignored).

What about patch455?
--------------------

Now, if you want to catch signals, but return the result of job anyway,
you should do the following:
  block signals
  r <- job
  unblock signals and try to return r
  catch exceptions, if any, print a message and return r eventually

In real Haskell code, this is done with the "unblock" primitive:
  block (
  job >>= \r ->
  unblock (return r)
  `catch`
  (\signal -> hPutStrLn stderr "error" >> return r))

If some signals have been queued, they are raised when entering
"unblock".  Note that if a signal happens after entering unblock but
before returning r, it will also be catched by "catch".  On the other
hand, a signal cannot happen in the signal handler (the right hand-side
of `catch`) because there is an implicit "block" protecting it (added by
GHC itself).

End of the story.  Except there is a bug...

The bug (or why signals are tricky)
-----------------------------------

There is a bug in patch455.  It does follow the above scheme, but rather
the following one:

  block (
  job >>= \r ->
  unblock (return r
  `catch`
  (\signal -> hPutStrLn stderr "error" >> return r)))

Notice the slightly incorrect bracket after unblock...

What happens is that a signal might occurr after we enter unblock but
before we return r.  In that case, it will not be caught by
`catch`.  As Simon Marlow explained on IRC:
> yes that can leak async exceptions - the catch needs to fully surround
> the unblock

To summarize, patch455 is something like:
  unblock (catch (return r) handle_signals)
whereas you need:
  catch (unblock $ return r) handle_signals

I shall submit shortly an amended patch fixing this issue.

End of the story.  What follows is about what patch455 does not do.


The other potential bug (or why mask is better than unblock)
------------------------------------------------------------

Actually, patch455 does change the semantics of withSignalsBlocks.  This
has to do with why block/unblock have been deprecated, and the new
"mask" primitive.

Consider the following code, which does some critical work, and calls
withSignalsBlocked at some point:

  withSignalsBlocked $ 
    ...
    withSignalsBlocked job
    ...

(If you find this stupid, imagine withSignalsBlocked is not called
directly, but burried in a few function calls.)

What happens if a signal is raised in "job"?
- With the current version, it will be caught by the external
  withSignalsBlocked because signals are still blocked by the external
  block.
- After patch455, it will be caught by the inner withSignalsBlocked
  because we explicitely call "unblock", which unblocks every signal
  even if there is an enclosing call to "block"!.

This looks like a small change, not much of concern (we are just
changing the point where exceptions are caught).

Now assume that at some point in darcs' code you use unblock.  For
instance, assume that my bug went unnoticed: you might leak an
asynchronous exception from the inner withSignalsBlocked, thus breaking
your external critical computation.

To retain the semantics of withSignalsBlocked, and avoid issues with
leaking exceptions, you shall use mask instead of block/unblock
(introduced in GHC 7):

  mask $ \restore -> (
  job >>= \r ->
  restore (return r)
  `catch`
  (\signal -> hPutStrLn stderr "error" >> return r))

mask/restore is exactly like block/unblock except restore will not do
anything if signals were already blocked before entering block.

You must also be careful not to use unblock anywhere (otherwise there is
no point in using mask).

How to handle legacy GHC versions?
----------------------------------

It turns out that mask is a very simple wrapper around block/unblock.
We could therefore provide it in Workaround.hs for GHC < 7 (probably the
best thing to do).  Or implement the same logic directly in
withSignalsBlocked (not a good idea, IMHO).

http://www.haskell.org/ghc/docs/7.0-latest/html/libraries/base-4.3.0.0/src/GHC-IO.html#mask

Open questions
--------------

- Some functions are interruptible.  The documentation explains it, but
  I do not fully understand what it means. I/O functions seem to be
  interruptible, so this is probably something we should understand.
  There is uninterruptibleMask:
  http://www.haskell.org/ghc/docs/7.0-latest/html/libraries/base-4.3.0.0/Control-Exception.html#v:uninterruptibleMask
  which might be necessary (the doc says: THIS SHOULD BE USED WITH GREAT
  CARE).
- Shall we ignore signals (as we do currently) or abort once the
  sensitive operation has completed?

Best,
-- 
Gabriel

This patch should be applied as soon as possible to screened, since it
fixes a serious race-condition (or the previous patch should be
reverted).

See http://bugs.darcs.net/msg13526 for a rationale.

1 patch for repository http://darcs.net/screened:

Wed Jan 19 18:45:40 CET 2011  Gabriel Kerneis <kerneis@pps.jussieu.fr>
  * Fix race-condition in withSignalsBlocked

Attachments

• fix-race_condition-in-withsignalsblocked.dpatch
• unnamed
• unnamed

Eric,

I forgot to answer your specific questions.  You might have spotted yet
another bug (!), please see point 3 below.

On Thu, Dec 23, 2010 at 12:39:58PM +0000, Eric Kow wrote:
> General confusion 1: unblock function vs unblocking
> ----------------------------------------------------------------------

More on that below.

> General confusion 2: what is this function for?
> ----------------------------------------------------------------------
> First, it's not literally blocking signals in the sense of "don't
> deliver these signals yet"; it's actually accepting the signals and
> dropping them, notifying the user that it has done so.  

Yes.

> Second, this isn't just about signals.  I was puzzling over the role of
> the 'block' in the (block job).  It occurred to me that actually the
> reason we disable asynchronous exceptions is not related to the dropping
> signals on the floor task; it's just because Darcs is in a sensitive job
> and you really don't want it interrupted at all.  So really the function
> is just a general goAwayImBusy

No.  It uses `catchSignals` which only catches signal-related
exceptions.  Other kinds of exceptions would be allowed to go through (I
believe).

> Generalize withSignalsBlocked and withGutsOf
> --------------------------------------------
> Three things I don't really understand:
> 
> 1. Re B: why is it OK to go from
>       catchSignal job couldnt_do
>    to
>       job >>= \r -> catchSignal ...
>    Will signals still be caught even if job isn't wrapped by
>    the catch? Why? Is it some sort of laziness-related thing?

Yes, they will be caught, because they are raised only when entering
unblock.  Job is wrapped in block, which delays signals until we unblock
them.

> 2. Why do we wrap (return r `catchSignal` couldnt_do r) with an
>    unblock?  It's not for the sake of couldnt_do as far as I can tell,
>    since catch puts an implicit block around the exception handler.

This is a bug (see previous mail).  I should have wrapped unblock in
catchSignal instead.

> 3. What's the link between this code and what Gabriel said about
>    only selectively re-enabling exceptions if they were already
>    enabled before hand?  If my understanding in 'General confusion 1'
>    is correct, then doesn't the block still brutally re-enable
>    exceptions after we're done anyway?

I might be wrong, but I think (hope!) block does not perform this kind
of brutal re-enabling.  I assume that it respects higher-level blocks
(even if unblock is brutal).

If this is not the case, the "potential bug" in my previous mail is in
fact an "actual bug" in current darcs code and my proposal for fixing it
should make it in 2.8 (even if we can hope there is not nested call to
withSignalsBlocked in darcs currently).  We should ask Simon Marlow, he
understands the semantics of block/unblock far better than I do.

Best,
-- 
Gabriel Kerneis

On Wed, Jan 19, 2011 at 07:21:55PM +0100, Gabriel Kerneis wrote:
> > 3. What's the link between this code and what Gabriel said about
> >    only selectively re-enabling exceptions if they were already
> >    enabled before hand?  If my understanding in 'General confusion 1'
> >    is correct, then doesn't the block still brutally re-enable
> >    exceptions after we're done anyway?
> 
> I might be wrong, but I think (hope!) block does not perform this kind
> of brutal re-enabling.  I assume that it respects higher-level blocks
> (even if unblock is brutal).

block restores the previous state: see "Asynchronous Exceptions in
Haskell" by Simon Marlow, Moran & SPJ, section 5.2 (it is very clearly
stated).  http://community.haskell.org/~simonmar/papers/async.pdf

Eric, if you still feel unsure about what signals do, you should read
this paper.  It made my day.

So there is no bug (except if someone calls "unblock" in the middle of a
critical operation).

Best,
-- 
Gabriel Kerneis

On Wed, Jan 19, 2011 at 19:21:55 +0100, Gabriel Kerneis wrote:
> I forgot to answer your specific questions.  You might have spotted yet
> another bug (!), please see point 3 below.

Hey thanks! These seem like really in-depth and helpful replies.

It's going to take me a while to digest them, so I just want to stress
for the list that it may be best for another reviewer to take a look so
Gabriel's patches don't get held up in the meantime.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

(Second patch screened as requested)

I've read through the discussion and it makes sense to me, but I 
wouldn't say that I understand the issues well enough to be absolutely 
certain this is all safe. For what it's worth, I don't think we 
currently use withSignalsBlocked in a nested way, nor call unblock 
ourselves. There's no accounting for libraries, of course.

In any case, I think this patch (plus followup fix) should be accepted. 
The possible followup changes you mention can be considered later if 
anyone implements them.

I agree with using mask + workaround for previous GHC, and I also agree 
about aborting if a signal was indeed caught - it's always seemed wrong 
that we catch it and throw it away.

Already in, I've created http://bugs.darcs.net/issue2048 to track
followup discussions.

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-1b8ee4c11170d931caf6cd632aafaeb6331e40a4

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-1b8ee4c11170d931caf6cd632aafaeb6331e40a4