Opened 2 years ago

Closed 2 years ago

#5699 enhancement closed fixed (fixed)

Add a string endpoint description plugin for twisted.internet.endpoints.TCP6ServerEndpoint

Reported by: ashfall Owned by: ashfall
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/tcp6-server-string-description-plugin-5699
(diff, github, buildbot, log)
Author: ashfall Launchpad Bug:

Description

A string endpoint description plugin for this TCP server endpoint that supports IPv6 addresses (added in #5694) is needed so that the endpoint is accessible from the command-line.

Change History (9)

comment:1 Changed 2 years ago by ashfall

  • Owner set to ashfall

comment:2 Changed 2 years ago by ashfall

  • Author set to ashfall
  • Branch set to branches/tcp6-server-string-description-plugin-5699

(In [34609]) Branching to 'tcp6-server-string-description-plugin-5699'

comment:3 Changed 2 years ago by ashfall

  • Keywords review added
  • Owner ashfall deleted

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

  • Keywords review removed
  • Owner set to ashfall

Thanks! This looks pretty good. A couple things are missing though, I think:

  1. There aren't any tests that actually exercise the new parser. See, eg, _parseStreamServerTest (combined with, eg, test_parseStreamServerINET) in SystemdEndpointPluginTests. In other words, a test should call parseStreamServer with a "tcp6:" endpoint string description.
  2. Hard-coding the backlog to 50 doesn't seem necessary. I don't think the backlog argument is actually very important (almost no one ever wants to specify a value for it), but I wonder why you didn't allow it to be supplied in the string. "tcp:1234:backlog=50" works for TCP4 servers, so it's surprising for it to not work for TCP6 servers, I think. On the other hand, if we want to make an explicit, intentional decision to not support it in TCP6, and drop support for it in TCP4 eventually, that may be fine too. So, mostly I wonder why it's hard-coded instead of parameterized. (Also, it is documented as a parameter in _parseServer.)
  3. The new class is private, and generally we don't need to bother to advertise private classes in news fragments. More importantly is the end-user facing feature, which is that IPv6 TCP servers can now be created using the endpoint string description feature. That's what the news fragment should talk about (perhaps mentioning twisted.internet.endpoints.serverFromString).

Thanks again.

comment:5 in reply to: ↑ 4 Changed 2 years ago by ashfall

  • Keywords review added
  • Owner ashfall deleted

Replying to exarkun:

  1. There aren't any tests that actually exercise the new parser. See, eg, _parseStreamServerTest (combined with, eg, test_parseStreamServerINET) in SystemdEndpointPluginTests. In other words, a test should call parseStreamServer with a "tcp6:" endpoint string description.

Added a test that uses _parseStreamServerTest in TCP6ServerEndpointPluginTests, I think that should suffice.

Fixed the other issues as well.

comment:6 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to ashfall

Thanks. There's still no good test coverage for the actual format for this endpoint plugin parser. "8080" is not a TCP6 server endpoint string! It's a TCP4 server endpoint string. test_parseStreamServerPort cheats by instantiating endpoints._TCP6ServerParser instead of calling serverFromString.

comment:7 Changed 2 years ago by ashfall

  • Keywords review added
  • Owner ashfall deleted

Added the said test.

comment:8 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to ashfall

Thanks! Looks great now. Please merge. :)

comment:9 Changed 2 years ago by ashfall

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

(In [34680]) Merge tcp6-server-string-description-plugin-5699: A string description plugin for the TCP6ServerEndpoint.

Author: ashfall
Reviewer: exarkun
Fixes: #5699

Added a string endpoint description plugin for the TCP6ServerEndpoint so that it can be accessed from serverFromString.

Note: See TracTickets for help on using tickets.