twisted.web2.stream readline size argument

[mkerrin]

The twisted.web2.wsgi.InputStream objects readline method doesn't accept a size
arguement making it none file-like.
I have attached a patch for the files wsgi.py and stream.py in order to support
this. Also included are tests for this feature. My only issue with the patch is
that it is slightly complicated but then again so is twisted.web2.stream
If accepted could I please request that this bug fix be back ported to the 2.1.x
 branch. The generated the patch against the trunk but it should easily apply to
this branch.
Thanks a million,
Michael Kerrin - michael.kerrin@openapp.biz

[jknight]

Thanks for the nicely done patch complete with tests! I'd like to note, however,
that according to: http://www.python.org/peps/pep-0333.html
"""The optional "size" argument to readline() is not supported, as it may be
complex for server authors to implement, and is not often used in practice."""
So, while much of this lower level bits of this patch could be useful in any
case, I'm not sure if you really want me to add an extension to the wsgi
interface that would make applications using it non-portable?

[mkerrin]

Sorry for the delay in getting back.
I never noticed that in the specification before, I just just saw that the input
stream should be a file-like object. The reason I started looking at this is
because of the bug http://www.zope.org/Collectors/Zope3-dev/535 (traceback of
someone trying to call readline with a size argument in Zope3) but I have run
into some nasty problems along the way like inconsistencies between cStringIO
and StringIO readline (see
http://sourceforge.net/tracker/index.php?func=detail&aid=1416477&group_id=5470&atid=105470)
which makes me think that the readline method should just be deprecated, and the
wsgi people are correct. Also it seems that the person who creating the Zope bug
was using a patched version of standard cgi.py module. After thinking about it a
lot I think the cgi module from the standard python library, is not quite up to
scratch. It shouldn't use readline but the read method instead, has you can need
up reading a lot a data into memory with the readline method. But this is for an
other day.
So I agree with you that this should probable not be added to the wsgi code but
parts of the patch that apply to the stream and test_stream modules may provide
some usefull utility functionality to some one somewhere done the line. I will
leave it up to you to decide if stream / test_stream part is worth inclusion.
Thanks.

[jknight]

Damnit, used the wrong text field for my comment.

What I meant to say is: Cleaning up: reclassifying as low prio enhancement.

[mgedmin]

Apparently Python 2.4.4 will incorporate the cgi.py patch from http://sourceforge.net/tracker/index.php?func=detail&aid=1112549&group_id=5470&atid=105470 and if you now try to pass a twisted.web2.wsgi.InputStream? object to cgi.FieldStorage? in a Python 2.4.4 prerelease, you'll get an exception.

Zope 3's HTTPInputStream has a temporary workaround (since today): it accepts a 'size' argument to readline() and then ignores it, because passing it on to twisted.web2.wsgi.InputStream? would cause an error.

I am not a language lawyer, but http://docs.python.org/lib/bltin-file-objects.html says that file objects have a readline() method that takes one optional 'size' argument. The difference in the descriptions of 'readline' and 'readlines' makes me think that while readlines() is allowed to ignore its sizehint arg, readline() shouldn't do that.

[jknight]

Given that cgi.py now uses the size argument, and that nobody seems to be making a strong decision whether that's wrong or wsgi spec is, I'm going to go ahead and merge this patch, despite it being not part of the wsgi spec. Thanks.

[jknight]

This and #2166, #2170 fixed in source:branches/readline-1451, needs reviewz.

[exarkun]

Basically looks good. Just a few minor comments, in order of decreasing importance:

• what is the expected/intended behavior for readline with a size hint which falls after the \r of a newline, but before the \n? could you add a test for this case?
• existence of size parameter to the wsgi.InputStream? readline method merits a brief comment at least, since it isn't part of the wsgi spec. wsgi.InputStream?'s readline, readlines, and iter could all do with docstrings as well.
• test_readInputLineSize and test_readInputLineSizeNegZero both use iadd which is technically against the coding standard.
• Can you reformat the multiline test method docstrings to look like the stream.BufferedStream?.readline readline method's docstring?
• Is anything going to interpolate the message of the ValueError? raised by BufferedStream?.readline?
• Consistent whitespace around assignment would be nice :)

The new test coverage is good. I think the wsgi-specific parts could be a little more unit-y, but there is also something to be said for exercising the whole stack, and it isn't causing any noticably slowdown in this case so I suppose it's fine.

As long as at least the first three bullet points are addressed, feel free just to merge without re-review.

[foom]

(In [18460]) Fix wsgi.input's readline, readlines, and iter methods. In detail:

• Remove BufferedStream?.readline maxLength argument.
• Add size argument to wsgi.input's readline(). This is not part of the WSGI spec, but code such as python stdlib cgi.py require it anyways. (#1451)
• Add BufferedStream?.readline size argument, to enable above.

• Fix wsgi.readlines() and wsgi.readline() to not add extraneous delimiters. (#2170)
• Make BufferedStream?.readline return delimiter as part of the result, to enable above.

• Add an iter method to wsgi.input, which is required by the spec. (#2166)

• Change fileupload's use of BufferedStream?.readline to conform with above changes.

Also, add some docstrings.

Author: mkerrin, jknight Reviewer: exarkun Merges branch: readline-1451 Fixes #1451 Fixes #2166 Fixes #2170