Skip to content

Implement socket names, in particular reading a string description of them #428

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

Closed
wants to merge 3 commits into from

Conversation

infinity0
Copy link
Contributor

Useful for 3rd-party networking applications that might want to pass around
service specifiers without worrying whether these are IP addresses, DNS names,
or UNIX-domain socket paths.

Previously, there was no data type to encapsulate these options together. In
particular, getAddrInfo had to be used to resolve DNS names into a SockAddr
before calling connect/bind, but it could not deal with UNIX domain sockets.
The new function sockNameToAddr takes this role, transparently converting DNS
names and passing through non-DNS-names unaltered, so that it can be used
uniformly without worrying about the specific type of input name/address.

The last two commits fix some historical behavioural oddities to make the
general end-user experience more friendly.

… them

Useful for 3rd-party networking applications that might want to pass around
service specifiers without worrying whether these are IP addresses, DNS names,
or UNIX-domain socket paths.

Previously, there was no data type to encapsulate these options together. In
particular, getAddrInfo had to be used to resolve DNS names into a SockAddr
before calling connect/bind, but it could not deal with UNIX domain sockets.
The new function sockNameToAddr takes this role, transparently converting DNS
names and passing through non-DNS-names unaltered, so that it can be used
uniformly without worrying about the specific type of input name/address.

--

I didn't implement a Read instance for SockAddr since readsPrec is supposed to
read in a Haskell expression with a precedence-context and that is incompatible
with how readSockName/readSockAddr is implemented.

In fact the current Show instance for SockAddr already violates the interface,
since it is supposed to output a Haskell expression. Arguably the Show instance
should be derived, and the existing instance should be moved to a top-level
function called something like showSockAddr.

However in theory "readSockAddr 0" should act as pretty much the inverse of
"show" from the current Show instance for SockAddr.
@infinity0 infinity0 force-pushed the master branch 2 times, most recently from 4dd1c4a to c4d1b5d Compare October 25, 2019 15:16
@kazu-yamamoto
Copy link
Collaborator

network is a cross-platform low level library. In my impression, this feature is a little bit middle level which should be implemented in another library atop network.

@eborden What do you think?

@infinity0
Copy link
Contributor Author

infinity0 commented Oct 29, 2019

@kazu-yamamoto Thanks for the comment, I can certainly understand your viewpoint and I was also not entirely satisfied with calling the new type "SockName" but I couldn't come up with any better options.

However I argue that "low-level" should not be interpreted as "close to the C API", and in fact this interpretation holds back non-C programming languages from fully-realising their own potential based on their own qualities. (The rust ecosystem has a naming convention where C API is directly wrapped in a package named X-sys, and then a higher-level API package is called simply X, which offers an API more idiomatic to rust.)

Instead I prefer to interpret "low-level" conceptually according to networking and communication stacking principles. All of the "high-level" libraries mentioned in the package introduction (network-simple, connection, hookup) already have some sort of concept combining a (host, port) but they can all work just as well with a UNIX domain socket, or some imagined future "virtual socket" that does not involve ports. Pulling this common factor out seems like the natural choice, and the effort is most easily achieved starting in the one place that they already depend on, i.e. this package.

Regardless of this, I hope we can agree the other changes - deriving Show, the bind behaviour workaround - do belong in this library? For example you are already setting IPv6Only in the socket function which is a break from the C API. (I actually disagree with this specific choice, but that's an issue for another day.)

@eborden
Copy link
Collaborator

eborden commented Oct 29, 2019

@infinity0, I tend to agree with @kazu-yamamoto. Though I am not impossible to convince. For this feature to reach any level of acceptance it is going to need tests so we can verify its behavior on both linux and windows distros (maybe some day mac and bsd 🙏).

@kazu-yamamoto
Copy link
Collaborator

Instead I prefer to interpret "low-level" conceptually according to networking and communication stacking principles. All of the "high-level" libraries mentioned in the package introduction (network-simple, connection, hookup) already have some sort of concept combining a (host, port) but they can all work just as well with a UNIX domain socket, or some imagined future "virtual socket" that does not involve ports. Pulling this common factor out seems like the natural choice, and the effort is most easily achieved starting in the one place that they already depend on, i.e. this package.

If network does not provide functionality to implement your package atop it, please let us know. We will try to provide such functionality.

Regardless of this, I hope we can agree the other changes - deriving Show, the bind behaviour workaround - do belong in this library? For example you are already setting IPv6Only in the socket function which is a break from the C API. (I actually disagree with this specific choice, but that's an issue for another day.)

Yes. Separated small PRs are welcome. For bind, I would like to recommend to write test cases first to demonstrate the problem, then add code to fix it. This approach makes our review process much easier.

@kazu-yamamoto
Copy link
Collaborator

I would like to close this. Please send us smaller PRs if you wish.

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.

3 participants