concurrently' looks a tad leaky

[treeowl]

Both thread IDs are retained in case they're needed to cancel the corresponding thread (s), which as I understand it keeps the threads live. But of course there's no need to cancel a thread if it's already finished! One option might be to use something richer than an Int to keep track of threads, like Starting | Started !ThreadId | Done. Now the thread IDs can live only in an IORef, so each thread can remove itself when it's done or killed.

[treeowl]

Perhaps there's a simpler way. I haven't looked at whether this would work for concurrently', but I think it would work for its applications. For example, I think we could use something vaguely like

data Status a = Started !ThreadId | Done !(Either SomeException a)

concurrently :: IO a -> IO b -> IO (a, b)
concurrently left right = do
  mva <- newEmptyMVar
  ta <- forkIO $ (do
    ar' <- left
    swapMVar mva $ Done (Right ar'))
       `catch` \e -> swapMVar mva $ Done (Left e)
  putMVar mva (Started ta)
  br <- right `onException` takeMVar mva >>= \case
    Started tid -> throwTo tid ThreadCancelled
    Done{} -> pure ()
  ar <- takeMVar mva
  pure (ar, br)

Unlike the current code, nothing is uninterruptible.

[simonmar]

Is this really a problem? A finished ThreadId will only keep alive the topmost stack chunk, which is 1KB. I'm worried the cost of fixing this "leak" could be worse than the leak itself.

[treeowl]

Dunno! I'll let you know if I come up with a solution I like. This stuff is terribly subtle.