Opened 3 years ago

Closed 9 months ago

#5086 enhancement closed fixed (fixed)

Accept IPv6 address literals in IReactorUDP.listenUDP

Reported by: exarkun Owned by: habnabit
Priority: normal Milestone:
Component: core Keywords: ipv6
Cc: satis Branch: branches/ipv6-listenUDP-5086-2
(diff, github, buildbot, log)
Author: habnabit Launchpad Bug:

Description (last modified by habnabit)

Similarly to #5084, Twisted should provide low-level support for IPv6 UDP servers. IReactorUDP.listenUDP implementations should accept IPv6 address literals and set up a UDP port bound to that address.

Nothing about how IPv4 support is provided will change - IPv4 address literals and hostnames will continue to listen only on an IPv4 address. The default will still be to listen on INADDR_ANY, which does not include an IPv6 addresses.

The resulting IListeningPort will return an IPv6Address (see #5084) from its getHost implementations.

The protocol will be connected to an IUDPTransport implementation which also returns IPv6Address instances from its getHost implementation.

When a datagram is delivered to the server, the datagramReceived method will be invoked with the address returned by socket.recvfrom (nominally a 4 tuple giving address, port, flow info, and scope id), just as is the case for IPv4 (where only address and port are present in the tuple).

The IUDPTransport.write implementation in this case will also accept a 4 tuple of this sort. Sending to IPv6 addresses in this way is supported, but only if an IPv6 address literal was passed to listenUDP in the first place.

IUDPTransport.getHost is documented as returning IPv4Address instances. This should probably change.

Once this is resolved, Twisted servers should be able to bind UDP ports on IPv6 addresses, eg ::1 or ::, as well as send datagrams to IPv6 address from such ports. Addresses that include an embedded scope id will be supported after #6647 is resolved.

Attachments (10)

fix-ipv6-literals-udp.patch (2.6 KB) - added by marto1_ 15 months ago.
fix-ipv6-literals-udp.2.patch (2.7 KB) - added by marto1_ 15 months ago.
Fixed patch, so all tests run, although twisted.scripts.test.test_tap2rpm fail sometimes.
fix-ipv6-literals-udp.3.patch (8.8 KB) - added by marto1_ 15 months ago.
fix-ipv6-literals-udp.4.patch (12.1 KB) - added by marto1_ 15 months ago.
Polished tests in twisted.test.test_udp
fix-ipv6-literals-udp.5.patch (25.7 KB) - added by marto1_ 15 months ago.
Here multicast works for v6 as far as loopback testing goes.
fix-ipv6-literals-udp.6.patch (8.9 KB) - added by marto1_ 14 months ago.
With the exception of the two FIXMEs this should work
fix-ipv6-literals-udp.7.patch (25.7 KB) - added by marto1_ 14 months ago.
fix-ipv6-literals-udp.8.patch (7.5 KB) - added by marto1_ 14 months ago.
Got the patch wrong, here is without the old stuff
twisted_udpv6.patch (21.6 KB) - added by satis 10 months ago.
patch based on ipv6-listenUDP-5086 branch and review comments
twisted_udpv6_value_error.patch (21.7 KB) - added by satis 10 months ago.
Variation of last patch where InvalidAddress now inherits from ValueError

Download all attachments as: .zip

Change History (61)

comment:1 Changed 3 years ago by thijs

  • Description modified (diff)

Changed 15 months ago by marto1_

comment:2 Changed 15 months ago by marto1_

  • Keywords review added

Changed 15 months ago by marto1_

Fixed patch, so all tests run, although twisted.scripts.test.test_tap2rpm fail sometimes.

comment:3 Changed 15 months ago by exarkun

  • Keywords review removed
  • Owner set to marto1_

Thanks for your work on this issue. Can you also write unit tests for this functionality? All changes and new code needs to have complete automated test coverage before it can be applied to trunk.

Changed 15 months ago by marto1_

comment:4 Changed 15 months ago by marto1_

  • Keywords review added
  • Owner marto1_ deleted

Changed 15 months ago by marto1_

Polished tests in twisted.test.test_udp

Changed 15 months ago by marto1_

Here multicast works for v6 as far as loopback testing goes.

comment:5 Changed 14 months ago by exarkun

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

comment:6 Changed 14 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to marto1_
  • Status changed from assigned to new

Thanks.

  1. twisted/test/test_udp.py - we're trying not to write tests in this style anymore. All new tests for reactor functionality:
    1. should be added in `twisted/internet/test/
    2. should be based on ReactorBuilder, which allows the test to be run against all reactor implementations instead of only one
  2. Please split the multicast changes out into a separate ticket
  3. The flowInfo and scopeId parts of this change look incomplete. They're also not necessary to resolve this ticket: the plan is to start off with the bare minimum necessary for IPv6, which just involves binding and connecting to IPv6 addresses. More advanced uses that involve flowInfo and scopeId can come later. I suggest removing them from this patch and contributing them as part of a separate ticket (along with documentation about what they're for and more test coverage that demonstrates the values we supply for them are correct). This should mean you won't need to make any changes to twisted/internet/address.py, I think - but I guess I'll also mention that the _bwHack changes added by this patch definitely shouldn't be added: that's backwards compatibility support code for IPv4Address. IPv6Address is not encumbered by those compatibility requirements because it never had the deprecated behavior _bwHack is in support of. Note also that if you did want to add a deprecation to IPv6Address, you'd want to make sure the warning mentions the correct API (this one still mentions IPv4Address) and have the correct version (the version the warning first appears in, which is usually the *next* released version of Twisted - that would be 13.2 at this point, since the 13.1 release process has already started - not 11.0 as the warning says).
  4. twisted/internet/abstract.py, covertIPv6ToInteger:
    1. there's a typo in the name :)
    2. also, this seems unused, so I think it doesn't need to be added to the patch
  5. in twisted/internet/udp.py
    1. avoid adding new public interfaces like setAddressFamily that are really just implementation details. If necessary, add such methods as private - eg, _setAddressFamily.
    2. It's usually a mistake to raise RuntimeError (Twisted code makes this mistake a lot). Also, this isn't a documented or tested way in which listenUDP can fail, so we probably want to avoid it. At the moment the TCP IPv6 code doesn't even try to deal with this case, so I think it's alright to follow that code's lead, but it raises an interesting question that's probably worth addressing in a future ticket.
    3. The TCP IPv6 code also avoided needing to change the resolver code in base.py by introducing the _requiresResolution flag. That might not be ideal, but I think we should either:
      1. follow that code's lead and leave base.py alone
      2. or decide that resolve is going to be smart about this and remove the workaround that's in the TCP IPv6 code, since it will no longer be necessary.
  6. Please also include a news fragment

Of 5.3.1 and 5.3.2, I'd prefer the former for this ticket, although the latter is nicer in the long term and could be done separately either before or after finishing this ticket.

Thanks again! Looking forward to the next revision of this patch.

Changed 14 months ago by marto1_

With the exception of the two FIXMEs this should work

comment:7 Changed 14 months ago by marto1_

  • Keywords review added
  • Owner marto1_ deleted

comment:8 Changed 14 months ago by marto1_

Interestingly enough this works quite fine http://pastebin.com/LJXx0LyD . In the worst of scenarios, with callLatters commented out, it just hangs, again, expected, but in the test case it just seems to 'ignore' the last two callbacks, also calling cbClientSend only once.

comment:9 Changed 14 months ago by marto1_

Forgive my ignorance, apparently you must execute self.runReactor(reactor) so the thing could actually work in test enviroment.

Changed 14 months ago by marto1_

Changed 14 months ago by marto1_

Got the patch wrong, here is without the old stuff

comment:10 Changed 14 months ago by habnabit

  • Author set to habnabit
  • Branch set to branches/ipv6-listenUDP-5086

(In [39237]) Branching to 'ipv6-listenUDP-5086'

comment:11 Changed 14 months ago by habnabit

(In [39239]) Apply patch from marto1_ (with fuzz).

Refs #5086.

comment:12 Changed 14 months ago by habnabit

(In [39240]) Fix up style; move tests to getListeningPort.

Refs #5086.

comment:13 Changed 14 months ago by habnabit

(In [39241]) Make pyflakes complain less.

Refs #5086.

comment:14 Changed 14 months ago by habnabit

Hi marto1_! I had to make some changes to make your patch cleanly apply, but it was clean at the time you submitted it. I think everything is good now, though. This will make it easier on the reviewer. I myself don't feel comfortable reviewing something that uses ReactorBuilder.

comment:15 Changed 14 months ago by habnabit

(In [39242]) Update some docstrings I noticed were missed.

Refs #5086.

comment:16 Changed 14 months ago by habnabit

What a beautiful buildbot result. It passes on windows now!

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/ipv6-listenUDP-5086

comment:17 Changed 13 months ago by exarkun

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

comment:18 Changed 13 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to habnabit
  • Status changed from assigned to new

Thanks!

  1. twisted/internet/test/test_udp.py
    1. The docstring test_getHostIPv6 could be improved. The sentence structure is a bit jumbled and the content covers more ground than the test itself actually does (there's no ports dealt with anywhere in the method).
    2. test_bindToIPv6Interface appears to be a near-exact duplicate of test_getHostIPv6.
    3. test_writeToIPv6Interface and test_connectedWriteToIPv6Interface look like they provide good test coverage of the relevant functionality. However, it looks like they'll fail in annoying ways - probably by timing out after a custom (and uncustomizable) timeout expires. I think that ReactorBuilder-style tests that gather a bunch of information while the reactor is running, then stop it, then make assertions about the gathered information a somewhat nicer. These tend to complete more quickly in more cases and provide test failures by raising exceptions rather than logging errors.
    4. I think I can guess the motivation for the explicit test for /no/ warning being logged in these two tests, but if I didn't know about that I'd definitely find these checks to be weird inclusions. What about warnings that are blamed on other functions? And what if something changes so that the current behavior of these tests triggers some /other/ warning? I think that everywhere else in the project, we content ourselves with looking at trial output and seeing that no unexpected warnings appear. That might be the way to go here as well.
  2. twisted/internet/udp.py / twisted/internet/iocpreactor/udp.py
    1. Looks like iocpreactor doesn't handle the magic "<broadcast>". Can you file a ticket for fixing this?
    2. As long as you're changing some lines directly above a raise X, y style exception, can you change those to raise X(y)?
    3. What happens to _setAddressFamily if the address family is neither AF_INET nor AF_INET6?
  3. The ticket summary and description explicitly calls for support of embedded scope ids, but I don't see that feature implemented here. It seems like it may be fine to delay this work until a later ticket, but please file that ticket and link to it from somewhere relevant (I hope it will be resolved before the next release, otherwise the discrepancy between TCP and UDP support of IPv6 will be a little annoying).

Thanks again!

comment:19 Changed 13 months ago by habnabit

  • Description modified (diff)
  • Status changed from new to assigned
  • Summary changed from Accept IPv6 address literals (with embedded scope ids) in IReactorUDP.listenUDP to Accept IPv6 address literals in IReactorUDP.listenUDP

comment:20 Changed 13 months ago by habnabit

  • Description modified (diff)

comment:21 Changed 13 months ago by habnabit

  1. Point by point:
    1. Will fix this.
    2. Will look into consolidating the two.
    3. That does sound like a better structure. What will do the timeout if not the test itself, though? It seemed like the trial test runner does not have a configurable timeout and defaults to 300 seconds.
    4. Makes sense. I'll remove the assertions.
  2. Also point by point:
    1. #6646.
    2. Done.
    3. This should instead raise a ValueError. Fixed.
  3. #6647.

comment:22 Changed 13 months ago by habnabit

(In [39345]) Address review comments: fix up the test_udp tests.

Refs #5086.

comment:23 Changed 13 months ago by habnabit

(In [39346]) Fix the old-style raise statements in iocpreactor/udp.py.

Refs #5086.

comment:24 Changed 13 months ago by habnabit

(In [39347]) Raise an error when listening on an invalid interface.

Refs #5086.

comment:25 Changed 13 months ago by habnabit

  • Keywords review added
  • Owner habnabit deleted
  • Status changed from assigned to new

comment:26 Changed 13 months ago by habnabit

https://github.com/twisted/twisted/compare/ipv6-listenUDP-5086~3...ipv6-listenUDP-5086 shows the changes made in the last iteration.

The buildbot has some failures which don't look related to these changes, though I'm slightly suspicious.

comment:27 Changed 12 months ago by rwall

  • Keywords review removed
  • Owner set to habnabit

Thanks habnabit and marto1,

This is looking good. I'm excited about being able to run an IPv6
Twisted DNS server.

Notes:

  • Merges cleanly
  • I did some functional testing as follows and it seemed to work perfectly.
  • Server:
    [richard@zorin ipv6-listenUDP-5086]$ twistd -n dns --port=10053 --interface='::1' --recursive
    2013-08-24 15:14:16+0100 [-] Log opened.
    2013-08-24 15:14:16+0100 [-] twistd 13.1.0 (/usr/bin/python 2.7.5) starting up.
    2013-08-24 15:14:16+0100 [-] reactor class: twisted.internet.epollreactor.EPollReactor.
    2013-08-24 15:14:16+0100 [-] DNSServerFactory starting on 10053
    2013-08-24 15:14:16+0100 [-] DNSDatagramProtocol starting on 10053
    2013-08-24 15:14:16+0100 [-] Starting protocol <twisted.names.dns.DNSDatagramProtocol object at 0x165e350>
    2013-08-24 15:14:20+0100 [DNSDatagramProtocol (UDP)] DNSDatagramProtocol starting on 55008
    2013-08-24 15:14:20+0100 [DNSDatagramProtocol (UDP)] Starting protocol <twisted.names.dns.DNSDatagramProtocol object at 0x165ecd0>
    2013-08-24 15:14:20+0100 [-] (UDP Port 55008 Closed)
    2013-08-24 15:14:20+0100 [-] Stopping protocol <twisted.names.dns.DNSDatagramProtocol object at 0x165ecd0>
    
  • Client
    from twisted.internet.task import react
    from twisted.names import dns
    
    def main(reactor):
        p = dns.DNSDatagramProtocol(controller=None)
        reactor.listenUDP(0, p, interface='::0')
    
        d = p.query(
            ('::1', 10053),
            [dns.Query('open.nlnet.nl', dns.AAAA)])
    
        def printResults(res):
            print 'ANS', res.answers
            print 'AUTH', res.authority
            print 'ADD', res.additional
        d.addCallback(printResults)
    
        return d
    
    react(main)
    

Points:

  1. source:branches/ipv6-listenUDP-5086/twisted/internet/iocpreactor/udp.py
    1. {{{73 raise ValueError(self.interface, 'is not an IPv4 or IPv6 address.')}}} results in the slightly unusual error message {{{ValueError: ('foo.bar', 'is not an IPv4 or IPv6 address.')}}}. Should it be a single string? I suppose it's useful to be able to access the bad address in isolation.
    2. {{{190 if not isIPAddress(addr[0]) and not isIPv6Address(addr[0]):}}} looks like its followed by a deprecation warning which is now over five years old. Perhaps now is the time to disallow hostnames - especially since the gai hostname endpoint just got merged.
      1. https://twistedmatrix.com/trac/browser/trunk/twisted/internet/iocpreactor/udp.py?annotate=blame
      2. same in posix: https://twistedmatrix.com/trac/browser/trunk/twisted/internet/udp.py?annotate=blame
    3. {{{224 raise ValueError("please pass only IP addresses, not domain names") }}} It would be useful to include the offending hostname in the error message.
    4. 282 def getHost(self):: Needs an @return and @rtype annotation.
  2. source:branches/ipv6-listenUDP-5086/twisted/internet/udp.py
    1. {{{249 @param addr: A tuple of (I{stringified dotted-quad IP address}, }}}: docstring needs updating.
    2. 268 if (not abstract.isIPAddress(addr[0]): See previous comment about removing the following deprecation warning.
    3. Does "<broadcast>" have any IPv6 equivalent?
    4. {{{299 raise ValueError("please pass only IP addresses, not domain names") }}}: consider adding the offending hostname to the error message.
    5. {{{364 Returns an L{IPv4Address} or L{IPv6Address}. }}}: Also add an @return and @rtype annotation.
  3. source:branches/ipv6-listenUDP-5086/twisted/internet/test/test_udp.py
    1. Use standard docstrings - even for nested fake classes
      1. https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1056/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
    2. {{{255 self.assertEqual(packet[1][:2], (cAddr.host, cAddr.port)) }}}: It's not clear what the slice is for. Perhaps add an explanatory comment mentioning what is being omitted.
    3. Same comment for test_connectedWriteToIPv6Interface
  4. source:branches/ipv6-listenUDP-5086/twisted/internet/interfaces.py
    1. Add full docstrings to IReactorUDP.listenUDP including info about IPv6 - modelled on listenTCP
      1. https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces.IReactorTCP.html#listenTCP
      2. Vs https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces.IReactorUDP.html#listenUDP
  5. Tested Coverage: All the changes seem to be covered, but there is quite a lot of uncovered adjacent code. Consider improving coverage of various udp write errors
    [richard@zorin ipv6-listenUDP-5086]$ coverage run --branch --source twisted.internet ./bin/trial twisted.internet.test.test_udp
    [richard@zorin ipv6-listenUDP-5086]$ coverage html
    
    1. Are there any IPv6 specific socket errors that need to be handled?
  6. Add some documentation (however terse) to https://twistedmatrix.com/documents/current/core/howto/udp.html explaining how to use the interface parameter to force IPv6.

Please answer or address the numbered points above and re-submit for
review with a link to clean build results.

Thanks.

-RichardW.

comment:28 Changed 12 months ago by habnabit

  1. Point by point.
    1. Yes, I think it's valuable to be able to index the exception to get the thing out instead of having to do string munging.
    2. Replaced with a ValueError raise.
    3. Done.
    4. Done.
  2. Point by point.
    1. Done.
    2. Also replaced with a ValueError raise.
    3. I don't believe IPv6 has the concept of a broadcast address at all.
    4. Done.
    5. Done.
  3. All done. The slice was removed as part of the regression fix discussed on IRC.
  4. Isn't this already done? https://twistedmatrix.com/trac/browser/branches/ipv6-listenUDP-5086/twisted/internet/interfaces.py#L845
  5. I think this should be ticketed separately. I can file a separate ticket unless you strongly disagree.
    1. I checked through the sendto(2) documentation for windows/linux/OS X/freebsd/openbsd, but didn't see anything IPv6-specific.
  6. Done.

comment:29 Changed 12 months ago by rwall

Those changes look good:

  1. branches/ipv6-listenUDP-5086/twisted/internet/udp.py
    1. Create a ticket about the "flow and scope ID" issue and add a link alongside your new comment.
    2. We talked about what happens if you attempt to write to an IPv4 address from an IPv6 socket (or vice versa). Should there be an extra check for this in the write method and raise a ValueError in that case? Or in the case of <broadcast> keyword being used with an IPv6 port?
    3. "please pass only IP addresses, not domain names" it would be nice all the error messages were consistent. Later there's yet another message "self.interface, 'is not an IPv4 or IPv6 address.')" and this time the offending address / host is args[0]
    4. (nit) "from which I am connecting" I think that style of documentation is frowned upon these days. (first person?) instead something like "the source address from which datagrams will be sent"
  2. branches/ipv6-listenUDP-5086/twisted/internet/test/test_udp.py
    1. (nit) "Writing to a connected IPv6 UDP socket on the loopback interface succeeds." isn't a great test docstring. Perhaps instead "An IPv6 address can be passed as the C{interface} argument to L{listenUDP}. The resulting Port accepts IPv6 datagrams."
  3. You didn't say anything about the remaining deprecation warnings in listenUDP? Do you think it's worth removing those at the same time as removing them from the write() method?

It's a +1 from me if you address or answer the points above. Might be
worth getting a second opinion from someone else though.

comment:30 Changed 12 months ago by rwall

A few more things...

comment:31 Changed 12 months ago by exarkun

raise ValueError(self.interface, 'is not an IPv4 or IPv6 address.')

Yes, I think it's valuable to be able to index the exception to get the thing out instead of having to do string munging.

Please don't introduce index-based interfaces. Always provide documented attributes, instead.

comment:32 Changed 10 months ago by satis

  • Cc satis added

I worked a bit on habnabit's branch and implemented the remarks from rwall and exarkun:

  • I replaced the ValueError with a new InvalidAddressError, which has an address attribute and an optional message. Where it made sense I removed value errors and replaced it with this one, which is in my opinion a clearer interface. However, this is not 100% backwards compatible, when a hostname is passed to listenUDP, even for v4 previously a ValueError was thrown. The other places where I introduced this are either new or were deprecation warnings before.
  • The documentation is now about sockets/ports and sending/receiving datagrams, I avoided words like connections and servers.
  • I added checks to the write methods as rwall suggested and added tests for this. You now cannot write IPv4 when the socket is IPv6 and vice-versa.
  • I took a look for remaining deprecations but the only one I can see is in the loseConnection, which is not IPv6-related.

I tested this locally and saw no failures (centos machine), but I couldn't test on windows so the IOCP changes might still have issues, though they should be an almost identical copy from the generic changes.

Changed 10 months ago by satis

patch based on ipv6-listenUDP-5086 branch and review comments

comment:33 Changed 10 months ago by satis

  • Keywords review added
  • Owner habnabit deleted

Changed 10 months ago by satis

Variation of last patch where InvalidAddress now inherits from ValueError

comment:34 Changed 10 months ago by satis

To give an alternative for the backwards incompatibility I mentioned before, I made a second patch where InvalidAddressError is derived from ValueError. I'm not 100% convinced this is semantically correct, but it won't break code that explicitly catched ValueError before.

comment:35 Changed 10 months ago by habnabit

I think that the semantics of deriving InvalidAddressError from ValueError are fine—ValueError means that there was an "inappropriate argument value", which seems to be exactly the case here.

comment:36 Changed 10 months ago by habnabit

The patch you posted appears to be based on trunk and not the ipv6-listenUDP-5086 branch. I've applied it anyway, but in the future, please submit patches against the current branch.

comment:37 Changed 10 months ago by habnabit

(In [40445]) Applying patch from satis. refs #5086

comment:38 Changed 10 months ago by habnabit

(In [40446]) Fix style issues. refs #5086

comment:40 Changed 10 months ago by rwall

  • Owner set to rwall
  • Status changed from new to assigned

Reviewing...

comment:41 Changed 10 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to habnabit
  • Status changed from assigned to new

Thanks habnabit, satis and everyone else involved in this branch.

The new consistent exception looks great except that the version in the branch
doesn't actually inherit from ValueError (see below). There are one or two other
issues and suggestions in the notes below.

Points:

  1. branches/ipv6-listenUDP-5086/twisted/internet/error.py
    1. InvalidAddressError
      1. I thought it was supposed to inherit from ValueError?
        1. Looks like the wrong patch was applied.
      2. Missing method docstrings in
        1. The @ivar docstrings should be references to docstrings in init
        2. See "It is not necessary to have a second copy" in https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto9
  1. branches/ipv6-listenUDP-5086/twisted/internet/test/test_udp.py
    1. nit C{InvalidAddressError} should be L{} ...but pydoctor doesn't look at the test docstrings so it doesn't really matter.
    2. test_writeToIPv6Interface
      1. nit The two assertions could be combined into one by putting all the expected and actual values in tuples....which would make a test failure easier to debug if it happened to fail.
      2. It's a style that exarkun encourages.
      3. Same applies to test_connectedWriteToIPv6Interface
    3. It seems a shame that the legacy IPv4 and IPv6 UDP write tests are now split.
      1. twisted.test.test_udp.UDPTestCase.test_sendPackets (and related tests) could be moved to the new ReactorBuilder testcase.
      2. It might also be an idea to write a test builder that automatically performs certain tests using both IPv4 and IPv6 (sendPacket, connectionRefused, bindError, rebind etc)
      3. Raise a ticket to consolidate the UDP tests.
    4. test_writingToIPv6OnIPv4RaisesInvalidAddressError
      1. nit It would be clearer if we specified an interface address explicitly.
      2. Should be two blank lines between these test methods.
  1. branches/ipv6-listenUDP-5086/twisted/internet/iocpreactor/udp.py
    1. Consider adding a @ivar docstring for addressFamily
    2. 191 if not isIPAddress
      1. For consistency, this should also check for '<broadcast>' like the posix UDP port
  1. branches/ipv6-listenUDP-5086/twisted/internet/udp.py
    1. # Remove the flow and scope ID from the address tuple,
      1. As noted in previous review, this comment should be marked as TODO and needs a ticket reference.
      2. The same reference should also be added to IOCP udp probably
    2. setAddressFamily
      1. It would be nice if this function could be shared with the IOCP port - it's identical.
  1. branches/ipv6-listenUDP-5086/twisted/internet/interfaces.py
    1. getHost: Consider adding an @return and @rtype

I'm keen to see this branch land and it's been through 5 rounds of review by
three reviewers. So please merge after:

  • fixing the inheritance of InvalidAddressError
  • merging forward
  • addressing or answering the numbered points above
  • and checking for clean build results.

-RichardW.

comment:42 Changed 10 months ago by habnabit

(In [40687]) Address more style issues. refs #5086

comment:43 Changed 10 months ago by habnabit

(In [40688]) Address review comments. refs #5086.

comment:44 Changed 10 months ago by habnabit

  • Branch changed from branches/ipv6-listenUDP-5086 to branches/ipv6-listenUDP-5086-2

(In [40689]) Branching to 'ipv6-listenUDP-5086-2'

comment:45 Changed 10 months ago by habnabit

(In [40690]) Merge forward. refs #5086

comment:46 Changed 10 months ago by habnabit

(In [40691]) Minor style fix. refs #5086

comment:47 Changed 10 months ago by habnabit

  1. Fixed. Not sure how I ended up applying the wrong patch. :(
  2. Point by point.
    1. Fixed.
    2. Fixed.
    3. Filed #6828.
    4. Not sure what you mean by specifying an address interface explicitly. It's already using the 'interface' keyword argument.
  3. Done. Even added the @ivar to t.i.udp.Port.
  4. Point by point.
    1. Filed #6826.
    2. It's something I've thought about, but I couldn't think of an implementation that I liked. There could be a top-level private function in t.i.udp that accepts a self parameter and gets assigned to a method in each of the class bodies?
  5. Done.

Forced a build which looks good so far.

comment:48 Changed 10 months ago by exarkun

Thanks for the work on this. I have a few more comments:

  1. Please update the doc formatting to comply with the standard being set out in #6537
  2. The __str__ for InvalidAddressError looks unnecessarily confusing. Why should this exception be formatted in this unusual way?
  3. The type information for the initializer arguments to InvalidAddressError are undocumented.
  4. The extra blank line at the beginning of *some* tests in this branch isn't dictated by the coding standard. The inconsistency is a little annoying. Personally I don't see how this blank line helps readability and would prefer not to see it.
  5. example.com is the canonical example domain (even better, perhaps, is example.invalid the canonical invalid domain). eggs.com is a real domain name that someone owns and hosts some content. I would hate for a bug in the implementation to start sending traffic there when the tests are run.
  6. It doesn't look like there's a test for the ValueError compatibility that's being provided by the new exception. Perhaps this isn't necessary... but I'm not sure.

Thanks again, all.

comment:49 follow-up: Changed 9 months ago by habnabit

Okay, I've addressed all of the points above. I'm slightly unsure about [40787], but I think it's good. If it looks fine to everyone else, I'll go ahead and merge.

comment:50 in reply to: ↑ 49 Changed 9 months ago by rwall

Replying to habnabit:

Okay, I've addressed all of the points above. I'm slightly unsure about [40787], but I think it's good. If it looks fine to everyone else, I'll go ahead and merge.

Yeah, I think those changes look fine. And it's nice to split out the subclass tests.

Maybe it would be nice to have an assertIsSubclass method, which gave a useful failure message, but that can be done another time.

I might have re-written those test docstrings as "x is a *subclass* of y" but that's a real nitpick. Who knows which is better.

Thanks habnabit. Please merge.

comment:51 Changed 9 months ago by habnabit

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

(In [40881]) Merge ipv6-listenUDP-5086-2: Accept IPv6 address literals in IReactorUDP.listenUDP

Authors: marto1_, habnabit, satis
Reviewers: exarkun, rwall
Fixes: #5086
Refs: #6646, #6647, #6826, #6828

IReactorUDP.listenUDP, IUDPTransport.write and IUDPTransport.connect now accept
IPv6 address literals.

Note: See TracTickets for help on using tickets.