Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#4771 release blocker: regression closed fixed (fixed)

jstrports and therefore buildServiceManager is broken by Twisted 10.2, and has no tests

Reported by: Glyph Owned by:
Priority: highest Milestone:
Component: words Keywords:
Cc: ralphm, sergej, Todd Pratt Branch:
Author:

Description

In addition to the theoretical concern that this is a regression, at least one piece of code actually uses this function, so it's broken by 10.2.

I think we should do a point-release to get this fix out once it's done.

Attachments (2)

test_jabberjstrports.py (480 bytes) - added by Todd Pratt 7 years ago.
twisted-ticket-4771.patch (1.7 KB) - added by Todd Pratt 7 years ago.
removed trailing space in import of _parse in jstrports.py

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by Glyph

Component: corewords
Owner: changed from Glyph to Jean-Paul Calderone

comment:2 Changed 7 years ago by Glyph

Owner: changed from Jean-Paul Calderone to Glyph

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

Some notes in case anyone wants to tackle this...

  • t.w.p.j.jstrports.parse uses the no-longer-present twisted.applications.strports._parse. This could probably be replaced with twisted,internet.endpoints._parse for now. jstrports.parse should probably be deprecated though.
  • If jstrports.parse is fixed, then jstrports.client might start to work again. Although with jstrports.parse deprecated, that would mean that jstrports.client should be deprecated as well, I think.
  • The hole this leaves in the API is one shaped like a function that takes an endpoint description (and a reactor and a factory, likely) and returns an IService which uses that factory to connect to that endpoint (ie clientFromString(...).connect(factory)). This service would parallel StreamServerEndpointService, which does exist, but which applications are expected to use directly (eg StreamServerEndpointService(serverFromString(...), factory)). So recommending a parallel, direct usage of a new StreamClientEndpointService seems appropriate.

comment:4 Changed 7 years ago by ralphm

Cc: ralphm added

comment:5 Changed 7 years ago by sergej

it also breaks PyICQ jabber transport.

comment:6 Changed 7 years ago by sergej

Cc: sergej added

comment:7 Changed 7 years ago by sergej

I fixed it by copy-pasting following code from 10.1.0 into strports.py. At least pyicq transport works now.

_OP, _STRING = range(2) def _tokenize(description):

current = ops = ':=' nextOps = {':': ':=', '=': ':'} description = iter(description) for n in description:

if n in ops:

yield _STRING, current yield _OP, n current = ops = nextOps[n]

elif n=='
':

current += description.next()

else:

current += n

yield _STRING, current

def _parse(description):

args, kw = [], {} def add(sofar):

if len(sofar)==1:

args.append(sofar[0])

else:

kw[sofar[0]] = sofar[1]

sofar = () for (type, value) in _tokenize(description):

if type is _STRING:

sofar += (value,)

elif value==':':

add(sofar) sofar = ()

add(sofar) return args, kw

comment:8 Changed 7 years ago by Glyph

There's actually a much smaller fix, since this code is still in Twisted:

  • twisted/words/protocols/jabber/jstrports.py

     
    66""" A temporary placeholder for client-capable strports, until we
    77sufficient use cases get identified """
    88
    9 from twisted.application import strports
     9from twisted.internet.endpoints import _parse
    1010
    1111def _parseTCPSSL(factory, domain, port):
    1212    """ For the moment, parse TCP or SSL connections the same """
     
    2222
    2323
    2424def parse(description, factory):
    25     args, kw = strports._parse(description)
     25    args, kw = _parse(description)
    2626    return (args[0].upper(),) + _funcs[args[0]](factory, *args[1:], **kw)
    2727
    2828def client(description, factory):

the real question here is: what should a test assert about this fix?

comment:9 Changed 7 years ago by Todd Pratt

Cc: Todd Pratt added

I propose the attached test_jabberstrports.py

Changed 7 years ago by Todd Pratt

Attachment: test_jabberjstrports.py added

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

That's not bad, as far as it goes. It would be nice to test the interesting functionality of this module as well, though.

comment:11 in reply to:  10 Changed 7 years ago by Todd Pratt

What if I add this:

def test_client(self):

from twisted.application.internet import TCPClient got = jstrports.client("tcp:DOMAIN:65535", "Factory") self.assertTrue(isinstance(got, TCPClient))

comment:12 Changed 7 years ago by Todd Pratt

Sorry:

    def test_client(self):
        from twisted.application.internet import TCPClient
        got = jstrports.client("tcp:DOMAIN:65535", "Factory")
        self.assertTrue(isinstance(got, TCPClient))

comment:13 Changed 7 years ago by Todd Pratt

Keywords: review added

comment:14 Changed 7 years ago by Michael Tharp

Keywords: review removed

There's a trailing space on the import in jstrports.py, other than that +1

Changed 7 years ago by Todd Pratt

Attachment: twisted-ticket-4771.patch added

removed trailing space in import of _parse in jstrports.py

comment:15 Changed 7 years ago by Todd Pratt

Keywords: review added

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

Keywords: review removed
Owner: changed from Glyph to Jean-Paul Calderone
Status: newassigned
  1. Copyright date should only include years in which the file actually existed, so test_jabberjstrports.py should just have 2011
  2. All modules need a docstring. The module docstring for test modules typically points out which implementation module the tests are for.
  3. All test methods need docstrings explaining what functionality or behavior is being tested.
  4. assertEquals is preferred over assertEqual
  5. assertIsInstance is preferred over assertTrue(isinstance(...))
  6. Nested imports should be avoided where possible
  7. Since jstrports.py is being updated, its copyright date should be updated
  8. Changes should be summarized in news fragments

Minor points all, so mainly consider them for future patches. I'll make the fixes and apply the patch. Thanks!

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

Resolution: fixed
Status: assignedclosed

(In [30430]) Fix jstrports parser regression by using the endpoints parser

Author: tpratt, exarkun Reviewer: exarkun Fixes: #4771

Fix a regression in 10.2 which broke the jstrports APIs by switching to a different private parser API.

comment:18 Changed 7 years ago by Glyph

Given that this was a regression in 10.2, I think we should backport the fix and do a 10.2.1. Should there be a separate ticket for that?

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

Should there be a separate ticket for that?

There is no defined workflow for tracking fix backports. How about let's say try that and then we'll see how it goes and consider basing future policy on the results?

comment:20 in reply to:  19 Changed 7 years ago by Glyph

Replying to exarkun:

Should there be a separate ticket for that?

There is no defined workflow for tracking fix backports. How about let's say try that and then we'll see how it goes and consider basing future policy on the results?

OK. #4798 and #4799 - one for the backport, one for the release.

comment:21 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.