Asynchronous exception bug in spawn

[basvandijk]

The following code in Network.Transport.Util is unsafe with regard to asynchronous exceptions. If an asynchronous exception is thrown to the forked thread before the endpoint address is put to the addr MVar then the final takeMVar will dead-lock:

spawn :: Transport -> (EndPoint -> IO ()) -> IO EndPointAddress
spawn transport proc = doo
  addr <- newEmptyMVar
  forkIO $ do
    Right endpoint <- newEndPoint transport
    putMVar addr (address endpoint)
    proc endpoint
  takeMVar addr

One way to solve this is using a combination of mask and try. However a way more simple implementation is:

spawn :: Transport -> (EndPoint -> IO ()) -> IO EndPointAddress
spawn transport proc = do
  Right endpoint <- newEndPoint transport
  forkIO $ proc endpoint
  return $ address endpoint

Since the original code has to wait for the completion for newEndPoint anyway we could just as well execute it in the main thread and only execute proc endpoint in a separate thread. No need for an MVar so no posibility of dead-lock.

However since this code is so simple I wonder if there's actually a need for this combinator. Is it used anywhere? If not, I propose to remove it.

[edsko]

Asynchronous exceptions are not an issue here because the thread ID of the new thread is not exposed and hence nobody can throw an asynchronous exception to that thread. Nevertheless, you are right of course that we were ignoring the possibility of error in newEndPoint. That's now fixed.

(You are also right that we could potentially removed this completely, but that would require a major version increment. Meh.)

[basvandijk]

Asynchronous exceptions are not an issue here because the thread ID of the new thread is not exposed and hence nobody can throw an asynchronous exception to that thread.

But the RTS can still throw asynchronous exceptions to the new thread.

Nevertheless, you are right of course that we were ignoring the possibility of error in newEndPoint. That's now fixed.

But why execute the newEndPoint inside the new thread and go to the trouble of creating an MVar and waiting for it when you can just execute it in the main thread which is simpler, safer and probably a bit faster to?

[edsko]

Yeah, I'm not sure either, but I probably had a (possibly wrong) reason at the time :) You're probably right that it can be simplified, but I couldn't remember why I had done it this way in the first place so I was a bit hesitant to change it.

And if your heap is overflowing inside spawn you have bigger problems :-) Point taken though.

[edsko]

It was something to do with not wanting to do certain operations on the main thread, because the main thread is by default a bound thread.. Even so it should probably be okay because newEndPoint will spawn its own threads to do most of the work..