Opened 9 years ago

Last modified 8 years ago

#5085 enhancement new

— at Accept IPv6 address literals (with embedded scope ids) in IReactorTCP.connectTCPVersion 12

Reported by: Jean-Paul Calderone Owned by: Matt
Priority: high Milestone:
Component: core Keywords: ipv6
Cc: Matt, Thijs Triemstra Branch:
Author:

Description (last modified by Thijs Triemstra)

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.

Change History (16)

Changed 9 years ago by chjurk

Attachment: twisted-tcp6.patch added

comment:1 Changed 9 years ago by chjurk

Keywords: review added

Attached a patch that offers the ability to connect to IPv6 hosts. It is based on the [log:branches/listentcp-ipv6-5084-3 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 Changed 9 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 9 years ago by Jean-Paul Calderone

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 9 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 9 years ago by chjurk

Attachment: twisted-tcp6_v2.patch added

comment:5 Changed 9 years ago by chjurk

Keywords: review added

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

comment:6 Changed 9 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 9 years ago by Jean-Paul Calderone

Attachment: failures.txt added

test suite failures

comment:7 Changed 9 years ago by Jean-Paul Calderone

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 8 years ago by Matt

Cc: Matt 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 8 years ago by Matt

comment:9 Changed 8 years ago by Matt

Keywords: review added

comment:10 Changed 8 years ago by Jean-Paul Calderone

Priority: normalhigh

comment:11 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Matt

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 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Description: modified (diff)
Note: See TracTickets for help on using tickets.