Opened 3 years ago

Closed 2 years ago

#5085 enhancement closed fixed (fixed)

Accept IPv6 address literals (with embedded scope ids) in IReactorTCP.connectTCP

Reported by: exarkun Owned by: glyph
Priority: high Milestone:
Component: core Keywords: ipv6
Cc: maddison@…, thijs Branch: branches/ipv6-connectTCP-5085-2
(diff, github, buildbot, log)
Author: acapnotic Launchpad Bug:

Description (last modified by thijs)

Similarly to #5084, Twisted should provide low-level support for IPv6 TCP clients. IReactorTCP.connectTCP implementations should accept IPv6 address literals and attempt to establish a connection to that IPv6 address.

All of the IPv4 features of connectTCP will remain the same. Passing an IPv4 address literal or a hostname will still attempt a connection to an IPv4 address.

The IConnector returned will return an IPv6Address (see #5084) from its getDestination method.

When a connection attempt is successful, the factory's buildProtocol method will be invoked with an IPv6Address instance giving the server's address.

The protocol will be connected to an ITCPTransport implementation which also returns IPv6Address instances from its getHost and getPeer implementations.

Once this is resolved, Twisted clients should be able to establish outgoing TCP connections to IPv6 addresses, eg ::1 or fe80::1%lo0 or 2001:6b0:e:2018::172%en3. Note that where the platform requires the scope id, connectTCP will require the scope id.

Attachments (4)

twisted-tcp6.patch (75.9 KB) - added by chjurk 3 years ago.
twisted-tcp6_v2.patch (20.6 KB) - added by chjurk 3 years ago.
failures.txt (6.4 KB) - added by exarkun 3 years ago.
test suite failures
connecttcp-ipv6-5085-1.patch (43.5 KB) - added by ragzilla 3 years ago.

Download all attachments as: .zip

Change History (60)

Changed 3 years ago by chjurk

comment:1 Changed 3 years ago by chjurk

  • Keywords review added

Attached a patch that offers the ability to connect to IPv6 hosts. It is based on the listentcp-ipv6-5084-3 branch as used in #5084. A test has been added as well, it is based on test_tcp, but uses IPv6 addresses instead.

comment:2 follow-up: Changed 3 years ago by glyph

  • Keywords review removed
  • Owner set to chjurk

Hi chjurk!

Thanks for working on ipv6. Great to have some community momentum on this feature :).

  1. It looks like you've just copied and pasted all of test_tcp, though, which is a bit problematic, as it makes the test suite twice as hard to maintain. It's hard to see because the copied module is so big, but I don't see any tests explicitly for IPv6 address functionality, Could you achieve this effect by writing a small test subclass, rather than copying and pasting an entire module?
  2. Your work in twisted.internet.address conflicts with the work already done in #5084. Can you coordinate with the author of that ticket to get something common used in both places? (Perhaps just help to get the branch already in progress on that ticket merged in?)
  3. This is a minor nitpick, but you appear to have generated a -p1 diff instead of a -p0 diff as requested by our contribution guidelines.
  4. New attributes, such as _addressType, will require new documentation - in that case, an @ivar declaration. (But again, have a look at #5084 first.)

Thanks again!

comment:3 Changed 3 years ago by exarkun

Could you achieve this effect by writing a small test subclass, rather than copying and pasting an entire module?

Beyond this, I'd add that ideally the tests for new functionality would end up in twisted.internet.test.test_tcp, written in the ReactorBuilder-style.

comment:4 in reply to: ↑ 2 Changed 3 years ago by chjurk

Replying to glyph:

Hi chjurk!

Thanks for working on ipv6. Great to have some community momentum on this feature :).

  1. It looks like you've just copied and pasted all of test_tcp, though, which is a bit problematic, as it makes the test suite twice as hard to maintain. It's hard to see because the copied module is so big, but I don't see any tests explicitly for IPv6 address functionality, Could you achieve this effect by writing a small test subclass, rather than copying and pasting an entire module?

I've added a suitable test class TCP6ClientTestsBuilder into twisted.internet.test.test_tcp (uses some of the tests provided by TCPClientTestsBuilder, but uses explicit IPv6 types).

  1. Your work in twisted.internet.address conflicts with the work already done in #5084. Can you coordinate with the author of that ticket to get something common used in both places? (Perhaps just help to get the branch already in progress on that ticket merged in?)

I am already using exarkun's listentcp-ipv6 branch as the basis for the changes I've made. Merging these changes into a new TCP-related ipv6 branch would be great.

  1. This is a minor nitpick, but you appear to have generated a -p1 diff instead of a -p0 diff as requested by our contribution guidelines.

Sorry, this is because I used git's diff command to create that patch. I'll attach a patch with proper formatting as soon as possible.

  1. New attributes, such as _addressType, will require new documentation - in that case, an @ivar declaration. (But again, have a look at #5084 first.)

I'll do that if exarkun is fine with that.

Thanks again!

Changed 3 years ago by chjurk

comment:5 Changed 3 years ago by chjurk

  • Keywords review added

Attached another patch. It also addresses some things therve pointed out in #5084. The patch requires branches/listentcp-ipv6-5084-3, as exarkun already added some of useful IPv6 stuff there.

comment:6 Changed 3 years ago by glyph

  • Owner chjurk deleted

(When submitting a ticket for review, it's best to un-assign it so that it will be picked up by whoever is free - otherwise they may think you need to do some special work to review it.)

Changed 3 years ago by exarkun

test suite failures

comment:7 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to chjurk

Thanks. I had two difficulties applying this patch. One was the funny path names in the index lines. They point at different levels of the hierarchy, so patch(1) can't apply the file. I fixed it manually without a huge amount of difficulty, but it'd be nice not to have to do in the future. :) The other difficulty was the Windows-style newlines in the patch. UNIX-style is preferred.

Apart from those issues:

  1. Some unit tests are failing. I attached a log of the failures.
  2. The _setRealAddress changes should not be necessary, and are probably even something we want to avoid. reactor.connectTCP("someipv6.host.name", ...) shouldn't enable IPv6 support. Note the 1st sentence of the 2nd paragraph of the ticket description. :)
  3. Similarly for some of the Connector changes. Only an IPv6 literal address should turn on the IPv6 behavior.
  4. in address.py, I think we want to skip everything about flow info and scope id for now. See jknight's comments on the IPv6 page on this topic.
  5. The __str__ for both IPv4Address and IPv6Address are silly. The __repr__ they inherit from _IPAddress should be sufficient anyway.
  6. The API documentation on _IPAddress will also be inherited by the children, so it doesn't need to be copied.
  7. Hmmmmm. Endpoints. I see why you made these changes, though I wasn't expecting them to be necessary for this branch. That complicates things a little. The IPv6 endpoints actually need to do a little more than they're doing now. They need to be resolving the name so they can pass the IPv6 address literal into the reactor and have it still work (after you disable all of the IPv6 hostname resolution support :).
  8. The SSL endpoints should be skipped for now though. They can be a simple addition implemented in a separate branch.
  9. In the tests, there's still a fair bit of duplication. Try factoring the tests that should be shared into a mixin, and have TCPClientTestsBuilder and TCP6ClientTestsBuilder both mix it in.
  10. I'm also not sure the test suite is complete yet. For example, I don't see a test for IConnector.getDestination in the IPv6 case. The ticket description is a pretty complete definition of all the things we want to happen, there should at least be tests for each of those things.

comment:8 Changed 3 years ago by ragzilla

  • Cc maddison@… added
  • Owner chjurk deleted

Attached connecttcp-ipv6-5085-1.patch
Generated against 33416

Hopefully addresses all concerns re lack of unit tests (cloned existing v4 tests in twisted.test.test_tcp and twisted.internet.test.test_tcp). Was able to refactor the twisted.internet.test.test_tcp tests into a mixin. Might be able to optimize that like the twisted.test.test_tcp tests however (where I altered the base v4 class to use an instance variable for the test address, then overrode in the v6 subclass)

Removed the resolve both behavior, this probably makes more sense to add in a separate enhancement once the non-blocking GAI is in and a draft-ietf-v6ops-happy-eyeballs-07 implementation can be made in Client and Connector.

Skipped SSL endpoints.

Fixed/added some API docs that were missing/double declared.

One thing I am noticing in the unit tests is that I get intermittent failures in twisted.test.test_tcp.HalfCloseTestCase.testCloseWriteCloser when I run the full test suite, but if I run the test directly it succeeds.

Changed 3 years ago by ragzilla

comment:9 Changed 3 years ago by ragzilla

  • Keywords review added

comment:10 Changed 3 years ago by exarkun

  • Priority changed from normal to high

comment:11 follow-up: Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to ragzilla

Thanks a lot for helping out with this, ragzilla. The end of this review bumped up against a time limit I had, so it may not be totally complete, however it does cover a decent number of points so I feel somewhat okay removing the "review" keyword.

  1. "%s starting on %d (<class 'twisted.internet.address.IPv4Address'>)" doesn't look like a great log message. I think there's another ticket for adding the full address to these log messages, so perhaps you should just leave these alone for now, but if you want to resolve that other ticket, I suggest a format more like "%s starting on <address>:<port>", eg "Foo starting on 127.0.0.1:80" or "Foo starting on [fe80::1234]:80".
  2. Watch out for adding or leaving trailing whitespace
  3. There appears to be no test coverage for link local address support, and there appears to be no link local address support.
  4. As far as twisted/test/test_tcp.py goes, we are trying to get rid of that entire module. The coverage provided by twisted/internet/test/ needs to be complete, so any changes to twisted/test/test_tcp.py can probably be skipped. If there is coverage that it provides that twisted/internet/test/ does not provide, twisted/internet/test/ needs to be improved.
  5. Ideally, the endpoints changes could be done separately. I see that some of the unit tests need these, so I see why you included them here. However, before we really introduce new public APIs for this functionality, the APIs need proper documentation and unit tests themselves. Since the IPv6 endpoints don't actually have any implementation right now, I suggest not adding them at all, but re-using the IPv4 endpoints which will work just fine to set up connections between IPv6 addresses, because all they do is pass on the address to listenTCP or connectTCP which will do the right thing with the rest of the changes in this branch. Mark these uses with a note and file a ticket for correcting them once the real IPv6 endpoints are added.

Thanks again!

comment:12 Changed 3 years ago by thijs

  • Cc thijs added
  • Description modified (diff)

comment:13 Changed 3 years ago by acapnotic

  • Owner changed from ragzilla to acapnotic

comment:14 follow-up: Changed 3 years ago by acapnotic

  • Branch set to ipv6-connectTCP-5085

the patch from comment 8 is now in a branch. There were a few failing tests which I've fixed.

comment:15 in reply to: ↑ 14 Changed 3 years ago by thijs

Replying to acapnotic:

the patch from comment 8 is now in a branch. There were a few failing tests which I've fixed.

So is this ready for review? Were the points from comment 11 addressed?

comment:16 Changed 3 years ago by acapnotic

No, the review points from comment 11 have not been addressed.

comment:17 Changed 3 years ago by acapnotic

  • Author set to acapnotic
  • Branch changed from ipv6-connectTCP-5085 to branches/ipv6-connectTCP-5085-2

(In [33772]) Branching to 'ipv6-connectTCP-5085-2'

comment:18 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by acapnotic

Replying to exarkun:

  1. There appears to be no test coverage for link local address support, and there appears to be no link local address support.

A link local address is just one that begins with "fe80", right? What sort of support is needed?

  1. As far as twisted/test/test_tcp.py goes, we are trying to get rid of that entire module. The coverage provided by twisted/internet/test/ needs to be complete, so any changes to twisted/test/test_tcp.py can probably be skipped. If there is coverage that it provides that twisted/internet/test/ does not provide, twisted/internet/test/ needs to be improved.

The impacted tests have been moved over to test.internet.test_tcp.

  1. Ideally, the endpoints changes could be done separately. [...]

working on that next.

comment:19 in reply to: ↑ 18 Changed 3 years ago by ragzilla

Replying to acapnotic:

Replying to exarkun:

  1. There appears to be no test coverage for link local address support, and there appears to be no link local address support.

A link local address is just one that begins with "fe80", right? What sort of support is needed?

From what I can tell full link local address support depends on #4362 - you pass the IPv6 literal with trailing interface identifier to getaddressinfo and it returns the required flow info / scope information (in the sockaddr tuple) to pass to socket.connect

comment:20 Changed 3 years ago by acapnotic

(In [33849]) Clean up the previous commit, which introduced the application of getaddrinfo
to IPv6 addresses, for the purpose of finding out which interface things with
a zone index (i.e. link local addresses) need to connect to.

Refs #5085

comment:21 Changed 3 years ago by acapnotic

I'm going to leave these notes here from IRC, but I'm not just going to commit the s/127.0.0.1/localhost/ change, because that seems to mean we've done something incompatible.

<oberstet> keturn, exarkun: to make the failing tests succeed : http://paste.pound-python.org/show/17443/

<keturn> oberstet: I am happy about passing tests but do you have any explanation as to why that works?

<oberstet> not really. here is what I found out. WriteSequenceTests._producerTest => the assert d2 is not None, "client protocolConnectionMade deferred is None?" fails
this fails since _before_ the line reactor.connectTCP() the self.client.protocolConnectionMade is stilled filled with a deferred, but right after its None
further: port.getHost() is really a IPv4 at this point.
I dont understand why A) reactor.connectTCP seems to "erase" the self.client.protocolConnectionMade to None and B) this does not happen when using "localhost" instead of "127.0.0.1"
FWIW: and it won't work with port = reactor.listenTCP(0, server, interface = "127.0.0.1") either (leaving the client connecting to 127.0.0.1 instead of localhost)

comment:22 follow-up: Changed 3 years ago by exarkun

From what I can tell full link local address support depends on #4362

We can ignore #4362 for a while. The implementation for this ticket may internally call getaddrinfo, but that doesn't mean we need a public API for applications to do the same.

I'm not just going to commit the s/127.0.0.1/localhost/ change, because that seems to mean we've done something incompatible.

Indeed, that's a very strange fix. Additionally, the tests use "127.0.0.1" for a reason. "localhost" is not so reliable.

comment:23 in reply to: ↑ 22 Changed 3 years ago by glyph

Replying to exarkun:

From what I can tell full link local address support depends on #4362

We can ignore #4362 for a while. The implementation for this ticket may internally call getaddrinfo, but that doesn't mean we need a public API for applications to do the same.

I'm not just going to commit the s/127.0.0.1/localhost/ change, because that seems to mean we've done something incompatible.

Indeed, that's a very strange fix. Additionally, the tests use "127.0.0.1" for a reason. "localhost" is not so reliable.

Specifically, localhost doesn't reliably produce an IPv4 address. This was run on the buildbot in question:

>>> pprint.pprint(socket.getaddrinfo('localhost', 0))
[(28, 2, 17, '', ('::1', 0, 0, 0)),
 (28, 1, 6, '', ('::1', 0, 0, 0)),
 (28, 5, 132, '', ('::1', 0, 0, 0)),
 (2, 2, 17, '', ('127.0.0.1', 0)),
 (2, 1, 6, '', ('127.0.0.1', 0)),
 (2, 5, 132, '', ('127.0.0.1', 0))]
>>> pprint.pprint(socket.getaddrinfo('127.0.0.1', 0))
[(2, 2, 17, '', ('127.0.0.1', 0)),
 (2, 1, 6, '', ('127.0.0.1', 0)),
 (2, 5, 132, '', ('127.0.0.1', 0))]

This is correct configuration, most machines should look vaguely the same way. I suspect that the IP->name translation is actually removing test coverage of some IPv4 code-path, not actually "fixing" anything.

comment:24 follow-up: Changed 3 years ago by acapnotic

  • Owner acapnotic deleted

With sprints over, I'm going to disown this.

I did get the endpoints stuff backed out, as mentioned earlier. Glyph discovered there were some other changes earlier in the history of this branch that have been problematic, but I'm not quite sure where that stands now. Check the commit history of the branch for more of the story. http://twistedmatrix.com/trac/log/branches/ipv6-connectTCP-5085-2

comment:25 in reply to: ↑ 24 Changed 2 years ago by glyph

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

Replying to acapnotic:

With sprints over, I'm going to disown this.

Thanks for moving the ball, acapnotic. I guess I'll take it over now.

I did get the endpoints stuff backed out, as mentioned earlier. Glyph discovered there were some other changes earlier in the history of this branch that have been problematic, but I'm not quite sure where that stands now. Check the commit history of the branch for more of the story. http://twistedmatrix.com/trac/log/branches/ipv6-connectTCP-5085-2

The current status is thus:

Tests seem to pass basically everywhere except Windows right now; see the build results (although they are definitely a work in progress). The Windows builders are skipping the IOCP/IPv6 tests, because that's not implemented yet, and are failing on some annoying log-formatting bug. I've successfully managed to refactor the relevant code to be shared between the IOCP reactor and the POSIXy reactors (eliminating yet more code from IOCP's copy of tcp.py, yay) and gotten that code working for IPv4 again. That took some doing. Now, I have to go write some Cython code to copy in some additional struct members and allow our little lpConnectEx function to honor the AF_INET6 address family. If you allow the TCP6Client/TCP6Connector tests to run now, they do time out (it's hard to write tests for this stuff which fail cleanly, with so many different events to coordinate) but they will at least quickly error first with a sensible "address family not supported" traceback.

comment:26 Changed 2 years ago by glyph

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

I believe that in the course of the branch's development, all of the review points thus far have been addressed in some form or another, including the never-actually-present-in-a-review point "it doesn't work at all on IOCP".

Here are some Build Results for the prospective reviewer to enjoy.

comment:27 Changed 2 years ago by glyph

Well, those results weren't great. But they're getting better.

The current status is that the build now passes everywhere except for earlier versions of Windows, which have neither inet_pton nor InetPton; I need to use WSAStringToAddressA instead. I fixed the infinite-looping issue (which was apparently only happening with _oldtls) by choosing an alternate attribute name for the new IOCP/POSIX refactoring of _base, and I fixed the epoll issue by making sure that the socket cleanup and reactor removal happened in the right order.

Also, all of this IOCP work was sort-of already outlined in #5116, so this branch will also fix that ticket.

comment:28 Changed 2 years ago by glyph

The branch is now using WSAStringToAddressA. Looks like it basically works now.

comment:29 Changed 2 years ago by exarkun

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

comment:30 follow-up: Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to glyph
  • Status changed from assigned to new
  1. twisted/internet/test/connectionmixins.py
    1. There's some trailing whitespace. There's also some funny whitespace around the : in some lambda expressions.
    2. Some of the new code is not indented consistently with the rest of the codebase
    3. Why the changes in ConnectionTestsMixin.test_logPrefix? Because of synchronously-firing Deferreds? That's kinda subtle. Merits a comment, probably. Same comment for test_protocolGarbageAfterLostConnection
    4. TCPClientTestsMixin should document the instance attributes it requires
      1. interface
      2. port
      3. family
      4. addressClass
      5. targetAddress
    5. Probably also document it's to be mixed in to a ReactorBuilder subclass
    6. FakeResolver was new-style before; now it's classic.
    7. I'm not sure what the skip at the top of TCPClientTestsMixin.test_addresses is about
  2. twisted/internet/test/test_tcp.py
    1. TCPClientTestsBuilder.serverEndpoint and TCPClientTestsBuilder.ServerEndpoint is some unfortunate naming. Same for client version of names.
    2. TCPClientTestsBuilder looks like another mixin to me. I think you should move ReactorBuilder down a level in the inheritance.
    3. Same comment about TCPConnectorTestsBuilder
    4. TCP6ClientTestsBuilder has an XXX in it. File a ticket, link to it?
    5. Just to point it out, cleaning up the reactor in ReactorBuilder tests is somewhat pointless. Trial won't complain if you don't (so you never know if you did it right, anyway). And nothing will leak, either globally or into another test method, if you don't.
    6. WriteSequenceTests._producerTest uses assert. It really oughtn't.
  3. twisted/internet/tcp.py
    1. _BaseBaseClient._requiresNoResolution is documented twice. Also, I think it and code using it would be clearer if it were not negated.
    2. In the docstring for _BaseBaseClient.resolveAddress, a reference to BaseClient should be a reference to _BaseBaseClient, I think.
    3. _BaseBaseClient.failIfNotConnected calls _collectSocketDetails. Only BaseClient implements the method, though. This seems like an undesirable separation.
    4. socket.AI_NUMERICHOST is cool, and I expect we want to use it. However, no tests fail if I replace it with 0. Seems like we want a test verifying that what we want to happen here is really happening. Faking getaddrinfo does not sound cool, I am not suggesting that. Perhaps a test should pass a name somewhere that would cause socket.AI_NUMERICHOST to kick in and refuse to resolve something? Or is there no way an application is supposed to be able to get a name in here? That seems like a possibility to me, since calls to _resolveIPv6 are all guarded by isIPv6Address. Or maybe I missed something.
  4. twisted/internet/iocpreactor/iocpsupport/iocpsupport.pyx
    1. I think we prefer raise Foo(bar) over raise Foo, bar in all cases.
    2. These functions _could_ have docstrings, right? Or is that illegal in Cython?
  5. I didn't get to twisted/internet/iocpreactor/tcp.py, but a skim reveals a promising number of deletions.

I'm not done digesting this. Fitting the gymnastics of twisted/internet/ into ones head takes a while. :/ I think there are enough interesting review points here to merit calling this a review, though. I promise I'll take another look when you're ready to submit for review again.

comment:31 Changed 2 years ago by glyph

(In [33986]) Fix whitespace; trailing whitespace, weird whitespace within lambdas, wrapping
of docstrings.

refs #5085 point 1.1.

comment:32 Changed 2 years ago by glyph

(In [33987]) Fix all the weird indentation I can find.

refs #5085 point 1.2.

comment:33 Changed 2 years ago by glyph

(In [33988]) Make a new function, needsRunningReactor, that just calls callWhenRunning but has a big expository docstring explaining why the tests need such a thing.

refs #5085 comment 1.3.

comment:34 Changed 2 years ago by glyph

(In [33989]) Document interface attribute for TCPClientTestsMixin.

refs #5085 review point 1.4.1

comment:35 Changed 2 years ago by glyph

(In [33990]) Document TCPClientTestsMixin.port

refs #5085 review point 1.4.2

comment:36 Changed 2 years ago by glyph

(In [33991]) Document TCPClientTestsMixin.family.

re #5085 review point 1.4.3

comment:37 Changed 2 years ago by glyph

(In [33992]) Document TCPClientTestsMixin.addressClass.

re #5085 review point 1.4.4

comment:38 Changed 2 years ago by glyph

(In [33994]) Remove TCPClientTestsMixin.targetAddress since apparently there's no reason for it. (Could have sworn there was, but it's always just set the same as .interface.)

refs #5085 review point 1.4.5

comment:39 Changed 2 years ago by glyph

(In [33995]) Document that TCPClientTestsMixin needs to be a ReactorBuilder test.

refs #5085 review point 1.5

comment:40 Changed 2 years ago by glyph

(In [33997]) Make FakeResolver new-style, like it used to be.

refs #5085 review point 1.6

comment:41 Changed 2 years ago by glyph

(In [33998]) Rather than skipping test_addresses on IPv6, explain the test a bit better and work around the lack of IPv6 address resolution by forcing the IPv6 version of the tests to just use an address literal.

refs #5085 review point 1.7

comment:42 Changed 2 years ago by glyph

(In [33999]) Re-name ServerEndpoint and ClientEndpoint attributes to be coding-standard compliant; and, while we're at it, document them.

refs #5085 review point 2.1.

comment:43 Changed 2 years ago by glyph

(In [34000]) The ticket was already filed, so why don't I just link to it.

refs #5085 review point 2.4.

comment:44 Changed 2 years ago by glyph

(In [34001]) Oops. Remove some debug asserts.

refs #5085 review point 2.6.

comment:45 Changed 2 years ago by glyph

(In [34002]) Remove duplicate documentation for the _requiresNoResolution flag, and make it positive instead of negative (_requiresResolution) for ease of understanding.

refs #5085 review point 3.1.

comment:46 Changed 2 years ago by glyph

(In [34003]) Reference the appropriate class in _BaseBaseClient.resolveAddress's docstring.

refs #5085 review point 3.2.

comment:47 Changed 2 years ago by glyph

(In [34004]) Document the required _collectSocketDetails attribute on _BaseBaseClient. Note, also, that twisted.internet.iocpreactor.tcp.Client also implements this method (and in fact does something ever so slightly different), which is why it's on the base class.

refs #5085 review point 3.3

comment:48 Changed 2 years ago by glyph

(In [34005]) Add direct tests for _resolveIPv6, slightly better documentation, and make sure AI_NUMERICHOST as well as AI_NUMERICSERV are passed.

refs #5085 review point 3.4

There's no way to get a hostname down through connectTCP into _resolveIPv6, because it's all guarded by isIPv6Address. I thought for a few minutes about this exact point when I wrote that code, whether I should be passing the flag or not, whether I should be testing that it was passed, given that it's belt-and-suspenders programming, there's no way to actually get the invalid address down that far.

My original rationale for not writing any tests here is that AI_NUMERICHOST is actually asking the platform to do less. In a sane, high-level API, these concerns would be separated out into different functions, like in6_resolve_flow_and_scope and in6_resolve_srv_id and in6_resolve_hostname, and we simply wouldn't call the ones we wouldn't need; we wouldn't need tests for that. But, in this case, we have to ask the platform to please not do the thing that it is going to over-solicitously do anyway.

However, as I was considering this, I realized that that's true of testing the whole tcp.Client rigamarole, but _resolveIPv6 itself is a nice simple function that I could test directly with very little fuss, and that's what I did. And while I was doing it I realized that we should be asking the platform to do even lesss work, so I added in AI_NUMERICSERV.

So there are now direct tests for _resolveIPv6, but no tests that verify that tcp.Client uses _resolveIPv6, since (as far as I can tell) it vets all the bad input out of the way via other means. However, since _resolveIPv6 is much more specific does less than getaddrinfo I think that my original reasoning holds for that part of the test.

comment:49 in reply to: ↑ 30 Changed 2 years ago by glyph

Replying to exarkun:

some words

To address just the points which aren't already covered by commit messages above:

Regarding points 2.2/2.3: wouldn't that just be redundant? ReactorBuilder intentionally leaves makeTestCaseClasses to the caller, so we don't have to build test case classes for TCPClientTestsBuilder. And so we don't.

Regarding point 4: in both cases, I was following the local style. And frankly that code scares me a bit, I had to stare at it long enough already to make sure it was doing the right thing. Also, the local style is so messed up in those files, an incremental improvement here would stick out like a sore thumb. I'd rather file a separate ticket to improve its style and write documentation, as well as doing the work necessary to verify that changing its style or adding docstirngs aren't going to have awful, subtle semantic implications. What do you think of that?

Regarding point 5: not even file descriptors? Precisely what types of cleanup are you referring to? I want to be sure before I remove any. (Recall that I did not write most of this code.)

comment:50 Changed 2 years ago by glyph

  • Keywords review added
  • Owner changed from glyph to exarkun

New build results are available. After fighting a little with AI_NUMERICSERV on Windows, I think that everything's in good shape now. Still waiting for a bunch of builds to finish but I'm optimistic at this point, so, up for review again.

Let me know if you have any further questions or if there's any way I can expedite the review at this point :).

comment:51 Changed 2 years ago by exarkun

  • Status changed from new to assigned

comment:52 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to glyph
  • Status changed from assigned to new
  1. The C{} in which mix this in must provide all of the documented C{@ivar}s in order to seems like an odd choice. I don't think readers will care about the particulars of our markup. Maybe documented instances variables?
  2. Regarding TCPClientTestsBuilder and TCPServerTestsBuilder (review comments 2.2 and 2.3) being refactored as mixins, it seems that the code would be more regular that way - with all transport-related parameters being provided by mixins. If you think it's just as good this way, feel free to leave it alone.
  3. Regarding cleanup in ReactorBuilder-style tests (review comment 2.5), any state on the reactor will be cleaned up by the ReactorBuilder-supplied tearDown. So you don't have to loseConnection servers and clients, or stopListening ports, or disconnect connectors. Anything outside the reactor is still your responsibility, of course. Hopefully that's the answer you were looking for?
  4. Regarding local style in iocp code (review comment 4), your concerns make sense. Let's leave it for now.
  5. fillinet6addr has a comment talking about inet_pton which I guess should be changed

The rhel6 builder is MIA and has no recent or successful build of this branch. Also the documentation builder has a lot of errors that I suppose are fixed in trunk, which a merge forward would be able to confirm. Otherwise I suppose this looks good now (though I am still looking forward to replacing a lot of twisted/internet/). Merge when:

  1. buildbot seems happy
  2. Addressable points in this review are addressed
  3. Cleanup code is cleaned up, if you feel like it

comment:53 Changed 2 years ago by glyph

(In [34017]) Remove implementation-detail-ish C{@ivar} and replace it with instance variables (as well as explaining why the documentation is done that way).

refs #5085 review point 1

comment:54 Changed 2 years ago by glyph

(In [34019]) Remove invalid comment reference to inet_pton.

refs #5085 review point 5

comment:55 Changed 2 years ago by glyph

  • Status changed from new to assigned

Replying to exarkun:

The rhel6 builder is MIA and has no recent or successful build of this branch.

I've already contacted thomasvs about it a couple of times, I think the builder is MIA in general. (3 failed builds on trunk, recently.)

Also the documentation builder has a lot of errors that I suppose are fixed in trunk, which a merge forward would be able to confirm.

Something weird happened with build ordering; after a rebuild (no commits, even) the new errors are quite clearly the ones fixed by ralphm's twisted.words errors, so I don't think I'm going to bother with a merge forward.

Otherwise I suppose this looks good now (though I am still looking forward to replacing a lot of twisted/internet/).

Well, this is at least a good step in the right direction. I think next time we have to touch this stuff we might be able to smash the inheritance hierarchy of Client between IOCP & friends into something a lot more linear.

Assuming nothing untoward happens in the final force-build, I will land it.

comment:56 Changed 2 years ago by glyph

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

(In [34020]) Merge ipv6-connectTCP-5085-2.

Author: acapnotic, glyph, chjurk, ragzilla

Reviewer: exarkun

Allow Twisted to connect to IPv6 addresses, using all currently supported reactors.

IPv6 clients are supported by passing an IPv6 address literal to connectTCP. Hostname resolution is the responsibility of the caller.

This change also significantly improves the internal maintenance documentation coverage for twisted.internet.tcp and refactors more client code to be shared between the IOCP reactor and all the UNIX reactors.

Fixes: #5085
Fixes: #5116

Note: See TracTickets for help on using tickets.