Change semantics of withAsync & double performance of race and concurrently

[basvandijk]

Hi Simon,

This is an attempt to solve #5 by following your suggested approach of changing the semantics of withAsync such that if the inner function receives an asynchronous exception it will also be thrown to the forked action. But if it receives a synchronous action the forked action will be killed.

This change in semantics allows for an implementation of race and concurrently which only needs to fork a single thread (instead of two) which doubles performance.

I'm pretty sure that concurrently is correct. I'm less sure about race because it uses a complex implementation. Although the test cases I added for race seem to indicate it operates correctly.

Let me know what you think.

This patch was co-written with my colleague @asayers.

[basvandijk]

I'm beginning to suspect it's impossible to implement a concurrently (and probably a race to) which satisfies the spec of:

concurrently left right =
  withAsync left $ \a ->
  withAsync right $ \b ->
  waitBoth a b

but forks only a single thread.

The following test case fails for my current implementation but succeeds for the spec:

concurrently_3 :: Assertion
concurrently_3 = do
  rightTidMv <- newEmptyMVar

  exMv <- newEmptyMVar

  forkIO $ do
    threadDelay 1000
    rightTid <- takeMVar rightTidMv
    throwTo rightTid UserInterrupt

  catchIgnore $
    concurrently (threadDelay 100000 `catch` putMVar exMv)
                 (do rightTid <- myThreadId
                     putMVar rightTidMv rightTid
                     threadDelay 10000)

  ex <- takeMVar exMv

  ex @?= UserInterrupt

catchIgnore :: IO a -> IO ()
catchIgnore m = void m `catch` \(e :: SomeException) -> return ()

I will give it another try tomorrow but I suspect I won't succeed :-(

[basvandijk]

I will give it another try tomorrow but I suspect I won't succeed

The latest commit actually passes the troublesome test-case.

[3noch]

This is a really old PR. Can it be merged?

[simonmar]

@3noch perhaps.. I need to give it some serious thought. Is it important for you?

[3noch]

@simonmar I'm not sure. I had a thread that was eating exceptions and took me hours to debug. I haven't tried this patch but it got me thinking.

[simonmar]

Probably not going to pursue this one. AFAIK the performance of race and concurrently hasn't been an issue, and changing the semantics could be a source of very subtle bugs.