Sockets refactor: allow any family/type/protocol association by ysbaddaden · Pull Request #3750 · crystal-lang/crystal

ysbaddaden · 2016-12-21T23:53:42Z

The whole idea behing this refactor is to give more possibilities over sockets, for some less common scenarios (eg: DGRAM UNIX sockets) as well as cleanup the sockets implementation, by avoiding some repetition.

Refactor:

Socket is now enough to create, configure and use any kind of socket association of family, type and protocol is also possible, as long as it's supported by the underlying OS implementation;
- as a consequence the connect, bind, listen, accept, send and receive methods were moved from subclasses into Socket itself;
The TCPSocket, TCPServer, UDPSocket, UNIXSocket and UNIXServer classes are merely sugar to avoid having to deal with socket details;
UNIXSocket and UNIXServer can now be used in DGRAM type, in addition to the default STREAM type.

Features:

Addrinfo DNS resolver, that wraps results from getaddrinfo;
Socket#receive : {String, IPAddress} to receive a String (like we can send(String);
Socket#bind(Int) to bind to all available interfaces;
Socket::Server module, included by both TCPServer and UNIXServer;
SO_REUSEPORT socket option; automatically set for TCPServer —see http://stackoverflow.com/a/14388707/199791

Breaking Change:

IPAddress now automatically detects the address family, so the argument was removed (limited impact).

Examples:

sock = UNIXSocket.new(Socket::Family::DGRAM)
sock.connect("/tmp/service.sock")
sock.send(message)

sock = Socket.tcp(Socket::Family::INET6)
# set some socket options (e.g. SO_REUSEPORT)
sock.bind(80)
sock.listen

closes #3214
closes #3495
refs #3586

maxpowa · 2016-12-22T04:58:13Z

Also should close #3495, if bind is expanded to be interface/port instead of just port.

ysbaddaden · 2016-12-22T08:51:59Z

Thanks, I couldn't find this issue anymore.

asterite · 2016-12-22T11:30:46Z

src/socket.cr

+
+  property family : Family
+  property type : Type
+  property protocol : Protocol


Can these three be changed after creating a socket? If not, they should probably be getters.

You're right. I'll change that.

asterite · 2016-12-22T11:32:10Z

src/socket.cr

+  # Force opened sockets to be closed on `exec(2)`. Only for platforms that don't
+  # support `SOCK_CLOEXEC` (e.g., Darwin).
+  protected def init_close_on_exec(fd : Int32)
+    {% unless LibC.constants.includes?("SOCK_CLOEXEC".id) %}


You can also use LibC.has_constant?("SOCK_CLOEXEC") (shorter plus probably a bit faster.) Not sure it works on the old compiler without .id, so it's probably better to leave the .id for now

asterite · 2016-12-22T11:40:48Z

src/socket.cr

+  #
+  # ```
+  # sock = Socket.tcp(Socket::Family::INET6)
+  # sock.bind "localhost", 1234


Should this doc comment just be sock.bind 1234?

asterite · 2016-12-22T11:46:38Z

src/socket/ip_socket.cr

  # Returns the `IPAddress` for the local end of the IP socket.
  def local_address
-    sockaddr = uninitialized LibC::SockaddrIn6
+    sockaddr = Pointer(LibC::SockaddrIn6).malloc.as(LibC::Sockaddr*)


Why this was changed? It's an extra allocation (tiny, but still...)

I ran into some issues. I hope I can change it back. Same below.

No worries, we can take care of this later. It's just a tiny allocation and only used if you do request the local address :-)

asterite · 2016-12-22T11:46:47Z

src/socket/ip_socket.cr

  def remote_address
-    sockaddr = uninitialized LibC::SockaddrIn6
-    addrlen = LibC::SocklenT.new(sizeof(LibC::SockaddrIn6))
+    sockaddr = Pointer(LibC::SockaddrIn6).malloc.as(LibC::Sockaddr*)


Same here, why the change?

asterite · 2016-12-22T11:53:10Z

Looks really good to me! It enables some new stuff, like #3495 and the code looks much nicer, readable and organized. I just made a few comments.

asterite · 2016-12-22T12:21:58Z

src/socket/udp_socket.cr

+  def receive(max_message_size = 512) : {String, IPAddress}
+    bytes = Bytes.new(max_message_size)
+    bytes_read, sockaddr, addrlen = recvfrom(bytes)
+    {String.new(bytes.to_unsafe, bytes_read), IPAddress.from(sockaddr, addrlen)}


Here there's a double allocation: Bytes.new and String.new (it copies the bytes). You can do something like:

str = String.new(max_message_size) do |buffer| bytes = Slice.new(buffer, max_message_size) bytes_read, sockaddr, addrlen = recvfrom(bytes) # etc. {bytes_read, 0} end

The only "problem" with this is that this will maybe allocate more bytes than needed, so maybe occupy more memory until the string is GC'ed. However, I'll soon change String#new to realloc the pointer if the resulting bytesize is less than the requested capacity. I even did a few benchmarks and this makes a program go much faster as later less memory needs to be reclaimed.

ysbaddaden · 2016-12-22T22:15:17Z

@asterite I applied all your suggestions, and skipped a few more extra allocations!

I also added support for SO_REUSEPORT which is a nice feature, and applied it automatically to TCPServer in addition to SO_REUSEADDR.

asterite · 2016-12-23T00:49:08Z

@ysbaddaden Oops, my bad! It seems has_constant? can be passed a string literal, and it works fine in both the current and next versions of the compiler. So changing those has_constant?("SOME".id) to has_constant?("SOME") should make travis happy, and then I'll merge this :-)

Refactor: - Socket is now enough to create, configure and use any kind of socket association of family, type and protocol is also possible, as long as it's supported by the underlying OS implementation. - The TCPSocket, TCPServer, UDPSocket, UNIXSocket and UNIXServer classes are merely sugar to avoid having to deal with socket details. - UNIXSocket and UNIXServer can now be used in DGRAM type, in addition to the default STREAM type. Features: - Adds Socket::Server type, included by both TCPServer and UNIXServer. - Adds Addrinfo DNS resolver, that wraps results from `getaddrinfo`. Breaking Changes: - IPAddress now automatically detects the address family, so the argument was removed (limited impact).

ysbaddaden · 2016-12-23T09:55:43Z

Fixed! Let's wait for Travis :-)

asterite · 2016-12-23T11:32:38Z

Almost! Just missing crystal tool format

asterite · 2016-12-23T12:58:04Z

Ah, never mind. I'll make a release today so i'll merge this and then run the formatter.

@ysbaddaden Thank you so much for this! ❤️

ysbaddaden force-pushed the std-sockets-refactor branch from 6e6923d to acd148f Compare December 22, 2016 00:35

asterite requested changes Dec 22, 2016

View reviewed changes

asterite reviewed Dec 22, 2016

View reviewed changes

asterite approved these changes Dec 23, 2016

View reviewed changes

ysbaddaden added 3 commits December 23, 2016 10:53

Sockets refactor: avoid extra allocations

afd3b2b

sockets: add SO_REUSEPORT option

9b37ca3

ysbaddaden force-pushed the std-sockets-refactor branch from b06c05d to 9b37ca3 Compare December 23, 2016 09:55

asterite added kind:feature topic:stdlib labels Dec 23, 2016

asterite added this to the 0.20.2 milestone Dec 23, 2016

asterite merged commit d1159e5 into crystal-lang:master Dec 23, 2016

ysbaddaden deleted the std-sockets-refactor branch December 23, 2016 13:57

bew mentioned this pull request Feb 22, 2017

[cancelled, not using Crystal anymore] Add abstract unix socket handling #4056

Closed

ysbaddaden mentioned this pull request Mar 22, 2024

Refactor #system_connect without yield #14383

Merged

Uh oh!

Conversation

ysbaddaden commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxpowa commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ysbaddaden commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asterite commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysbaddaden commented Dec 22, 2016

Uh oh!

asterite commented Dec 23, 2016

Uh oh!

ysbaddaden commented Dec 23, 2016

Uh oh!

asterite commented Dec 23, 2016

Uh oh!

asterite commented Dec 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ysbaddaden commented Dec 21, 2016 •

edited

Loading

maxpowa commented Dec 22, 2016 •

edited

Loading