Fix: configurable SO_REUSEPORT socket setting (disabled by default) #4046

ysbaddaden · 2017-02-17T10:17:12Z

This led to confusing behavior when starting different TCP services that default to the same default ports. They didn't fail to start, but were failing to receive some requests.

I propose here to allow the setting to be easily configured and to disable it by default. You may enable it by setting the reuse_port named argument to true. For example:

TCPServer.open(9292, reuse_port: true) { }

server = HTTP::Server.new(9292) { |ctx| }
server.listen(reuse_port: true)

~~Maybe #listen could accept the reuse_port but I think #bind is a better place. Same for #new which would require to memoize the choice until we call #bind. I favored explicitness.~~ Both HTTP::Server#bind and HTTP::Server#listen now accept the reuse_port method.

fixes #4019

ysbaddaden · 2017-02-17T10:19:17Z

I also chose to not allow to configure SO_REUSEADDR which will (almost) never be set to false, as I explained in #4019 (comment). That would only cruft the API.

ysbaddaden · 2017-02-17T10:21:05Z

src/socket/tcp_server.cr

-        self.reuse_port = true
-      rescue Errno
-      end
+      self.reuse_port = true if reuse_port


Bonus: now we can raise if the OS doesn't support SO_REUSEPORT, since it's now explicitly enabled.

RX14 · 2017-02-17T13:53:41Z

src/http/server.cr

@@ -138,8 +138,8 @@ class HTTP::Server
    end
  end

-  def bind
-    @server ||= TCPServer.new(@host, @port)
+  def bind(reuse_port = false)


Wouldn't it make more sense to have either all of host, port and reuse_port on #bind, or all in either .new or #listen. I appreciate it makes the API a bit more unwieldy but it seems to me that reuse_port option should be in the same place as port and host.

Maybe. I have no strong opinion here.

In the discussion of this issue we suggested to make it a property of HTTP::Server. What's wrong with that? That way it's used in both bind and listen, and if you stop the server and start it again the configuration is preserved.

A few things:

host and port aren't properties, why should reuse_port be?

reuse_port, as host and port, only applies before binding the socket;

stopping and restarting HTTP::Server... will that happen? I'm not sure this is even possible (as of now).

Maybe an argument to #new would be better, just like TCPServer#new.

But we have like 8 constructors, it will clutter the API.

For me, having it as a property or as named arguments in bind/listen works well too. But I'd probably add it to both listen and bind, because it's not very common to do bind + listen, just listen.

Allright. Do we need the property is we add the argument to both listen and bind? Just say "yes" or "no" :-)

This led to confusing behavior when starting different TCP services that default to the same default ports. They didn't fail to start, but were failing to receive some requests. The setting is now configurable and disabled by default. You may enable it by setting the `reuse_port` named argument to true in the following methods: TCPServer.new TCPServer.open HTTP::Server#bind HTTP::Server#listen

ysbaddaden · 2017-02-20T15:00:02Z

Updated so HTTP::Server#listen now also takes a reuse_port argument.

ysbaddaden commented Feb 17, 2017

View reviewed changes

RX14 reviewed Feb 17, 2017

View reviewed changes

ysbaddaden force-pushed the fix-tcp-server-reuse-port branch from 099f387 to 8c2e073 Compare February 20, 2017 10:19

ysbaddaden force-pushed the fix-tcp-server-reuse-port branch from 8c2e073 to a2b3209 Compare February 20, 2017 14:56

asterite added kind:feature topic:stdlib labels Feb 20, 2017

asterite added this to the 0.21.0 milestone Feb 20, 2017

asterite merged commit b29a846 into crystal-lang:master Feb 20, 2017

ysbaddaden deleted the fix-tcp-server-reuse-port branch February 20, 2017 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: configurable SO_REUSEPORT socket setting (disabled by default) #4046

Fix: configurable SO_REUSEPORT socket setting (disabled by default) #4046

ysbaddaden commented Feb 17, 2017 •

edited

Loading

ysbaddaden commented Feb 17, 2017

ysbaddaden Feb 17, 2017

RX14 Feb 17, 2017

ysbaddaden Feb 17, 2017

asterite Feb 17, 2017

ysbaddaden Feb 17, 2017

asterite Feb 17, 2017

ysbaddaden Feb 17, 2017

asterite Feb 17, 2017

ysbaddaden commented Feb 20, 2017

Fix: configurable SO_REUSEPORT socket setting (disabled by default) #4046

Fix: configurable SO_REUSEPORT socket setting (disabled by default) #4046

Conversation

ysbaddaden commented Feb 17, 2017 • edited Loading

ysbaddaden commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden commented Feb 20, 2017

ysbaddaden commented Feb 17, 2017 •

edited

Loading