Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does getAddrInfo return IO (NonEmpty AddrInfo)? #407

Open
endgame opened this issue Jun 3, 2019 · 11 comments · Fixed by #408
Open

Does getAddrInfo return IO (NonEmpty AddrInfo)? #407

endgame opened this issue Jun 3, 2019 · 11 comments · Fixed by #408

Comments

@endgame
Copy link
Contributor

endgame commented Jun 3, 2019

The documentation for Network.Socket.getAddrInfo specifies that:

If the query fails, this function throws an IO exception instead of returning an empty list. Otherwise, it returns a non-empty list of AddrInfo values.

That the res pointer is given a non-empty list appears to be required by POSIX:

Upon successful return of getaddrinfo(), the location to which res points shall refer to a linked list of addrinfo structures, each of which shall specify a socket address and information for use in creating a socket with which to use that socket address. The list shall include at least one addrinfo structure.

The manpages on my system do not mention this postcondition.

The implementation in Info.hsc does not seem to enforce this.

How best to proceed? The ideal world in my mind involves returning IO (NonEmpty AddrInfo), but there are some complications:

  • base >= 4.9 (GHC >= 8) gives us NonEmpty.
  • If we want to maintain support for the older GHCs, we can add a conditional dependency on semigroups for GHC < 8.
  • If we add the conditional dependency, old versions of GHC will pull in extra dependencies. Many of semigroups' dependencies are boot packages, but not all; I do not know whether this is acceptable.

Whether or not getAddrInfo returns NonEmpty or [], should we guarantee that a nonempty list is returned by throwing, if we see an empty list in res? The function is already potentially throwing IOError.

@kazu-yamamoto
Copy link
Collaborator

I think it's too late to change the result type to IO (NonEmpty AddrInfo). I prefer to throw an exception if we see an empty list in res.

@endgame
Copy link
Contributor Author

endgame commented Jun 4, 2019

Ah, that's a shame. I personally disagree, but you have a lot of reverse-dependencies to consider. Here is a PR to raise an exception.

@kazu-yamamoto
Copy link
Collaborator

I don't understand why you think it's a shame.
Even if we change the signature to IO (NonEmpty AddrInfo), it should throw an exception at the case of empty res, right?

@endgame
Copy link
Contributor Author

endgame commented Jun 6, 2019

Correct, we will have to throw some exception (even if it is just an error call) either way. I'll try to explain my thinking further.

I believe that types should convey as much information as practically possible. The implied contract of functions in this library is "result or throw exception", which means that for getAddrInfo, the contract from context and types is "list-that-may-be-empty or throw exception". Returning NonEmpty moves the information that the result is never empty from the haddock to the type. Functions that use the result of getAddrInfo can easily rely on at least one result being there, head in Data.List.NonEmpty is a total function, and so on.

But as you said earlier, it might be too late to change that type because it breaks so many reverse dependencies. I might make changes to the example code on my PR branch, so that people get in the habit of immediately turn the [AddrInfo] into a NonEmpty AddrInfo.

@eborden
Copy link
Collaborator

eborden commented Jun 7, 2019

@endgame The more compatible way to change the type of getAddrInfo is to introduce a new version of that function that returns NonEmpty. We can then deprecate getAddrInfo in a later version.

@endgame
Copy link
Contributor Author

endgame commented Jun 7, 2019

I am willing to prepare such a PR. My initial thought was getAddrInfo', with a plan that looks something like:

  1. Today: getAddrInfo' = fromList <$> getAddrinfo, mark getAddrInfo deprecated.
  2. Some future version: replace getAddrInfo = getAddrInfo', mark getAddrInfo' deprecated.
  3. Some far future version: remove getAddrInfo'.

What did you have in mind?

@endgame
Copy link
Contributor Author

endgame commented Jun 8, 2019

Ah, I forgot. The other problem with bringing in NonEmpty now is that this library still claims to support GHC 7.10, which doesn't have NonEmpty in base. Adding a conditional dependency on semigroups will add a bunch of dependencies to those builds, but maybe that's OK? Alternatively, do you have a timeline for retiring GHC 7.10?

@eborden
Copy link
Collaborator

eborden commented Jun 9, 2019

Yes, the dependency issues is a problem. Our official support is 3 versions, so technically we are really only bound to 8.2.x. However I'd really only drop support for an earlier version if there is a very compelling case. I'm not sure if this case warrants that, but @kazu-yamamoto may feel differently.

For now the exception proposal gets us visibility if an issue occurs.

@kazu-yamamoto
Copy link
Collaborator

I was a fan of NonEmpty but recently not honestly. Actually, I'm trying to remove NonEmpty from my code since it makes code complicated but gains little in my cases. See yesodweb/wai@919eb78#diff-1844ab066ed4af048ce229abe6e7e69fL16

In the case of getAddrInfo, I don't see much benefit, either. We already throw an exception in the case of empty and the pattern match addr:_ <- getAddrInfo does not fail anymore.

We would feel very good if we provided good type signatures for APIs. But it is inconvenient for uses to ask change the current style and switch to NonEmpty, which has a dependency problem.

@kazu-yamamoto kazu-yamamoto reopened this Aug 30, 2024
@kazu-yamamoto
Copy link
Collaborator

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)

@kazu-yamamoto
Copy link
Collaborator

See #587.

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 a pull request may close this issue.

3 participants