Created on 2019-06-07 01:38 by rouilj, last changed 2019-10-11 00:57 by rouilj.
|test.py||rouilj, 2019-06-09 01:23|
|msg6531||Author: [hidden] (rouilj)||Date: 2019-06-07 01:38|
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 <firstname.lastname@example.org> 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?
|msg6532||Author: [hidden] (rouilj)||Date: 2019-06-07 01:44|
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.
|msg6533||Author: [hidden] (joseph_myers)||Date: 2019-06-07 02:02|
I'm guessing this is an issue to do with super() in old-style classes with Python 2. Try adding "or sys.version_info < 3" to the condition for using tempfile.TemporaryFile directly (the inheritance is only really relevant with Python 3)?
|msg6534||Author: [hidden] (rouilj)||Date: 2019-06-07 11:02|
Hi Joseph: In message <alpine.DEB.email@example.com>, 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 < 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.
|msg6535||Author: [hidden] (joseph_myers)||Date: 2019-06-07 11:37|
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.
|msg6536||Author: [hidden] (rouilj)||Date: 2019-06-07 15:20|
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
|msg6537||Author: [hidden] (joseph_myers)||Date: 2019-06-07 16:41|
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.
|msg6540||Author: [hidden] (rouilj)||Date: 2019-06-07 22:02|
Tested all the classes passed to super() with issubclass(..., object) and all classes return true. So that's good.
|msg6541||Author: [hidden] (rouilj)||Date: 2019-06-09 01:23|
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
|msg6724||Author: [hidden] (rouilj)||Date: 2019-10-11 00:57|
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.
|2019-10-11 00:57:09||rouilj||set||status: open -> fixed|
messages: + msg6724
messages: + msg6541
|2019-06-07 22:02:42||rouilj||set||messages: + msg6540|
|2019-06-07 16:41:12||joseph_myers||set||messages: + msg6537|
|2019-06-07 15:20:42||rouilj||set||status: new -> open|
assignee: joseph_myers -> rouilj
messages: + msg6536
nosy: + ezio.melotti
|2019-06-07 11:37:35||joseph_myers||set||messages: + msg6535|
|2019-06-07 11:02:53||rouilj||set||messages: + msg6534|
|2019-06-07 02:02:11||joseph_myers||set||messages: + msg6533|
messages: + msg6532