zope.publisher.http.HTTPInputStream Bug

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.

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

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

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.

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?

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.

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.

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?

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.

Status: Pending => Deferred

Status: Deferred => Pending

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

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

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?

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"

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.

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.

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

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.