#6126 closed bug (fixed)

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

Reported by: basvandijk Owned by:
Priority: normal Milestone:
Component: libraries/base Version: 7.4.1
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Documentation bug Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

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.

Attachments (1)

0001-Fix-documentation-of-waiting-for-threads-in-Control..patch (5.0 KB) - added by basvandijk 23 months ago.

Download all attachments as: .zip

Change History (4)

comment:1 Changed 23 months ago by simonmar

  • Difficulty set to Unknown

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.

comment:2 Changed 23 months ago by basvandijk

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.

comment:3 Changed 22 months ago by simonmar

  • Resolution set to fixed
  • Status changed from new to 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

Note: See TracTickets for help on using tickets.