Skip to content

Conversation

@arybczak
Copy link
Contributor

#587 removed the call to c_freeaddrinfo in getaddrinfo.

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

haskell#587 removed the call to `c_freeaddrinfo`
in `getaddrinfo`.

This restores it and in addition makes the function async exception safe.
@kazu-yamamoto kazu-yamamoto self-requested a review November 25, 2024 23:09
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Collaborator

I meant bracketOnError, not bracket.

@kazu-yamamoto
Copy link
Collaborator

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
Copy link

kubek2k commented Nov 26, 2024

@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
Copy link
Contributor Author

arybczak commented Nov 26, 2024

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
Copy link

Vlix commented Nov 26, 2024

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
Copy link
Collaborator

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 kazu-yamamoto self-requested a review November 27, 2024 00:07
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now LGTM.

kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Nov 27, 2024
@kazu-yamamoto
Copy link
Collaborator

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
Copy link

Vlix commented Nov 27, 2024

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

@kazu-yamamoto
Copy link
Collaborator

My bad. X-(
Fixed!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 30, 2025
## Version 3.2.7.0

* Using nested `bracket` for `gracefulClose`.
  [#591](haskell/network#590)
* Fix memory leak in getaddrinfo and make it async exception safe.
  [#591](haskell/network#591)
* Make call to c_free async exception safe.
  [#592](haskell/network#592)

## Version 3.2.6.0

* fixing the Show instance of IPv4-mapped IPv6 address on little endian machines

## Version 3.2.5.0

* `gracefulClose` based on STM racing and `timeout`.
  [#587](haskell/network#587)

## Version 3.2.4.0

* New API: setSockOptValue.
  [#588](haskell/network#588)

## Version 3.2.3.0

* Making getAddrInfo polymorphic
  [#587](haskell/network#587)

## Version 3.2.2.0

* New API: waitReadSocketSTM, waitAndCancelReadSocketSTM,
  waitWriteSocketSTM, waitAndCancelWriteSocketSTM
  [#586](haskell/network#586)
* Checking the length of ASCII string allowing trailing 0.
  [#585](haskell/network#585)

## Version 3.2.1.0

* Trying to release with the latest autoreconf.
  Packing "network" in the local directory instead of CI.
* Remove includes from .cabal-file
  [#583](haskell/network#583)
* making gracefulClose more graceful
  [#580](haskell/network#580)
* Update config.guess, config.sub to their latest versions
  [#579](haskell/network#579)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants