Fix risk of dead-lock in documentation of Control.Concurrent

The current documentation of Control.Concurrent proposes a function to wait for a child thread to complete:

   myForkIO :: IO () -> IO (MVar ())
   myForkIO io = do
     mvar <- newEmptyMVar
     forkIO (io `finally` putMVar mvar ())
     return mvar

This function has the risk of causing a dead-lock. If an asynchronous exception if thrown before the putMVar exception handler is installed the mvar will remain empty. This causes a dead-lock when the mvar is taken.

The attached patch fixes this problem by correctly masking asynchronous exceptions before forking. Also, instead of returning the mvar it returns a computation that waits for the child thread to complete. This is safer than returning the mvar itself since a user can't accidentally put the mvar (which will dead-lock) or take the mvar (which when executed twice will dead-lock).

The attached patch additionally rewrites the function to wait for a group of threads to complete. Instead of keeping a list of MVars, I use a counter (stored in a TVar) that counts the number of running threads. The counter is incremented when a thread if forked and decremented when it completes. Waiting for the group of threads to complete is accomplished by checking if the counter has reached 0 and if not retry the transaction.

Trac metadata

Trac field	Value
Version	7.4.1
Type	Bug
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	libraries/base
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

changed weight to 5

added Tbug Trac import core libraries labels

Attached file 0001-Fix-documentation-of-waiting-for-threads-in-Control..patch (download).

I've actually been intending to fix this a different way. I want to add forkFinally:

forkFinally :: IO a -> (Either SomeException a -> IO ()) -> IO ThreadId
forkFinally action and_then =
  mask $ \restore ->
    forkIO $ try (restore action) >>= and_then

Furthermore I'd like to add an Async API that builds on this to add some higher level concurrency operators, but I'll post a proposal on the libraries list about this.

I'm looking forward to it. The functions in my threads package can also benefit from this. Or maybe the package will be deprecated in favor of your new Async API.

closed

Fixed:

commit 40d1be115d2a5a409e9b747c347cd909a9607c42
Author: Simon Marlow <marlowsd@gmail.com>
Date:   Thu Jun 7 16:10:47 2012 +0100

    add forkFinally
    
    This is a more robust version of "forkIO (m `finally` k)", because it
    closes a window between thread creation and the finally where the
    thread can receive an async exception.  Useful for layers over threads
    that need to catch threads dying with absolute certainty.
    
    forkFinally :: IO a -> (Either SomeException a -> IO ()) -> IO ThreadId
    forkFinally action and_then =
      mask $ \restore ->
        forkIO $ try (restore action) >>= and_then

and the new Async API is currently being discussed on the libraries list; a preliminary implementation is here: https://github.com/simonmar/async

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

added documentation label

added Pnormal label