Opened 7 years ago

Last modified 6 years ago

#6325 task new

twisted.web.tap should use endpoint instead of deprecated strports

Reported by: Tom Prince Owned by:
Priority: normal Milestone:
Component: web Keywords: easy
Cc: jknight Branch: branches/web-tap-endpoint-6325
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

strports was deprecated in 10.2, so all uses of it in twisted need to be fixed.

Attachments (3)

6325.patch (2.2 KB) - added by eddie 6 years ago.
Directly invoke internet.StreamServerEndpointService
6325_2.patch (11.8 KB) - added by eddie 6 years ago.
6325_3.patch (9.2 KB) - added by eddie 6 years ago.
deleted --dsc

Download all attachments as: .zip

Change History (18)

comment:1 Changed 7 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 7 years ago by Thijs Triemstra

Also see #5536.

Changed 6 years ago by eddie

Attachment: 6325.patch added

Directly invoke internet.StreamServerEndpointService

comment:3 Changed 6 years ago by eddie

Keywords: review added
Owner: Tom Prince deleted

comment:4 Changed 6 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to eddie

Thanks for the patch!

  1. The help string should mention that this is using endpoints string format, so people know it's not just a TCP port, and give some examples (see twisted/mail/tap.py for a good example).
  2. The tests that test this code still use strports (twisted/web/test/test_tap.py). Also you should test that the plain port behavior that used to work continues to work. So, you should update the tests to use endpoints, and add a test for the bare port behavior. twisted/mail/test/test_options.py should provide some sample code demonstrating how to do this.
  3. All patches must include a news file (https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles). However, this is a minor enough change (I think the string formats are mostly compatible), so an empty .misc file may suffice, and can be added by whoever merges the ticket.

Please submit a new patch addressing comments 1 and 2 above.

comment:5 Changed 6 years ago by Jean-Paul Calderone

I don't think a .feature news fragment would be a bad thing. This is end-user visible functionality that's being enhanced, seems like we should advertise it.

Changed 6 years ago by eddie

Attachment: 6325_2.patch added

comment:6 Changed 6 years ago by eddie

Owner: eddie deleted

twisted.web now supports an option named --des which can add multiple endpoints according to the specified argument. Clarification: after reading twisted.mail.tap I think it's reasonable to add an optparam to support endpoint string description

option '--https --port --certificate --privkey" of twisted.web is now deprecated. Reason: --https --certificate --privkey can be replaced by --des. As to --port, it's a little tricky as --port is needed by the param --personal. However, I still marked --port as deprecated and let --personal be dependent on --des for the sake of unification.

I also added modified the test_tap.py to test things like the multiple endpoint support.

comment:7 Changed 6 years ago by eddie

Keywords: review added

comment:8 Changed 6 years ago by Itamar Turner-Trauring

The general switch to remove the HTTPS hard coded options with generic option is a good idea. I'd just call it --port, not --des.

A question for Tom, who filed the bug: you said in ticket description that strports is deprecated, but that is not the case. strports.parse is deprecated, but this module used strports.service which is _not_ deprecated.

Changed 6 years ago by eddie

Attachment: 6325_3.patch added

deleted --dsc

comment:9 Changed 6 years ago by Richard Wall

Author: rwall
Branch: branches/web-tap-endpoint-6325

(In [38134]) Branching to 'web-tap-endpoint-6325'

comment:10 Changed 6 years ago by Richard Wall

(In [38135]) apply 6325_3.patch. refs #6325

comment:11 Changed 6 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Forced build

reviewing...

comment:12 Changed 6 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to eddie
Status: assignednew

Code Review

Thanks for the latest patch introom. I like the new port options, especially that you can now configure multiple http and https ports for a single server instance.

Notes:

  • Patch applies cleanly to trunk
  • All tests pass on my fedora 18 python 2.7 x86_64 laptop

Please address or answer the following comments:

  1. Missing news file (see exarkun's comment about using a feature news file above) especially since this introduces the ability to configure multiple listening ports for twistd web.
  2. Build failures
    1. new pyflakes errors: https://buildbot.twistedmatrix.com/builders/pyflakes/builds/298/steps/pyflakes/logs/new%20pyflakes%20errors
    2. new twistedchecker errors: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/587/steps/run-twistedchecker/logs/new%20twisted checker%20errors
  3. source:branches/web-tap-endpoint-6325/twisted/web/tap.py
    1. Use example port numbers instead of the word "port" in the --https --certificate --privkey and --port help strings. See twistd mail --help for example.
    2. opt_port: I think the opt_port help may cause confusion. Users won't be familiar with the term "endpoint". I suggest "Add listening port" instead of "Add a specified endpoint".
    3. _toEndpoint: I'd have thought that the deprecation of the --port <int> option should start now (v13.1) not since v11
    4. _toEndpoint: I see you've copied this from twisted.mail.tap. Consult on #twisted-dev about moving that function to a shared module so that it can be re-used. Presumably it's going to be needed for twistd ftp, news, conch and any other plugin that currently supports --port <int>.
    5. makeService: Why are you now assigning to the private _raiseSynchronously variable? ( "svc._raiseSynchronously = True") Add a comment explaining why that is necessary or remove that change.
  4. source:branches/web-tap-endpoint-6325/twisted/web/test/test_tap.py
    1. Why are test names prefixed "dsc"?
    2. test_dscBarePort: Add a test for deprecation warnings when using deprecated --https and --certificate options (see twisted.mail.test.test_options for example).
    3. Missing test coverage
      1. --https "SSL support not installed" (https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r38135/twisted_web_tap.html#n208)
      2. _toEndpoint: https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r38135/twisted_web_tap.html#n231
  5. Consult with tom.prince and itamar on #twisted-dev about comment 8 ( ticket:6325#comment:8) and reply to that comment.

Please address or answer the numbered comments above and submit another patch (against source:branches/web-tap-endpoint-6325) for review.

Thanks again.

-RichardW.

comment:13 Changed 6 years ago by Itamar Turner-Trauring

I still think the original description of this ticket is wrong (strports.service is not deprecated), but switching to endpoints.serverFromString seems fine. So just need to address review comments 1 to 4.

comment:14 Changed 6 years ago by eddie

I will fix this sometime later.

comment:15 Changed 6 years ago by Jean-Paul Calderone

Owner: eddie deleted
Note: See TracTickets for help on using tickets.