Making getAddrInfo polymorphic by kazu-yamamoto · Pull Request #587 · haskell/network

kazu-yamamoto · 2024-08-30T06:06:08Z

The recent GHCs warn partial pattern matchings and partial functions.
So, let's provide getAddrInfoNE:

Since GHC 9.8 or later warn partial functions, I begin to think to provide:

getAddrInfoNE
    :: Maybe AddrInfo
    -> Maybe HostName
    -> Maybe ServiceName
    -> IO (NonEmpty AddrInfo)

@khibino Would you review this PR?

endgame

Would it be worth putting a {-# DEPRECATED #-} on getAddrInfo, and laying out a migration plan of:

Remove getAddrInfo
Re-add getAddrInfo = getAddrInfoNE, deprecate getAddrInfoNE
Remove getAddrInfoNE?

kazu-yamamoto · 2024-08-30T09:34:23Z

It is a nice idea to put DEPRECATED onto getAddrInfo.
But to maintain the backward compatibility, I would not like to do the other steps.

kazu-yamamoto · 2024-08-30T09:37:48Z

It would be worth making getAddrInfo polymorphic.

Roughly,

getAddrInfo
    :: (??? t)
    => Maybe AddrInfo
    -> Maybe HostName
    -> Maybe ServiceName
    -> IO (t AddrInfo)
    ```

kazu-yamamoto · 2024-08-30T09:49:50Z

@endgame d97f57d implements the polymorphic getAddrInfo.
I hope you like it.

endgame · 2024-08-30T10:04:00Z

Some thoughts, which might conflict with each other:

Does GHC let us deprecate instances yet?
Have you considered only exporting the method and not the typeclass? I don't know if that makes the haddocks too confusing but I can't see anyone else needing to create an instance. An appropriate haddock on the method would make it clear that the NonEmpty instance is preferred but the [] instance works.

endgame · 2024-08-30T10:04:59Z

Network/Socket/Info.hsc

+    -> IO (NE.NonEmpty AddrInfo)
+getAddrInfoNE hints node service =
+    -- getAddrInfo never returns an empty list.
+    NE.fromList <$> getAddrInfo hints node service


Since getAddrInfoList also does a non-empty check and throws an IO error if the list is empty, you could make getAddrInfoNE the "real" function and implement getAddrInfoList by fmapping toList over the result of getAddrInfoNE?

kazu-yamamoto · 2024-08-31T01:45:31Z

Does GHC let us deprecate instances yet?

I cannot find a way to deprecate instance from the GHC documentation.

kazu-yamamoto · 2024-08-31T01:46:10Z

Have you considered only exporting the method and not the typeclass?

OK. Done.

khibino · 2024-09-03T08:11:13Z

How about not allocating the head tuple when constucting the NonEmpty AddrInfo as follows?

khibino@630cc16

kazu-yamamoto · 2024-09-03T22:08:44Z

@khibino 80fadc1 implements your idea with the original code logic.

endgame · 2024-09-03T23:21:36Z

Network/Socket/Info.hsc

-getAddrInfo
+class GetAddrInfo t where
+    -----------------------------------------------------------------------------
+    -- | Resolve a host or service name to one or more addresses.


I rendered the haddocks, and it doesn't give the reader a way to see what instances of GetAddrInfo exist. I think we should document them here.

Alternatively, we could use the IsList class? But I think that will be more confusing and should only be for -XOverloadedLists support.

It seems to me that IsList is confusing.
1411634 improves the doc.

khibino · 2024-09-04T08:11:03Z

IsList example:
khibino@3bc61f1

Applicative and Semigroup example:
khibino@0141ba2

Which type signature would be better?

endgame · 2024-09-05T00:02:57Z

IsList example: khibino@3bc61f1

Applicative and Semigroup example: khibino@0141ba2

Which type signature would be better?

I think Applicative × Semigroup doesn't necessarily give you right semantics (it requires (<>) = liftA2 (<>) which is common but by no means universal.) I agree with @kazu-yamamoto that IsList looks confusing. I think the design decision reduces to: do we make the function polymorphic with a custom typeclass, or do we make two distinct named functions and deprecate the [] one? I could go either way. It's a real shame that we can't deprecate typeclass instances.

kazu-yamamoto · 2024-09-05T00:16:29Z

@endgame I like the current implementation.

kazu-yamamoto · 2024-09-06T00:16:16Z

Is this ready for merging?

endgame

Some final thoughts, most small. I reworded the haddock too.

Network/Socket/Info.hsc

endgame · 2024-09-06T00:45:47Z

Network/Socket/Info.hsc

+    -- >>> addr <- NE.head <$> getAddrInfo (Just hints) (Just "127.0.0.1") (Just "http")
+    -- >>> addrAddress addr
+    -- 127.0.0.1:80
+    getAddrInfo


A @since line might also be useful for future readers, to know when a NonEmpty is available.

Co-authored-by: endgame <[email protected]>

endgame

Seems good. One final nit (my fault, sorry) and a thought for future work. Thank you for this.

Network/Socket/Info.hsc

endgame · 2024-09-06T02:36:33Z

Network/Socket/Info.hsc

-    | ptr_ai == nullPtr = return []
+    -- POSIX requires that getaddrinfo(3) returns at least one addrinfo.
+    -- See: http://pubs.opengroup.org/onlinepubs/9699919799/functions/getaddrinfo.html
+    | ptr_ai == nullPtr = ioError $ mkIOError NoSuchThing "getaddrinfo must return at least one addrinfo" Nothing Nothing


Might be nice to capture errno in the IO error, but that's out of scope for this PR.

The error reporting of network is historically poor.
We should implement a mechanism to maintain error numbers in the future.

Co-authored-by: endgame <[email protected]>

kazu-yamamoto · 2024-09-11T00:32:39Z

Merged.
Thank you for your review.
I will release a new version.

haskell#587 removed the call to c_freeaddrinfo in getAddrInfoNE. 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.

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

providing getAddrInfoNE

8c18f94

kazu-yamamoto mentioned this pull request Aug 30, 2024

Does getAddrInfo return IO (NonEmpty AddrInfo)? #407

Closed

endgame reviewed Aug 30, 2024

View reviewed changes

kazu-yamamoto added 3 commits August 31, 2024 10:41

making getAddrInfo polymorphic

152aea0

exporting getAddrInfo only

c3fbccc

defining followAddrInfo for NonEmpty

795cebb

kazu-yamamoto force-pushed the non-empty branch from d97f57d to 795cebb Compare August 31, 2024 01:41

kazu-yamamoto changed the title ~~Providing getAddrInfoNE~~ Makeing getAddrInfo polymorphic Sep 3, 2024

using the original list code.

80fadc1

improve the error message

30df86b

endgame reviewed Sep 3, 2024

View reviewed changes

improving document

1411634

endgame reviewed Sep 6, 2024

View reviewed changes

kazu-yamamoto and others added 4 commits September 6, 2024 11:21

Update Network/Socket/Info.hsc

ff99e97

Co-authored-by: endgame <[email protected]>

moving the comment (suggested by @endgame)

e32a05e

Update Network/Socket/Info.hsc

34bbbfe

Co-authored-by: endgame <[email protected]>

adding @SInCE

ae72d08

kazu-yamamoto changed the title ~~Makeing getAddrInfo polymorphic~~ Making getAddrInfo polymorphic Sep 6, 2024

endgame approved these changes Sep 6, 2024

View reviewed changes

Update Network/Socket/Info.hsc

b7ba6ee

Co-authored-by: endgame <[email protected]>

kazu-yamamoto merged commit a7b15f8 into haskell:master Sep 11, 2024

kazu-yamamoto deleted the non-empty branch September 11, 2024 00:32

arybczak mentioned this pull request Nov 25, 2024

Fix memory leak in getaddrinfo and make it async exception safe #591

Closed

Conversation

kazu-yamamoto commented Aug 30, 2024

Uh oh!

endgame left a comment

Choose a reason for hiding this comment

Uh oh!

kazu-yamamoto commented Aug 30, 2024

Uh oh!

kazu-yamamoto commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazu-yamamoto commented Aug 30, 2024

Uh oh!

endgame commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazu-yamamoto commented Aug 31, 2024

Uh oh!

kazu-yamamoto commented Aug 31, 2024

Uh oh!

khibino commented Sep 3, 2024

Uh oh!

kazu-yamamoto commented Sep 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khibino commented Sep 4, 2024

Uh oh!

endgame commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazu-yamamoto commented Sep 5, 2024

Uh oh!

kazu-yamamoto commented Sep 6, 2024

Uh oh!

endgame left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

endgame left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazu-yamamoto commented Sep 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kazu-yamamoto commented Aug 30, 2024 •

edited

Loading

endgame commented Sep 5, 2024 •

edited

Loading