Opened 11 years ago

Closed 9 months ago

#454 enhancement closed fixed (fixed)

Add UDP broadcast support

Reported by: pbridger Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: itamarst, pbridger, matth, hawkowl@…, firxen@… Branch: branches/udp-broadcast-454-3
(diff, github, buildbot, log)
Author: hawkowl, rwall Launchpad Bug:

Description


Attachments (6)

udpbroadcastunittest.patch (1.8 KB) - added by hawkowl 19 months ago.
Unit Test - UDP Broadcast over loopback
broadcastudptestcasev2.patch (2.1 KB) - added by hawkowl 14 months ago.
Unit Test - UDP Broadcast over loopback, v2
broadcastudptestcasev3.patch (1.8 KB) - added by hawkowl 14 months ago.
Unit Test - UDP Broadcast over loopback, v3
udpbroadcast.patch (1.7 KB) - added by hawkowl 14 months ago.
Implementation of set/getBroadcastAllowed and corresponding test
udpbroadcast-fix.patch (1.9 KB) - added by hawkowl 12 months ago.
Fixes to udpbroadcast.patch, for IOCP + TwistedChecker
udpbroadcast.py (3.3 KB) - added by rwall 12 months ago.
Send UDP broadcast messages for tracking a cluster of hosts

Download all attachments as: .zip

Change History (36)

comment:1 Changed 11 years ago by pbridger

<me> How can I setsockopt?
<exarkun> By violating encapsulation
<exarkun> itamar will fix it soon
<exarkun> Or we will stone him to death

comment:2 Changed 11 years ago by itamarst

My solution is to add a method to transport, getHandle(). This returns the
socket object on default reactor (might return something else on e.g. Windows
reactor), which you can then call setsockopt on or whatever. I don't want to add
a method for this because behaviour is way too OS specific to make a reasonable
abstraction for.

comment:3 Changed 11 years ago by itamarst

No comments, guess this is closed.

comment:4 Changed 4 years ago by <automation>

  • Owner itamarst deleted

comment:5 Changed 19 months ago by hawkowl

  • Cc hawkowl@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

My solution to this would be to use the existing reactor.listenUDP(...) method [rather than create a .listenBroadcast(...) to copy how Multicasts are done in .listenMulticast(...) ] and add a get/set method on the IUDPTransport that controls the relevant socket option (SO_BROADCAST). This solution seems to be consistent with ITCPTransport's socket option methods (eg. get/setTcpNoDelay) and would make the method to enable UDP broadcasting "self.transport.setUDPBroadcast(True)" or similar.

comment:6 Changed 19 months ago by exarkun

To add a bit more context, we had a discussion about ISystemHandle.getHandle and concluded that while this does make it possible to set the necessary socket option for broadcast, it doesn't make it easy. And since there is presumably just a correct way to do this on POSIX and a correct way to do it on Windows (are the ways even different?) having implementations of those ways under a portable interface in Twisted seems like a good idea (that's the idea of big chunks of Twisted, after all).

Changed 19 months ago by hawkowl

Unit Test - UDP Broadcast over loopback

comment:7 Changed 19 months ago by hawkowl

Attached a unit test into test_udp.py that uses the self.transport.socket.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) method of enabling broadcast, then broadcasts a UNIX timestamp and passes if it receives the same timestamp back. If it works, then we could implement unit tests that broadcast to lo for testing whether a potential .enableBroadcast() transport method actually works.

comment:8 Changed 19 months ago by hawkowl

  • Keywords review added

comment:9 Changed 19 months ago by hawkowl

For context: the unit test patch is for running on buildbot so that the behaviour of the test can be seen for each of the supported platforms, to make sure that this way of enabling UDP broadcast works universally before a proper .enableBroadcast() feature is implemented.

comment:10 Changed 19 months ago by exarkun

  • Owner set to exarkun
  • Status changed from reopened to new

comment:11 Changed 19 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/udp-broadcast-454

(In [38112]) Branching to 'udp-broadcast-454'

comment:12 Changed 19 months ago by exarkun

(In [38113]) Apply udpbroadcastunittest.patch

refs #454

comment:13 Changed 19 months ago by exarkun

(In [38114]) Mostly trivial cleanups, but also be sure to return the Deferred from the test method so trial runs the reactor long enough for something to happen.

refs #454

comment:14 Changed 19 months ago by exarkun

  • Author changed from exarkun to hawkowl
  • Keywords review removed
  • Owner changed from exarkun to hawkowl

Thanks for your work on this hawkowl. Sorry about the long review latency.

I checked your patch in with some changes (visible in the comment above) and forced a build on buildbot to see what happens.

Some notes for future patches:

  1. Since there's a branch, now, it's probably a good idea to make patches against that instead of trunk. This isn't strictly required, and since the code in the branch now is mostly for testing what happens on various platforms, it might make sense to just start fresh once we've learned what the results are. Either way, just make sure it's clear whether the next patch is based on trunk or on the branch.
  2. I'd strongly consider putting the tests in twisted/internet/test/test_udp.py instead, and writing them in the style demonstrated by the tests already in that module. twisted/test/test_udp.py is full of tests written in an older style that's not as useful (primarily, it can only test the UDP implementation of a single reactor per test suite run; the style used in twisted/internet/test/test_udp.py can test all of the reactors in one run).

Thanks again!

Changed 14 months ago by hawkowl

Unit Test - UDP Broadcast over loopback, v2

comment:15 Changed 14 months ago by hawkowl

  • Keywords review added

Hi, I've come back to fix this :)

I've moved the tests to where exarkun noted they should go, incorporated the fixes that were put in before, and hopefully it matches the style guide for now.

This patch is based off current trunk (just in case 13.0/13.1 changed anything), and has the same goal as the last patch, to make sure that this is a proper method of implementing the test (or if we can test it at all), and what platforms support it.

On my machine (2.7.5, Linux64), it passes on all the reactors that run tests, so hopefully buildbot gives us all green for the rest :)

comment:16 Changed 14 months ago by hawkowl

Jerith pointed out this patch was actually broken, since I totally didn't see that you had to start your own reactor. Also, he helped me fix up the style/where to put things, since it was pretty atrocious. New patch follows.

Changed 14 months ago by hawkowl

Unit Test - UDP Broadcast over loopback, v3

comment:17 Changed 14 months ago by jerith

  • Cc firxen@… added

FWIW, this test fails for me on Mac OS X 10.8 with "socket.error: [Errno 49] Can't assign requested address".

comment:18 Changed 14 months ago by hawkowl

  • Owner hawkowl deleted

I believe that's the problem we had with 10.8 with the original tests... It works here, so I'm not sure if that's a OS X thing. It also seemed to work on 10.6, with the original patch. Hmm.

comment:19 Changed 14 months ago by hawkowl

  • Keywords review removed

Okay, so, a few things learned:
a) you can't UDP broadcast to lo
b) it silently works on Linux but hard fails on OSX and Windows (as it should)
c) I shouldn't be testing this anyway, as it's doing things that's out of the context of this patch.

I'll write the feature and the unit test and post it up here when I think it's reviewable.

Changed 14 months ago by hawkowl

Implementation of set/getBroadcastAllowed and corresponding test

comment:20 Changed 14 months ago by hawkowl

  • Keywords review added

I've written the functionality for this ticket how I mentioned in my first post - wrapping something around setsockopt() to enable it. I set both functions to use/return a bool, breaking convention with get/setTcpNoDelay in tcp.py (which passes a raw 1/0 to setsockopt(), but returns a bool via operator.truth() in the get function), but I think it's much more 'expected'. There's also a test in there to check that the setting is applied.

I'm not 100% sure on the naming, or if the bool -> int stuff I do in setBroadcastAllowed() is messy, so feedback on that would be appreciated. I'm also guessing that documentation in the UDP section would be required, so I'll follow up with that if this gets through.

I have no idea how this will react on IOCP, so I guess we'll have to see what Buildbot says :)

comment:21 Changed 12 months ago by rwall

  • Author changed from hawkowl to hawkowl, rwall
  • Branch changed from branches/udp-broadcast-454 to branches/udp-broadcast-454-2

(In [40405]) Branching to 'udp-broadcast-454-2'

comment:22 Changed 12 months ago by rwall

(In [40406]) Apply udpbroadcast.patch from hawkowl. refs #454

Changed 12 months ago by hawkowl

Fixes to udpbroadcast.patch, for IOCP + TwistedChecker

comment:23 Changed 12 months ago by rwall

(In [40413]) Apply udpbroadcast-fix.patch from hawkowl. refs #454

Changed 12 months ago by rwall

Send UDP broadcast messages for tracking a cluster of hosts

comment:24 Changed 12 months ago by rwall

  • Keywords review removed
  • Owner set to hawkowl

Hi hawkowl,

This seems to work well. Here are some thoughts:

Points:

  1. When we were discussing this the other day I was assuming that the feature was reactor specific, but actually it's only the OS and Python version that matters isn't it. So:
    1. Debian: https://buildbot.twistedmatrix.com/builders/debian-easy-py2.6-epoll/builds/1847/steps/epoll/logs/stdio twisted.internet.test.test_udp.UDPServerTestsBuilder_SelectReactor.test_allowBroadcast ... [OK]
  1. Fedora17: https://buildbot.twistedmatrix.com/builders/fedora17-x86_64-py2.7/builds/1311/steps/select/logs/stdio twisted.internet.test.test_udp.UDPServerTestsBuilder_SelectReactor.test_allowBroadcast ... [OK]
  1. FreeBSD 8.2: https://buildbot.twistedmatrix.com/builders/freebsd-8.2-i386/builds/2222/steps/select/logs/stdio twisted.internet.test.test_udp.UDPServerTestsBuilder_SelectReactor.test_allowBroadcast ... [OK]
  1. Ubuntu Natty: https://buildbot.twistedmatrix.com/builders/natty64-py2.6-select/builds/3592/steps/trial/logs/stdio twisted.internet.test.test_udp.UDPServerTestsBuilder_SelectReactor.test_allowBroadcast ... [OK]
  1. RHEL6: https://buildbot.twistedmatrix.com/builders/rhel6-x86_64-py2.6/builds/1296/steps/epoll/logs/stdio twisted.internet.test.test_udp.UDPServerTestsBuilder_SelectReactor.test_allowBroadcast ... [OK]
  1. Windows7: https://buildbot.twistedmatrix.com/builders/windows7-64-py2.7/builds/3267/steps/select/logs/stdio twisted.internet.test.test_udp.UDPServerTestsBuilder_SelectReactor.test_allowBroadcast ... [OK]
  1. Windows XP: https://buildbot.twistedmatrix.com/builders/winxp32-py2.7/builds/2954/steps/select/logs/stdio twisted.internet.test.test_udp.UDPServerTestsBuilder_IOCPReactor.test_allowBroadcast ... [OK]
  1. Python3: https://buildbot.twistedmatrix.com/builders/python-3.3-tests/builds/1583/steps/shell/logs/stdio test_allowBroadcast (twisted.internet.test.test_udp.UDPServerTestsBuilder_SelectReactor) test_allowBroadcast ... ok
  1. In that case, it's probably not necessary to add the test to the reactor builder test cases.
    • Instead I suggest creating a new static test case class in test_udp
  1. How do you feel about the API? I'm not sure I like it. The implication is that we'll have to add separate methods for getting and setting every available socket option. (and duplicate the methods for the IOCP reactor).
  1. Some other tickets requiring new socket options
    1. SO_REUSEADDR: https://twistedmatrix.com/trac/ticket/1288
    2. IPV6_V6ONLY: https://twistedmatrix.com/trac/ticket/5997
  2. Maybe it would be nicer to define some flag constants which can be passed to an endpoint....but that's another ticket...and anyway...
  1. I see that this is how it's been done for various TCP options.
  1. https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces.ITCPTransport.html
  1. So we should add setBroadcastAllowed to IUDPTransport?
  1. How about enabling this option automatically if we detect a broadcast address or the "<broadcast>" keyword.
    1. https://twistedmatrix.com/trac/browser/tags/releases/twisted-13.1.0/twisted/internet/udp.py#L182
  1. Incidentally, the "<broadcast>" keyword isn't mentioned in the API docs.
  1. And if not, it would be nice to raise something more descriptive than "permission denied" if a user attempts to broadcast without having set SO_BROADCAST.
    1. https://twistedmatrix.com/trac/ticket/1363
    2. https://twistedmatrix.com/trac/ticket/465
  1. Please add some documentation explaining when UDP broadcasts are useful, how they can be enabled and a short example script.
  1. Here: https://twistedmatrix.com/documents/current/core/howto/udp.html
  1. I tested the broadcast feature with the attached script which might be a good example. (attachment:udpbroadcast.py)

That's all for now. Please address or answer the points above and resubmit for
review.

Thanks again.

-RichardW.

comment:25 Changed 9 months ago by hawkowl

Hi rwall,

Thanks for the review! Got some more time to work on this.

  1. The static test cases are deprecated - in the reactor builder test cases is the proper place for new tests.
  2. I think the API is fine - it's *an* API, rather than breaking encapsulation. The current API matches what's used in the TCP implementation, so it's somewhat consistent, even if it's not perfect. We can deprecate it later anyway.
  3. I don't think we should - adding it to IUDPTransport would break other UDP implementations, wouldn't it?
  4. Enabling it automatically is non-obvious, I don't think we should.
  5. New patch (coming soon!) fixes that.
  6. Probably not in the scope of this ticket, I think, but it would be nice.
  7. I wrote some long-form docs in the new patch (coming soon). I adapted the multiservice part and general layout from your script there, I hope you don't mind.

The patch will be in a .patch as my internet is currently pretty dodgy. It's based off current trunk.

comment:26 Changed 9 months ago by hawkowl

  • Branch changed from branches/udp-broadcast-454-2 to branches/udp-broadcast-454-3

(In [41506]) Branching to udp-broadcast-454-3.

comment:27 Changed 9 months ago by hawkowl

  • Keywords review added

Actually, my internet was good enough!

Looks green so far on the buildbots, so, putting up for review.

comment:28 Changed 9 months ago by hawkowl

  • Owner hawkowl deleted

comment:29 Changed 9 months ago by hynek

  • Keywords review removed
  • Owner set to hawkowl

Wow, a 10 years old ticket. :)

Let’s see:

  1. docs/projects/core/examples/udpbroadcast.py
    1. 7: receive
    2. 16: is the -o option really necessary? I never used it, does it actually do anything here?
    3. Why a * as prompt indicator? I usually use $ but I think something even different was used throughout Twisted.
  2. docs/projects/core/howto/index.rst
    1. I don’t think Multicast and Broadcast should be capitalized.
    2. Is the comma really necessary?
    3. maybe “…including multicast…” is better?
  3. docs/projects/core/howto/udp.rst
    1. 290: I find that sentence unclear, who or what is “it”?
    2. 296: I find it a bit awkward and the “please” superfluous. How about For a complete example of this feature, see?
  4. twisted/internet/udp.py
    1. It says “Whether the socket may broadcast” in one case and “Whether this UDP socket may broadcast” in the other; please make this consistent.

Please fix 1.*, 3.1, and 4.1; the rest is up to you. Feel free to merge afterwards if you feel confident enough. :)

comment:30 Changed 9 months ago by hawkowl

  • Resolution set to fixed
  • Status changed from new to closed

(In [41516]) Merge udp-broadcast-454-3: Add UDP broadcast support

Author: hawkowl
Reviewer: exarkun, rwall, hynek
Fixes: #454

This adds encapsulation for enabling broadcast on a UDP port.

Note: See TracTickets for help on using tickets.