Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4771 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: twistedmatrix.com@…, pupykin.s@…, ae88925@… Branch:
Author: Launchpad Bug:

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 tpratt 4 years ago.
twisted-ticket-4771.patch (1.7 KB) - added by tpratt 4 years ago.
removed trailing space in import of _parse in jstrports.py

Download all attachments as: .zip

Change History (23)

comment:1 Changed 4 years ago by glyph

  • Component changed from core to words
  • Owner changed from glyph to exarkun

comment:2 Changed 4 years ago by glyph

  • Owner changed from exarkun to glyph

comment:3 Changed 4 years ago by exarkun

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 4 years ago by ralphm

  • Cc twistedmatrix.com@… added

comment:5 Changed 4 years ago by sergej

it also breaks PyICQ jabber transport.

comment:6 Changed 4 years ago by sergej

  • Cc pupykin.s@… added

comment:7 Changed 4 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 4 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 4 years ago by tpratt

  • Cc ae88925@… added

I propose the attached test_jabberstrports.py

Changed 4 years ago by tpratt

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

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 4 years ago by tpratt

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 4 years ago by tpratt

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 4 years ago by tpratt

  • Keywords review added

comment:14 Changed 4 years ago by gxti

  • Keywords review removed

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

Changed 4 years ago by tpratt

removed trailing space in import of _parse in jstrports.py

comment:15 Changed 4 years ago by tpratt

  • Keywords review added

comment:16 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner changed from glyph to exarkun
  • Status changed from new to assigned
  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 4 years ago by exarkun

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

(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 4 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 follow-up: Changed 4 years ago by 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?

comment:20 in reply to: ↑ 19 Changed 4 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 3 years ago by <automation>

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