Ticket #5694 enhancement closed fixed

Opened 12 months ago

Last modified 12 months ago

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
Author: ashfall Launchpad Bug:

Description

Narrowing down #4470 for servers.

Change History

1

Changed 12 months ago by ashfall

  • branch set to branches/ipv6-tcp-server-endpoint-5694
  • branch_author set to ashfall

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

2

Changed 12 months ago by ashfall

  • owner ashfall deleted
  • keywords review added

3

Changed 12 months ago by exarkun

  • owner set to ashfall
  • keywords review removed

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.

4

Changed 12 months ago by ashfall

  • owner ashfall deleted
  • keywords review added

Changed code as per exarkun's review.

Filed #5699 for the string endpoint description plugin.

5

Changed 12 months 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.

6

Changed 12 months ago by ashfall

  • keywords review added
  • owner ashfall deleted

Addressed the issues raised in exarkun's review.

7

Changed 12 months ago by exarkun

8

Changed 12 months ago by ashfall

Also fixes #5692.

9

Changed 12 months 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!

10

Changed 12 months ago by ashfall

  • status changed from new to closed
  • resolution set to fixed

(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.