zope.publisher.http.HTTPInputStream Bug

[Andrew Sawyers]

2006-01-20T14:47:27 ERROR SiteError http://localhost:8080
Traceback (most recent call last):
  File "/home/andrew/rhg/rhg_dev/opt/Zope-3.2.0/lib/python/zope/publisher/publish.py", line 131, in publish
    request.processInputs()
  File "/home/andrew/rhg/rhg_dev/opt/Zope-3.2.0/lib/python/zope/publisher/browser.py", line 255, in processInputs
    fs = FieldStorage(fp=fp, environ=self._environ, keep_blank_values=1)
  File "/home/andrew/rhg/rhg_dev/opt/Python-2.4.2/lib/python2.4/cgi.py", line 526, in __init__
    self.read_multi(environ, keep_blank_values, strict_parsing)
  File "/home/andrew/rhg/rhg_dev/opt/Python-2.4.2/lib/python2.4/cgi.py", line 646, in read_multi
    environ, keep_blank_values, strict_parsing)
  File "/home/andrew/rhg/rhg_dev/opt/Python-2.4.2/lib/python2.4/cgi.py", line 528, in __init__
    self.read_single()
  File "/home/andrew/rhg/rhg_dev/opt/Python-2.4.2/lib/python2.4/cgi.py", line 661, in read_single
    self.read_lines()
  File "/home/andrew/rhg/rhg_dev/opt/Python-2.4.2/lib/python2.4/cgi.py", line 683, in read_lines
    self.read_lines_to_outerboundary()
  File "/home/andrew/rhg/rhg_dev/opt/Python-2.4.2/lib/python2.4/cgi.py", line 711, in read_lines_to_outerboundary
    line = self.fp.readline(1<<16)
TypeError: readline() takes exactly 1 argument (2 given)

[Paul Winkler (slinkp)]

This is related to Python bug [ 1112549 ] "cgi.FieldStorage memory usage can spike in line-oriented ops".
http://sourceforge.net/tracker/index.php?func=detail&aid=1112549&group_id=5470&atid=105470

Andrew was running a version of Python with that patch applied. I know this because he's a coworker and i have the same buildout :)

If the Python patch is ever committed (hint hint), Zope will need to be patched before we can use that Python release, so that the stream's readline() method takes a size hint. Something like the attached patch to lib/python/zope/publisher/http.py. Should be safe to patch zope even without the Python patch, it will just fail to prevent the python bug.

[Paul Winkler (slinkp)]

oops, forgot the patch and the collector's not letting me upload now.

Originally I had size=-1 but that caused some publisher test failures, no time to research that now; this version seems OK.

 *** http.py.origorig Fri Feb 17 18:16:40 2006
 --- http.py Fri Feb 17 19:12:22 2006
 ***************
 *** 198,205 ****
           self.cacheStream.write(data)
           return data

 ! def readline(self):
 ! data = self.stream.readline()
           self.cacheStream.write(data)
           return data

 --- 198,205 ----
           self.cacheStream.write(data)
           return data

 ! def readline(self, size=None):
 ! data = self.stream.readline(size)
           self.cacheStream.write(data)
           return data

[Stephan Richter (srichter)]

Changes: submitter email, edited transcript, importance (medium => critical)

[Fred Drake (fdrake)]

The referenced patch to Python has been accepted and applied for both Python 2.5 and 2.4.4.

I've not looked into why using a -1 as the default size fails. If the Python documentation is correct, accepting a negative value for size is required.

[Marius Gedminas (mgedmin)]

I now have Python 2.4.4c0 (from Ubuntu Edgy), and this bug bites me.

Would anyone object if I committed the fix to the 3.2 branch?

[Marius Gedminas (mgedmin)]

Status: Pending => Resolved

I've applied the patch to the trunk, the 3.3 branch, and the 3.2 branch. In all three branches the patch fixed over 20 test failures in the 'zope' package.

[Marius Gedminas (mgedmin)]

Status: Resolved => Pending

The patch broke one test (in zope.app.twisted.tests.test_inputbuffering) when ran on Python 2.4.3, according to http://buildbot.zope.org/.

Investigating.

[Marius Gedminas (mgedmin)]

OK, the problem is that twisted.web2.wsgi.InputStream.readline doesn't accept a size hint. I didn't notice this because I ran the tests at level 1, and this test is level 2.

The docs (http://docs.python.org/lib/bltin-file-objects.html) seem to say that file-like objects should accept an optional 'size' argument to the 'readline' method, and the cgi module in Python 2.4.4 relies on this. Is this then a bug in twisted? Can we work around it somehow?

I suppose HTTPInputStream could accept and ignore the 'size' argument for 'readline'. The Python documentation allows this behavior for readlines(), but doesn't allow this for readline(). Thoughs?

[Marius Gedminas (mgedmin)]

Ok, I'm going to change HTTPInputStream.readline to accept and discard the size argument. This will sidestep the Denial-of-Service avoidance that cgi.py does in Python 2.4.4+, but it'll work with twisted.wsgi.

The Twisted bug is http://twistedmatrix.com/trac/ticket/1451. When it is fixed, we can go and fix HTTPInputStream.readline completely.

[Marius Gedminas (mgedmin)]

Status: Pending => Deferred

[Christian Theune (ctheune)]

Status: Deferred => Pending

Reopening this issue, as the bug in the twisted collector is marked as fixed.

[Christian Theune (ctheune)]

Changes: edited transcript, topic (community => core), importance (critical => 3.3 Release), classification (issue => bug), revised version_info

[Christophe Combelles (ccomb)]

The current code in http.py is:

    def readline(self, size=None):
        # XXX We should pass the ``size`` argument to self.stream.readline
        # but twisted.web2.wsgi.InputStream.readline() doesn't accept it.
        # See http://www.zope.org/Collectors/Zope3-dev/535 and
        # http://twistedmatrix.com/trac/ticket/1451
        data = self.stream.readline()
        self.cacheStream.write(data)
        return data

Twisted is fixed and now accept the "size" argument. Should we pass it now?

[Stephan Richter (srichter)]

On Friday 15 August 2008, Christophe Combelles wrote:
> Twisted is fixed and now accept the "size" argument. Should we pass it
> now?

Yes. :-)

Regards,
Stephan
--
Stephan Richter
Web Software Design, Development and Training
Google me. "Zope Stephan Richter"

[Christophe Combelles (ccomb)]

Commited in rev 89888
twisted 2.5.0 is svn:external'ed from zope.app.twisted 3.4.x and does support the size argument.

[Marius Gedminas (mgedmin)]

The change in rev 89888 causes these errors for me:

  File "/home/mg/tmp/buildout-eggs/zope.publisher-3.4.4-py2.5.egg/zope/publisher/http.py", line 218, in readline
    data = self.stream.readline(size)
TypeError: an integer is required

self.stream is a cStringIO.StringIO. Its readline method does not like size=None at all. You need to either pass an integer, or not pass the argument at all.

Reopening bug.

[Marius Gedminas (mgedmin)]

Fix committed in revs 90357 and 90358, will be released in zope.publisher 3.4.5 and 3.5.4.

[Marius Gedminas (mgedmin)]

Fix released in zope.publisher 3.4.5, but not yet in the 3.5 branch. On the other hand, there was no 3.5.x release with this bug.

Also, I'm thinking it's about time to introduce launchpad projects for the separate Zope packages.