Fix memory leak in getaddrinfo and make it async exception safe

[arybczak]

#587 removed the call to c_freeaddrinfo in getaddrinfo.

This restores it and in addition makes the function async exception safe.

[kazu-yamamoto]

Thank you for bringing this issue.
This is a nice catch.

This is a common pattern.
So, I would like you to check if bracketOnError can be used instead of mask.

[arybczak]

bracket can be used, but then the code will have to check ret /= 0 twice unlike the current version. If you're fine with that, I can edit it.

[kazu-yamamoto]

I meant bracketOnError, not bracket.

[kazu-yamamoto]

See the following definition. It uses the same pattern as you wrote.

bracketOnError
        :: IO a         -- ^ computation to run first (\"acquire resource\")
        -> (a -> IO b)  -- ^ computation to run last (\"release resource\")
        -> (a -> IO c)  -- ^ computation to run in-between
        -> IO c         -- returns the value from the in-between computation
bracketOnError before after thing =
  mask $ \restore -> do
    a <- before
    restore (thing a) `onException` after a

[kubek2k]

@kazu-yamamoto seems to me that the result of before would have to be visible outside of bracketOnError call (ret /= 0) which defeats the purpose here :(

[arybczak]

It uses the same pattern as you wrote.

Not really, because I used c_freeaddrinfo twice, just as in regular bracket.

In any case, I tried to use bracket but branching on ret throws a wrench into this pattern and makes the code very convoluted:

      bracket
        (do ret <- c_getaddrinfo c_node c_service c_hints ptr_ptr_addrs
            ptr_addrs <- if ret == 0
                         then peek ptr_ptr_addrs
                         else return nullPtr
            return (ret, ptr_addrs)
        )
        (\(ret, ptr_addrs) -> do
            -- Don't call freeaddrinfo with NULL, it segfaults on some
            -- platforms.
            when (ret == 0) $ c_freeaddrinfo ptr_addrs
        )
        (\(ret, ptr_addrs) ->
           if ret == 0
           then followAddrInfo ptr_addrs
           else do
             err <- gai_strerror ret
             ioError $ ioeSetErrorString
               (mkIOError NoSuchThing message Nothing Nothing)
               err
        )

that's why I unrolled it in the first place and I don't think trying to force it is worth it.

If you disagree, please fix it the way you like.

Also, please make a new release with this fix asap and deprecate 3.2.3.0, 3.2.4.0, 3.2.5.0 and 3.2.6.0 on Hackage because this is a very serious bug.

[Vlix]

How about something like this?

getAddrs = do
    ret <- c_getaddrinfo c_node c_service c_hints
    if ret == 0
        then Right <$> peek ptr_ptr_addrs
        else pure $ Left ret

addrErr ret = do
    err <- gai_strerror ret
    ioError $ ioeSetErrorString
        (mkIOError NoSuchThing message Nothing Nothing)
        err

bracket
    getAddrs 
    (mapM_ c_freeaddrinfo)
    (either addrErr followAddrInfo)

[kazu-yamamoto]

Not really, because I used c_freeaddrinfo twice, just as in regular bracket.

You are right.

After this discussion, the original PR code is simplest, I think.
So, let's merge this.

[kazu-yamamoto]

Now LGTM.

[kazu-yamamoto]

v3.2.7.0 has been released.
v3.2.3.0, 3.2.4.0, 3.2.5.0 and 3.2.6.0 are deprecated.
Thank you for your contribution!

[Vlix]

@kazu-yamamoto
It seems 3.2.7.0 is also deprecated on hackage?
(and 3.2.3.0 is not)

[kazu-yamamoto]

My bad. X-(
Fixed!