async icon indicating copy to clipboard operation
async copied to clipboard

Change semantics of withAsync & double performance of race and concurrently

Open basvandijk opened this issue 10 years ago • 5 comments

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 avatar Oct 09 '14 11:10 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 avatar Oct 09 '14 22:10 basvandijk

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

The latest commit actually passes the troublesome test-case.

basvandijk avatar Oct 10 '14 07:10 basvandijk

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

3noch avatar Feb 06 '17 20:02 3noch

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

simonmar avatar Feb 10 '17 20:02 simonmar

@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.

3noch avatar Feb 10 '17 21:02 3noch