Hi Joseph,

Sorry to bother you but I bisected the code and it looks like the 
commit rev5671:f60c44563c3a makes image file uploads and large (967 
works, 1303 fails) messages fail when running under python2:

====
changeset:   5671:f60c44563c3a
user:        Joseph Myers <jsm@polyomino.org.uk>
date:        Sun Mar 24 21:49:17 2019 +0000
files:       roundup/cgi/client.py
description:
Adjust make_file override to use binary files only when needed.

Previous version would produce errors for large text fields in form
submissions with Python 3, so inherit from the default make_file
implementation and only force binary files in the specific case
(self.length >= 0) identified in <https://bugs.python.org/issue27777>.
====

So it looks like this error is now happening under python2 post commit.

Running python2 demo.py nuke and attaching a file or adding a largish 
message while creating a new issue (or adding to an existing issue) 
causes the following crash:

EXCEPTION AT Fri Jun  7 01:26:07 2019
Traceback (most recent call last):
  File "roundup.hg/roundup/scripts/roundup_server.py", line 222, in 
run_cgi
    self.inner_run_cgi()
  File "roundup.hg/roundup/scripts/roundup_server.py", line 459, in 
inner_run_cgi
    tracker.Client(tracker, self, env).main()
  File "roundup.hg/roundup/cgi/client.py", line 395, in __init__
    self.form = BinaryFieldStorage(fp=request.rfile, environ=env)
  File "/usr/lib/python2.7/cgi.py", line 507, in __init__
    self.read_multi(environ, keep_blank_values, strict_parsing)
  File "/usr/lib/python2.7/cgi.py", line 636, in read_multi
    environ, keep_blank_values, strict_parsing)
  File "/usr/lib/python2.7/cgi.py", line 509, in __init__
    self.read_single()
  File "/usr/lib/python2.7/cgi.py", line 646, in read_single
    self.read_lines()
  File "/usr/lib/python2.7/cgi.py", line 668, in read_lines
    self.read_lines_to_outerboundary()
  File "/usr/lib/python2.7/cgi.py", line 728, in 
read_lines_to_outerboundary
    self.__write(odelim + line)
  File "/usr/lib/python2.7/cgi.py", line 675, in __write
    self.file = self.make_file('')
  File "roundup.hg/roundup/cgi/client.py", line 246, in make_file
    return super().make_file()
TypeError: super() takes at least 1 argument (0 given)

Not sure what to do here. Should it be super().make_file(self)?

At the very least it looks like we have no test for this code path.

Ideas?

Joseph this crash is happening on the commit you did to handle:

Previous version would produce errors for large text fields in form
submissions with Python 3,

See the initial email in this issue for details.

I had hoped assigning it to you would have put you on the nosy list and
got the initial message sent to you, but it looks like it didn't
work as I intended.

I'm guessing this is an issue to do with super() in old-style classes with 
Python 2.  Try adding "or sys.version_info[0] < 3" to the condition for 
using tempfile.TemporaryFile directly (the inheritance is only really 
relevant with Python 3)?

Hi Joseph:

 In message <alpine.DEB.2.21.1906070159130.3830@digraph.polyomino.org.uk>,
Joseph Myers writes:
>I'm guessing this is an issue to do with super() in old-style classes with 
>Python 2.  Try adding "or sys.version_info[0] < 3" to the condition for 
>using tempfile.TemporaryFile directly (the inheritance is only really 
>relevant with Python 3)?

I'll give that a shot and see if it works. Do you have any idea about
how to generate a test case that will call super()?

Have a great weekend.

I don't know the right way to add tests (of large messages / uploads) that 
go through this code and so demonstrate both this issue and the previous 
one with Python 3.

Hi Joseph:

Yup it looks like you are correct its' a super mispmatch between
py2 and py3.

The form of the super() call isn't python 2 compatible.
Also it looks like the cgi.FieldStorage class under python2
is an old style class. So Ezio's suggestion of changing
it to:

   python2 you need to use super(SubClass, self), on python3
   super() is enough, but super(SubClass, self) is also accepted

doesn't work either as super(BinaryFieldStorage, self) throws:

   File "roundup/cgi/client.py", line 246, in make_file
      return super(BinaryFieldStorage,self).make_file()
   TypeError: super() argument 1 must be type, not classobj

Your suggestion to check python version does work, but it seems
icky.

Since there is only single inheritance on the class, I changed it to use:

  class BinaryFieldStorage(cgi.FieldStorage):
    def make_file(self, mode=None):
        import tempfile
        if self.length >= 0:
            return tempfile.TemporaryFile("wb+")
        return cgi.FieldStorage().make_file()

This seems to work in manual testing with both p3 and p2.
Do you know of any reason it wouldn't work?

Also there are a number of places where super(Subclass,self) is
being called. Do you know if any of those were touched
with the python 3 patches? If so they need to be audited and
invoked by tests to make sure we don't have old style class issues.
Unless codecov already reports they are covered.

Also I guess somebody has to figure out how to test this patch
and the other super calls too.
Sigh.

-- rouilj

I don't think cgi.FieldStorage().make_file() is right; the point of using 
super() is that make_file (in Python 3's cgi module) checks 
self._binary_file, which means it needs to be run on the *correct* 
FieldStorage object - with the correct _binary_file value - not on a newly 
constructed object that might have the wrong _binary_file value.

Tested all the classes passed to super() with issubclass(..., object)
and all classes return true. So that's good.

I have committed rev5775:17e110426ad7 preserving the super call.

The diff is:

diff -r 765f8c0e99ef -r 17e110426ad7 roundup/cgi/client.py
--- a/roundup/cgi/client.py     Fri Jun 07 21:53:55 2019 -0400
+++ b/roundup/cgi/client.py     Sat Jun 08 21:10:39 2019 -0400
@@ -230,7 +230,8 @@
         if set_cookie:
             self.client.add_cookie(self.cookie_name, self._sid, 
expire=expire)
 
-class BinaryFieldStorage(cgi.FieldStorage):
+# import from object as well so it's a new style object and I can use 
super()
+class BinaryFieldStorage(cgi.FieldStorage, object):
     '''This class works around the bug https://bugs.python.org/issue27777.
 
        cgi.FieldStorage must save all data as binary/bytes. This is
@@ -243,7 +244,7 @@
         import tempfile
         if self.length >= 0:
             return tempfile.TemporaryFile("wb+")
-        return super().make_file()
+        return super(BinaryFieldStorage, self).make_file()
 
 class Client:
     """Instantiate to handle one CGI request.

I arrived at this thanks to modifying an example from Ezio.
I have attached the modified example as test.py.

There were three ways to handle this:

  1) make sure super is never called on python 2 using the 'or ...'
     clause Joseph initially suggested. Workable but seemed like it
     wasn't in the python spirit.

  2) Use my suggested rewrite to call the base class directly. Joseph
     was correct that it shouldn't be:

         return cgi.FieldStorage().make_file()

     Ezio produced an earlier version of test.py that showed the 
     call should be:

                 return cgi.FieldStorage().make_file(self)

    which addresses Joseph's issue if I were to go this way.

  3) The patch above which is also shown in test.py makes
     BinaryFieldStorage inherit from object as well which makes
     super() when called using python2 and python 3 compatible
     semantics as super(BaseFieldStorage, self).... work.

test.py when run under py2 and py3 produces:

10
50
10
50

showing that super works as expected in this case.

I still can't figure out how to trigger this test case by calling
Client. But I am able to trigger both code paths manually using binary
and text data.

Comments?

-- rouilj

I would like to get a test on this case, but I have struggled and failed to find something that hits the second code path. Closing as it is not
a blocker anymore.