Asynchronous exception bug in spawn

Description

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:

```haskell
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:

```haskell
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.

Environment

None

Activity

Show:
Edsko de Vries
October 23, 2012, 2:27 PM

Comment:edsko:10/23/12 03:27:40 PM:

reopened

Edsko de Vries
October 12, 2012, 8:53 AM

Comment:edsko:10/12/12 09:53:26 AM:

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

Edsko de Vries
October 11, 2012, 8:48 PM

Comment:edsko:10/11/12 09:48:45 PM:

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.

basvandijk
October 11, 2012, 8:35 PM

Comment:basvandijk:10/11/12 09:35:21 PM:

> 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](http://hackage.haskell.org/packages/archive/base/latest/doc/html/Control-Exception.html_t:AsyncException) 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 de Vries
October 10, 2012, 8:29 AM

Comment:edsko:10/10/12 09:29:56 AM:

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

Assignee

Edsko de Vries

Reporter

basvandijk

External issue ID

None

OS

None

Affects versions

Priority

Major