Opened 2 years ago

Closed 2 years ago

#5694 enhancement closed fixed (fixed)

Add IPv6 TCP Server Endpoint

Reported by: ashfall Owned by: ashfall
Priority: normal Milestone:
Component: core Keywords: endpoint ipv6
Cc: Branch: branches/ipv6-tcp-server-endpoint-5694
(diff, github, buildbot, log)
Author: ashfall Launchpad Bug:

Description

Narrowing down #4470 for servers.

Change History (10)

comment:1 Changed 2 years ago by ashfall

  • Author set to ashfall
  • Branch set to branches/ipv6-tcp-server-endpoint-5694

(In [34469]) Branching to 'ipv6-tcp-server-endpoint-5694'

comment:2 Changed 2 years ago by ashfall

  • Keywords review added
  • Owner ashfall deleted

comment:3 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to ashfall

Thanks!

  1. The news fragment might as well explicitly name the new API. The goal of these news fragments is to make it easy to see what's changed in a new release of Twisted.
  2. It's unfortunate to have to document _port/port, _reactor/reactor, etc on TCP6ServerEndpoint twice. I see that the other endpoints do this as well. I think it's a mistake. Documenting only the __init__ parameters, so long as they very closely mirror the attributes, should be sufficient.
  3. Also, the _listenArgs attribute is defined but never used. This is also true for the same attribute on TCP4ServerEndpoint. And UNIXServerEndpoint documents it but doesn't define it.
  4. The only difference between TCP6ServerEndpoint and TCP4ServerEndpoint seems to be the default value for interface. Perhaps there's a way to implement this without duplicating so much code?
  5. I see a bunch of code about clients in the unit tests, but there's no client functionality being added in this branch. Perhaps this code belongs in the client branch, instead?
  6. The typical way of spelling "0:0:0:0:0:0:0:1" is "::1"
  7. We also need a string endpoint description plugin for this new server endpoint. Can you file a new ticket for that? Generally, there should be one of these for every kind of endpoint. Without them, the endpoints aren't accessible from the command line.

comment:4 Changed 2 years ago by ashfall

  • Keywords review added
  • Owner ashfall deleted

Changed code as per exarkun's review.

Filed #5699 for the string endpoint description plugin.

comment:5 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to ashfall

Thanks! A few more comments:

  1. TCP4ServerEndpoint and TCP6ServerEndpoint can both be simplified. The listen method they both implement is identical and provides exactly the same behavior as the base class's listen method. If you delete the method from both subclasses, they'll keep working the same.
  2. TCP6ServerEndpoint also doesn't need to declare it implements IStreamServerEndpoint. It inherits that declaration from the base class.
  3. The introduction of TCPServerEndpoint raises some questions. As an approach to code sharing, it's fine and does the job well. As a public interface, though, it's redundant with TCP4ServerEndpoint and TCP6ServerEndpoint. I think having both TCP4ServerEndpoint and TCP6ServerEndpoint also presents some redundancy, but that's the plan we've chosen so I guess I shouldn't be worrying about it now.
  4. You can use @type in an __init__ docstring. Please do that instead of starting @param fields with (some type).
  5. The change to test_endpointListenSuccess partly breaks the intent of the test, which is to exercise the defaults for the endpoint. Previously backlog and interface were not passed, but the test verified that the proper defaults were passed to listenTCP. Now the test exercises basically the same code path as test_endpointListenNonDefaultArgs. Fortunately the change doesn't seem necessary? The tests pass on my laptop if I revert it. Did I overlook something?
  6. The docstring for createServerEndpoint says "an L{TCP6ServerEndpoint}". Should be "a L{TCP6ServerEndpoint}", since the "L" is silent. :)
  7. twisted/test/proto_helpers.py still has this pyflakes warning: twisted/test/proto_helpers.py:415: undefined name 'UnsupportedAddressFamily'. Can you also fix that one?
  8. The news fragment is a little awkward. Move all of the name up front and it will flow a little better: twisted.internet.endpoints now provides TCP6ServerEndpoint, an IPv6 TCP server endpoint.

comment:6 Changed 2 years ago by ashfall

  • Keywords review added
  • Owner ashfall deleted

Addressed the issues raised in exarkun's review.

comment:8 Changed 2 years ago by ashfall

Also fixes #5692.

comment:9 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to ashfall

Thanks. Regarding point 3 on my last review, I think TCPServerEndpoint should be made private instead (which I wasn't clear on in my last review, sorry about that). Please rename it _TCPServerEndpoint and remove it from __all__. After that I think the branch will be fine to merge. Thanks again!

comment:10 Changed 2 years ago by ashfall

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

(In [34526]) Merge ipv6-tcp-server-endpoint: Added IPv6 TCP Server Endpoint.

Author: ashfall
Reviewer: exarkun
Fixes: #5692, #5694

This adds an IPv6 stream endpoint through twisted.internet.endpoints.TCP6ServerEndpoint.

Note: See TracTickets for help on using tickets.