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

tcp: adding support for SO_REUSEPORT #759

Open
wants to merge 2 commits into
base: trunk
from

Conversation

@wdauchy
Copy link

wdauchy commented Apr 1, 2017

this makes it possible to listen to the same tcp port using different
processes under linux >= v3.9

@wdauchy wdauchy force-pushed the wdauchy:9101-wdauchy-tcp_reuseport branch from a092dfe to 3854807 Apr 1, 2017
@codecov
Copy link

codecov bot commented Apr 1, 2017

Codecov Report

Merging #759 into trunk will decrease coverage by 1.1%.
The diff coverage is 64.7%.

@@            Coverage Diff             @@
##            trunk     #759      +/-   ##
==========================================
- Coverage   89.62%   88.52%   -1.11%     
==========================================
  Files         844      844              
  Lines      147851   147868      +17     
  Branches    13065    13067       +2     
==========================================
- Hits       132518   130897    -1621     
- Misses      12919    14505    +1586     
- Partials     2414     2466      +52
@wdauchy wdauchy force-pushed the wdauchy:9101-wdauchy-tcp_reuseport branch 11 times, most recently from 9e29d84 to 63b13fe Apr 1, 2017
@wdauchy
Copy link
Author

wdauchy commented Apr 2, 2017

I couldn't validate codecov tests since it requires linux >= 3.9 and not available on windows. However it does work on my local machine. In order to go through codecov, I would have to remove the test.

@hawkowl
Copy link
Member

hawkowl commented Apr 10, 2017

@wdauchy Hi! Do you have a ticket on https://twistedmatrix.com/trac that we can reference it against? When we have a ticket number, we can sync it to our buildbot builders (of which some have a kernel version greater than 3.9) so we can run it against the full test suite.

Please file a ticket, or add it to the description, and add the 'review' tag to the trac ticket with a link to this PR, then we can move this along :)

@hawkowl
Copy link
Member

hawkowl commented Apr 16, 2017

@hawkowl
Copy link
Member

hawkowl commented Apr 16, 2017

Running tests on builders...

@wdauchy
Copy link
Author

wdauchy commented Apr 19, 2017

@hawkowl thanks for the update; is there anything else I can do to help? From what I have seen, failing tests are either on windows or ubuntu 12.04 which has linux 3.2

@wdauchy
Copy link
Author

wdauchy commented Apr 27, 2017

@hawkowl ping?

@hawkowl
Copy link
Member

hawkowl commented Apr 27, 2017

@wdauchy Sorry, there was a problem on our build infra that prevented the tests from going green. Re-running...

wdauchy added a commit to criteo-forks/carbon that referenced this pull request Apr 27, 2017
create the socket on our side to use our own options

this will avoid the need to bind each carbon process to a different port
and balance the traffic with iptables rules

this requires linux >= 3.9

patches are on their way to support reuseport directly in twisted
see twisted/twisted#759 for the tcp part, udp
will fillow

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
wdauchy added a commit to criteo-forks/carbon that referenced this pull request Apr 27, 2017
create the socket on our side to use our own options

this will avoid the need to bind each carbon process to a different port
and balance the traffic with iptables rules

this requires linux >= 3.9

patches are on their way to support reuseport directly in twisted
see twisted/twisted#759 for the tcp part, udp
will fillow

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
wdauchy added a commit to criteo-forks/carbon that referenced this pull request Apr 27, 2017
create the socket on our side to use our own options

this will avoid the need to bind each carbon process to a different port
and balance the traffic with iptables rules

this requires linux >= 3.9

patches are on their way to support reuseport directly in twisted
see twisted/twisted#759 for the tcp part, udp
will fillow

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
wdauchy added a commit to criteo-forks/carbon that referenced this pull request Apr 28, 2017
create the socket on our side to use our own options

this will avoid the need to bind each carbon process to a different port
and balance the traffic with iptables rules

this requires linux >= 3.9

patches are on their way to support reuseport directly in twisted
see twisted/twisted#759 for the tcp part, udp
will fillow

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
wdauchy added a commit to criteo-forks/carbon that referenced this pull request Apr 28, 2017
create the socket on our side to use our own options

this will avoid the need to bind each carbon process to a different port
and balance the traffic with iptables rules

this requires linux >= 3.9

patches are on their way to support reuseport directly in twisted
see twisted/twisted#759 for the tcp part, udp
will fillow

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
@wdauchy
Copy link
Author

wdauchy commented Apr 28, 2017

@hawkowl how does it sound to you? is there anything I can improve?

@hawkowl
Copy link
Member

hawkowl commented Apr 29, 2017

@wdauchy I think it might be better implemented how we do the other TCP options: https://github.com/wdauchy/twisted/blob/66929c6d2843e23a6bc16deff0ae8d55e979ac2e/src/twisted/internet/tcp.py#L302 , is that possible? It would reduce the API changes a lot (which I think is a bit excessive). You needn't implement it in iocpreactor, as Windows doesn't support it, so you can just do a twisted.python.runtime.platform.isLinux() check for the tests.

@wdauchy wdauchy force-pushed the wdauchy:9101-wdauchy-tcp_reuseport branch from 66929c6 to 0dfe419 Apr 29, 2017
@wdauchy
Copy link
Author

wdauchy commented Apr 29, 2017

@hawkowl thanks for your review. I will remove the iocpreactor part as soon as possible; I indeed did not catch it was windows specific; that is understood now.
I don't think it is possible to implement it as a regular tcp option since it has to be set before binding; so it requires us to pass it as argument; do you see any other way to do it in order to avoid API changes?
for the test, I used "hasattr(socket, "SO_REUSEPORT"), which is supposed to avoid issues on both windows or mac. I can however replace it with platform.isLinux() if you prefer; I might also be wrong if those are defined on windows and mac.

@hawkowl
Copy link
Member

hawkowl commented Apr 29, 2017

@wdauchy you're right about the hasattr way, because an isLinux check isn't sufficient on older Linuxes. Right now, SO_REUSEPORT is just a Linux specific addition, I don't think it'll ever get adopted by macOS or Windows.

Let me think about the API ramifications. @glyph , you have any ideas, maybe?

@glyph
Copy link
Member

glyph commented May 1, 2017

SO_REUSEADDR on Windows actually behaves a lot like SO_REUSEPORT on Linux; however, it describes this behavior as "non-deterministic" and mostly seems to talk about it as a security hole. See here: See here: https://msdn.microsoft.com/en-us/library/windows/desktop/ms740621(v=vs.85).aspx

Someone with more time than me should do the research into how to get consistent behavior between platforms, and add an API that does that, rather than trying to make the exact flags (which mean different things anyway) apply on both platforms.

wdauchy added 2 commits Mar 31, 2017
this makes it possible to listen to the same tcp port using different
processes

available on linux >= 3.9
avoid windows test since it is not supported

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
this makes it possible to listen to the same tcp port (over ssl)
using different processes

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
@wdauchy wdauchy force-pushed the wdauchy:9101-wdauchy-tcp_reuseport branch from 0dfe419 to d84cdf9 May 2, 2017
@wdauchy
Copy link
Author

wdauchy commented May 2, 2017

@hawkowl @glyph so on windows we may set SO_REUSEADDR in case of multiListen set and SO_EXCLUSIVEADDRUSE otherwise
I don't see how to get a real consistent behavior here, since the exact reuseport functionnality is simply not supported elsewhere.

For now, I put back the IOCP interfaces, since it was breaking the endpoints interface; I did not saw a clean way to avoid it.

piotr1212 added a commit to piotr1212/carbon that referenced this pull request May 15, 2017
create the socket on our side to use our own options

this will avoid the need to bind each carbon process to a different port
and balance the traffic with iptables rules

this requires linux >= 3.9

patches are on their way to support reuseport directly in twisted
see twisted/twisted#759 for the tcp part, udp
will fillow

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
piotr1212 added a commit to piotr1212/carbon that referenced this pull request Jun 7, 2017
create the socket on our side to use our own options

this will avoid the need to bind each carbon process to a different port
and balance the traffic with iptables rules

this requires linux >= 3.9

patches are on their way to support reuseport directly in twisted
see twisted/twisted#759 for the tcp part, udp
will fillow

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
Copy link
Contributor

moshez left a comment

Thanks for working on this! I've recently had the occasion to have to shim in reuseport support into Twisted for an example, and that was pretty painful -- so it would be wonderful to have it in the core.

I've listed a few things that would be better done in a different way.

@@ -961,6 +964,8 @@ def createInternetSocket(self):
s = base.BasePort.createInternetSocket(self)
if platformType == "posix" and sys.platform != "cygwin":
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
if self.listenMultiple and hasattr(socket, "SO_REUSEPORT"):

This comment has been minimized.

Copy link
@moshez

moshez Oct 4, 2017

Contributor

This should probably fail if listenMultiple has been requested and we cannot do it, since it literally changes semantics (e.g., we can't run the program twice).

@@ -463,11 +463,15 @@ def __init__(self, reactor, port, backlog, interface):
@param interface: The hostname to bind to
@type interface: str
@param listenMultiple: allow multiple processes to listen to the same port
@type listenMultiple: bool

This comment has been minimized.

Copy link
@moshez

moshez Oct 4, 2017

Contributor

This feature is called:

  • listenMultiple (in some places)
  • multiListen (in others)
  • SO_REUSEPORT (by the kernel level documentation that someone will read and wonder "does Twisted support it)

I suggest collapsing all of these into a single name "reuseport" (lose the SO), thus keeping close to the kernel terminology and not inventing our own.

@@ -728,7 +728,7 @@ def lookupZone(name, timeout=None):

class IReactorTCP(Interface):

def listenTCP(port, factory, backlog=50, interface=''):
def listenTCP(port, factory, backlog=50, interface='', listenMultiple=False):

This comment has been minimized.

Copy link
@moshez

moshez Oct 4, 2017

Contributor

This is changing an interface to add a new optional parameter.

This is a backwards incompatible change -- since someone external to Twisted who has written their own reactor is now no longer complying with this interface.

If we want to do a backwards incompatible change, we should go by the appropriate process -- probably following the "exceptions" section in https://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html#procedure-for-exceptions-to-this-policy

An alternative is to add reuse port in a separate interface (e.g., :code:IReactorTCPListenMulti), which we would only add to the reactor if we detected SO_REUSEPORT in socket)

Also -- presumably we would like to be able to REUSEPORT on IPv6 as well?
(Either way, whether we do a backwards incompatible change or if we add a new interface,
it would be worthwhile to chunk those into one change.)

OTOH -- I'm not that sure we need the support in listenSSL -- since that functionality can easily be accomplished with the txsni plugin (and we probably want to get away from SSL having to lock-step with TCP)

@moshez
Copy link
Contributor

moshez commented Apr 10, 2018

FYI -- there has been no response on this for about 6 months. I am going to continue working on this in #994 . Once it is green on buildbot, I will close this one with a link.

@wdauchy
Copy link
Author

wdauchy commented Apr 14, 2018

@moshez sorry for the lack of reply.

@wiml
Copy link
Contributor

wiml commented Oct 29, 2018

Someone with more time than me should do the research into how to get consistent behavior between platforms, and add an API that does that, rather than trying to make the exact flags (which mean different things anyway) apply on both platforms.

FWIW, this Stack Overflow reply does an excellent job of covering different OS's various REUSE options, and how there are at least two distinct behaviors being controlled by SO_REUSEfoo sockopts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.