apport-hackers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/apport/py3k_exc_reraise_1024836 into lp:apport.

Requested reviews:
  Apport upstream developers (apport-hackers)
Related bugs:
  Bug #1024836 in Apport: "apport-gtk crashed with TypeError in exc_raise(): __init__() takes exactly 6 arguments (2 given)"
  https://bugs.launchpad.net/apport/+bug/1024836

For more details, see:
https://code.launchpad.net/~gz/apport/py3k_exc_reraise_1024836/+merge/116827

Branch fixes the reraising of exceptions from REThread on Python 3.

This is caused by a bad translation in 2to3 that's confusing everyone writing compatibility code.

The rule used is:
<http://docs.python.org/library/2to3.html#2to3fixer-raise>

This is correct for (uncommon) method of raising a new exception by passing a single argument separately from the class:

    raise ValueError, "Bad value", tb
 ->
    raise ValueError("Bad value).with_traceback(tb)

But does the wrong thing in the general case:

    raise OSError, (13, "Permission denied"), tb
 ->
    raise OSError((13, "Permission denied")).with_traceback(tb) # misses star

In the context of reraising exceptions, this suggested translation is even worse. The return on sys.exc_info() on Python 3 is reliably (exception_type, exception_instance, traceback). So, the translation attempts to construct a new exception with the existing instance given as the argument, this is not a form of copying exception objects generally expect:

    raise ValueError, ValueError("Bad value"), tb
 ->
    raise ValueError(ValueError("Bad value")).with_traceback(tb)

And causes a knock-on failure such as reported in the bug when an exception instance actually checks or cares about what arguments it receives.

The correct form for this case, where we know the second argument is an exception instance, is to just raise that with the traceback.
-- 
https://code.launchpad.net/~gz/apport/py3k_exc_reraise_1024836/+merge/116827
Your team Apport upstream developers is requested to review the proposed merge of lp:~gz/apport/py3k_exc_reraise_1024836 into lp:apport.

=== modified file 'apport/REThread.py'
--- apport/REThread.py	2012-07-12 15:11:48 +0000
+++ apport/REThread.py	2012-07-26 10:37:20 +0000
@@ -61,6 +61,6 @@
             # hack using exec() here
             # Python 3:
             if sys.version > '3':
-                raise self._exception[0](self._exception[1]).with_traceback(self._exception[2])
+                raise self._exception[1].with_traceback(self._exception[2])
             else:
                 exec('raise self._exception[0], self._exception[1], self._exception[2]')

=== modified file 'test/test_rethread.py'
--- test/test_rethread.py	2012-07-12 15:11:48 +0000
+++ test/test_rethread.py	2012-07-26 10:37:20 +0000
@@ -71,4 +71,18 @@
             self.assertTrue(exc[-2].endswith('return x / y\n'))
         self.assertTrue(raised)
 
-unittest.main()
+    def test_exc_raise_complex(self):
+        '''Exceptions that can't be simply created are reraised correctly
+
+        A unicode error takes several arguments on construction, so trying to
+        recreate it by just passing an instance to the class, as the Python 3
+        reraise expression did, will fail. See lp:1024836 for details.
+        '''
+        t = apport.REThread.REThread(target=str.encode, args=("\xff", "ascii"))
+        t.start()
+        t.join()
+        self.assertRaises(UnicodeError, t.exc_raise)
+
+
+if __name__ == "__main__":
+    unittest.main()