classification
Title: Raise SystemError if a function returns a result with an exception set
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: vstinner Nosy List: Tim.Graham, berker.peksag, jcea, pitrou, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-03-03 11:58 by vstinner, last changed 2015-11-04 23:34 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
check_result.patch vstinner, 2015-03-03 11:59 review
check_result-2.patch vstinner, 2015-03-06 13:28 review
check_result-3.patch vstinner, 2015-03-06 13:51 review
check_result-4.patch vstinner, 2015-03-06 14:16 review
check_result-5.patch vstinner, 2015-03-06 14:53 review
Messages (41)
msg237123 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-03 11:58
Attached patch changes PyObject_Call() and PyCFunction_Call() to raise a SystemError and destroy the result (Py_DECREF) if a function returns a result with an exception set.

I also refactored PyCFunction_Call() and added "assert(!PyErr_Occurred());" at the beginning of PyCFunction_Call().

I removed duplicate checks in ceval.c.

--

I didn't check the impact on performances.

If the patch has a significant overhead: _Py_CheckFunctionResult() may be marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may be marked as unlikely using GCC __builtin_expect(), something like:

#ifdef __GNUC__
#  define Py_likely(x)       __builtin_expect((x),1)
#  define Py_unlikely(x)     __builtin_expect((x),0)
#else
#  define Py_likely(x)       x
#  define Py_unlikely(x)     x
#endif

Be careful of __builtin_expect(), misused it can make the code slower! (benchmark, benchmark, benchmark!)
msg237128 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-03 12:49
> If the patch has a significant overhead: _Py_CheckFunctionResult() may be
> marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may
> be marked as unlikely using GCC __builtin_expect(), something like:

Could you please open separate issue or even a topic on Python-Ideas for 
Py_likely/Py_unlikely? I prefer spelling _Py_LIKELY.
msg237132 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-03 13:16
2015-03-03 13:49 GMT+01:00 Serhiy Storchaka <report@bugs.python.org>:
>> If the patch has a significant overhead: _Py_CheckFunctionResult() may be
>> marked to be inlined, and PyCFunction_Call() and PyObject_Call() checks may
>> be marked as unlikely using GCC __builtin_expect(), something like:
>
> Could you please open separate issue or even a topic on Python-Ideas for
> Py_likely/Py_unlikely? I prefer spelling _Py_LIKELY.

I'm not really interested by micro-benchmark here :-) I also propose
solutions *if* my patch makes the code slower.

Since __builtin_expect() may have a negative effect on performances, I
would prefer to avoid it if we can :-)
msg237348 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 13:02
I ran http://hg.python.org/benchmarks/

Result:
---
$ python3 perf.py -r -b default ~/prog/python/default/python.orig  ~/prog/python/default/python.patched 
INFO:root:Automatically selected timer: perf_counter
(...)
Report on Linux smithers 3.18.3-201.fc21.x86_64 #1 SMP Mon Jan 19 15:59:31 UTC 2015 x86_64 x86_64
Total CPU cores: 8

### etree_parse ###
Min: 0.273882 -> 0.297294: 1.09x slower
Avg: 0.277947 -> 0.312787: 1.13x slower
Significant (t=-11.54)
Stddev: 0.00446 -> 0.02987: 6.6906x larger

### fastunpickle ###
Min: 0.510721 -> 0.526520: 1.03x slower
Avg: 0.521282 -> 0.542610: 1.04x slower
Significant (t=-8.33)
Stddev: 0.01024 -> 0.02348: 2.2931x larger

The following not significant results are hidden, use -v to show them:
2to3, django_v2, etree_generate, etree_iterparse, etree_process, fastpickle, json_dump_v2, json_load, nbody, regex_v8, tornado_http.
---

I ran again etree_parse alone and the slowdown disappeared :-p
---
The following not significant results are hidden, use -v to show them:
etree_parse.
---

Output when I ran fastunpickle alone:
---
### fastunpickle ###
Min: 0.510994 -> 0.522419: 1.02x slower
Avg: 0.524633 -> 0.536876: 1.02x slower
Significant (t=-2.69)
Stddev: 0.02983 -> 0.03436: 1.1518x larger
---

2% slower on a single test doesn't look to be significant. Raising a SystemError is more important than the minor slowdown. What do you think?
msg237349 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 13:03
I ran pybench, even if I know that it's not really revelant: overall 0.3% slowdown. But pybench doesn't look reliable: some tests are faster, which looks like noise in the benchmark.

-------------------------------------------------------------------------------
PYBENCH 2.1
-------------------------------------------------------------------------------
* using CPython 3.5.0a1+ (default:648b35f22b91+, Mar 6 2015, 13:18:57) [GCC 4.9.2 20141101 (Red Hat 4.9.2-1)]
* disabled garbage collection
* system check interval set to maximum: 2147483647
* using timer: time.perf_counter
* timer: resolution=1e-09, implementation=clock_gettime(CLOCK_MONOTONIC)

-------------------------------------------------------------------------------
Benchmark: orig.pybench
-------------------------------------------------------------------------------

    Rounds: 10
    Warp:   10
    Timer:  time.perf_counter

    Machine Details:
       Platform ID:    Linux-3.18.3-201.fc21.x86_64-x86_64-with-fedora-21-Twenty_One
       Processor:      x86_64
    
    Python:
       Implementation: CPython
       Executable:     /home/haypo/prog/python/default/python.orig
       Version:        3.5.0a1+
       Compiler:       GCC 4.9.2 20141101 (Red Hat 4.9.2-1)
       Bits:           64bit
       Build:          Mar  6 2015 13:19:47 (#default:648b35f22b91)
       Unicode:        UCS4


-------------------------------------------------------------------------------
Comparing with: patch.pybench
-------------------------------------------------------------------------------

    Rounds: 10
    Warp:   10
    Timer:  time.perf_counter

    Machine Details:
       Platform ID:    Linux-3.18.3-201.fc21.x86_64-x86_64-with-fedora-21-Twenty_One
       Processor:      x86_64
    
    Python:
       Implementation: CPython
       Executable:     /home/haypo/prog/python/default/python.patched
       Version:        3.5.0a1+
       Compiler:       GCC 4.9.2 20141101 (Red Hat 4.9.2-1)
       Bits:           64bit
       Build:          Mar  6 2015 13:18:57 (#default:648b35f22b91+)
       Unicode:        UCS4


Test                             minimum run-time        average  run-time
                                 this    other   diff    this    other   diff
-------------------------------------------------------------------------------
          BuiltinFunctionCalls:    41ms    41ms   -1.1%    41ms    41ms   -1.5%
           BuiltinMethodLookup:    24ms    24ms   +0.3%    24ms    25ms   -0.8%
                 CompareFloats:    26ms    26ms   +2.2%    27ms    26ms   +1.2%
         CompareFloatsIntegers:    59ms    60ms   -0.9%    60ms    61ms   -2.5%
               CompareIntegers:    38ms    36ms   +4.6%    38ms    37ms   +3.5%
        CompareInternedStrings:    26ms    25ms   +0.6%    26ms    26ms   -0.5%
                  CompareLongs:    22ms    21ms   +1.1%    22ms    22ms   +0.0%
                CompareStrings:    22ms    23ms   -1.3%    23ms    23ms   -1.3%
    ComplexPythonFunctionCalls:    40ms    40ms   -1.5%    40ms    41ms   -2.5%
                 ConcatStrings:    28ms    28ms   -0.3%    28ms    29ms   -1.2%
               CreateInstances:    41ms    42ms   -1.7%    42ms    43ms   -2.6%
            CreateNewInstances:    31ms    32ms   -2.2%    32ms    33ms   -3.2%
       CreateStringsWithConcat:    59ms    56ms   +5.6%    60ms    58ms   +2.7%
                  DictCreation:    43ms    48ms  -10.3%    44ms    50ms  -12.7%
             DictWithFloatKeys:    36ms    37ms   -4.8%    36ms    38ms   -6.0%
           DictWithIntegerKeys:    28ms    29ms   -4.1%    29ms    31ms   -6.4%
            DictWithStringKeys:    24ms    26ms   -8.9%    24ms    27ms  -10.3%
                      ForLoops:    22ms    22ms   -0.9%    22ms    23ms   -2.2%
                    IfThenElse:    31ms    33ms   -5.9%    31ms    34ms   -7.9%
                   ListSlicing:    35ms    35ms   -0.1%    36ms    36ms   -2.0%
                NestedForLoops:    34ms    35ms   -2.3%    34ms    36ms   -4.7%
      NestedListComprehensions:    37ms    37ms   -1.2%    37ms    40ms   -7.6%
          NormalClassAttribute:    77ms    71ms   +8.1%    77ms    74ms   +3.9%
       NormalInstanceAttribute:    37ms    36ms   +2.1%    38ms    38ms   -0.6%
           PythonFunctionCalls:    35ms    34ms   +1.7%    35ms    35ms   -0.5%
             PythonMethodCalls:    45ms    44ms   +3.3%    46ms    45ms   +0.7%
                     Recursion:    60ms    58ms   +3.7%    60ms    59ms   +1.0%
                  SecondImport:    33ms    33ms   -0.0%    33ms    33ms   -1.9%
           SecondPackageImport:    34ms    34ms   +0.1%    34ms    34ms   -2.0%
         SecondSubmoduleImport:    83ms    86ms   -3.5%    85ms    87ms   -2.4%
       SimpleComplexArithmetic:    23ms    23ms   +0.7%    23ms    24ms   -2.2%
        SimpleDictManipulation:    51ms    53ms   -3.8%    52ms    55ms   -5.7%
         SimpleFloatArithmetic:    24ms    24ms   +0.3%    24ms    25ms   -2.0%
      SimpleIntFloatArithmetic:    31ms    30ms   +1.1%    31ms    31ms   -0.1%
       SimpleIntegerArithmetic:    30ms    30ms   +0.0%    30ms    31ms   -2.0%
      SimpleListComprehensions:    30ms    32ms   -4.6%    31ms    33ms   -5.7%
        SimpleListManipulation:    27ms    26ms   +2.4%    27ms    27ms   +0.4%
          SimpleLongArithmetic:    21ms    21ms   +2.1%    22ms    22ms   +0.2%
                    SmallLists:    36ms    36ms   -1.3%    36ms    37ms   -3.0%
                   SmallTuples:    43ms    43ms   -0.5%    44ms    45ms   -2.5%
         SpecialClassAttribute:    75ms    72ms   +4.3%    75ms    73ms   +2.5%
      SpecialInstanceAttribute:    38ms    41ms   -7.7%    38ms    42ms   -9.0%
                StringMappings:    77ms    76ms   +1.4%    77ms    78ms   -0.3%
              StringPredicates:    43ms    44ms   -2.6%    44ms    45ms   -3.9%
                 StringSlicing:    39ms    38ms   +3.2%    39ms    39ms   +1.0%
                     TryExcept:    21ms    21ms   -2.8%    21ms    22ms   -4.8%
                    TryFinally:    29ms    29ms   +0.2%    29ms    29ms   -1.2%
                TryRaiseExcept:    11ms    11ms   -0.2%    11ms    11ms   -1.1%
                  TupleSlicing:    42ms    43ms   -0.8%    45ms    45ms   +0.5%
                   WithFinally:    45ms    45ms   -0.8%    45ms    45ms   -1.2%
               WithRaiseExcept:    36ms    36ms   -0.9%    36ms    36ms   -1.5%
-------------------------------------------------------------------------------
Totals:                          1920ms  1927ms   -0.3%  1941ms  1980ms   -2.0%

(this=orig.pybench, other=patch.pybench)
msg237350 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 13:08
Even worse benchmark: timeit :-)

python -m timeit -s 'def f(): pass' 'f()'

* original: 67.5 nanoseconds
* patched: 64.1 nanoseconds (faster!?)
msg237351 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 13:28
New patch which removes some assertions and move other assertions on result/PyErr_Occurred.

I removed useless duplicated assertions and move the final assertion at the end of PyEval_EvalFrameEx() to guarantee that the function behave correctly.
msg237353 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 13:51
New patch to take in account Serhiy's comments. I also fixed call_function(), I forgot to call _Py_CheckFunctionResult() in two cases.
msg237355 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-06 13:56
I'm not sure that all removed assertions are not needed. There are many ways to get raised exception, not only by calling a function.
msg237357 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 14:16
Updated patch to take in account more Serhiy's comments.
msg237359 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-06 14:22
Could you please run microbenchmarks from issue23507?
msg237360 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 14:53
Oh, test_io and test_sqlite are failing in release mode with check_result-4.patch. io and sqlite modules only call PyErr_Clear() in debug mode. It's my fault, I added them, but I made them conditionnal to not impact performances. It's now fixed in the new patch.

Patch 5:

- "./python -m test -j0 -rW" succeeded in debug mode
- "./python -m test -j0 -rW" succeeded in release mode
- pybench: overall 0.6% slower with the patch (on the minimum runtime column)

Micro-benchmarks of issue #23507:

$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "list(filter(f, s))"
Unpatched: 10000 loops, best of 3: 96.3 usec per loop
Patched:   10000 loops, best of 3: 99.2 usec per loop (+3.0%)

$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "list(map(f, s))"
Unpatched: 10000 loops, best of 3: 89 usec per loop
Patched:   10000 loops, best of 3: 92.3 usec per loop (+3.7%)

$ ./python -m timeit -s "f = lambda x: x" -s "s = list(range(1000))" -- "sorted(s, key=f)"
Unpatched: 10000 loops, best of 3: 104 usec per loop
Patched:   10000 loops, best of 3: 106 usec per loop (+1.9%)
msg237364 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-06 15:55
We can ignore such little slow down (if this is not just compiler artifact).

The patch LGTM. I afraid there will be issues with third-party code that will need an adaptation to 3.5, but on other hand, this patch could help with catching some hidden bugs.
msg237387 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 22:22
2015-03-06 16:55 GMT+01:00 Serhiy Storchaka <report@bugs.python.org>:
> We can ignore such little slow down (if this is not just compiler artifact).

Maybe it's time to play with micro-optimizations like __builtin_expect
:-) But I would prefer to do that in a separated issue. The patch is
already long and complex.

> The patch LGTM. I afraid there will be issues with third-party code that will need an adaptation to 3.5, but on other hand, this patch could help with catching some hidden bugs.

My patch doesn't make more strict. Without my patch, if a function
returns a result and raise an exception, the exception will still be
noticed, but probably later. The purpose of my change is to notify
immediatly that the function raised an exception.

Without my patch, if a function returns a result and raise an
exception, it's possible that the exception is cleared and so never
seen.

In older Python versions, Python/ceval.c displayed the message "XXX
undetected error" when an exception was raised but also a result was
returned.

Anyway, you must fix your code :-)
msg237394 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-06 22:53
New changeset 97ef38236dc1 by Victor Stinner in branch 'default':
Issue #23571: PyObject_Call(), PyCFunction_Call() and call_function() now
https://hg.python.org/cpython/rev/97ef38236dc1
msg237396 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 23:01
Thanks for the very useful reviews Serhiy!
msg237755 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-10 12:25
New changeset 52c7017fdcdd by Victor Stinner in branch 'default':
Issue #23571: Oops, fix #ifdef assert()
https://hg.python.org/cpython/rev/52c7017fdcdd
msg238192 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-16 10:55
New changeset 91fbe0fff882 by Victor Stinner in branch 'default':
Issue #23571: Restore removed assert(!PyErr_Occurred()); in
https://hg.python.org/cpython/rev/91fbe0fff882
msg238749 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-03-21 02:07
Can we add a note (probably with an example) to the Porting to Python 3.5 section of https://docs.python.org/3.5/whatsnew/3.5.html#porting-to-python-3-5

I believe that this patch exposes some subtle bugs in Django (see https://gist.github.com/berkerpeksag/8b8dbe594eb1a1c51275) and it would be great to add a note for third party libraries.
msg238786 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-21 11:00
Serhiy was right: we should mention the name of the function in the
exception. In the Django traceback, it's not easy to guess what raised the
SystemError.

Ok to mention the change in What's New in Python 3.5.
msg238789 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-03-21 11:32
Instead of an assert(), you could use Py_FatalError() at the end of _Py_CheckFunctionResult().
msg238805 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-21 14:06
New changeset f30a5f6a665c by Victor Stinner in branch 'default':
Issue #23571: _Py_CheckFunctionResult() now gives the name of the function
https://hg.python.org/cpython/rev/f30a5f6a665c
msg238806 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-21 14:08
> I believe that this patch exposes some subtle bugs in Django (see https://gist.github.com/berkerpeksag/8b8dbe594eb1a1c51275) and it would be great to add a note for third party libraries.

Berker, can you please rerun your test with my new commit to check if you see the function error in the error?

(I didn't update the doc yet, not investigated Antoine's suggestion, I will do that later.)
msg238810 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-21 14:47
Non-Linux buildbots failed.

http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/2798/steps/test/logs/stdio
======================================================================
FAIL: test_return_result_with_error (test.test_capi.CAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_capi.py", line 206, in test_return_result_with_error
    self.assertIn(b'_Py_CheckFunctionResult: Assertion', err)
AssertionError: b'_Py_CheckFunctionResult: Assertion' not found in b'2015-03-21 10:28:29.114 defaults[68384:903] \nThe domain/default pair of (com.apple.CrashReporter, DialogType) does not exist\nAssertion failed: (!err_occurred), function _Py_CheckFunctionResult, file Objects/abstract.c, line 2088.\nFatal Python error: Aborted\n\nCurrent thread 0x00007fff71296cc0 (most recent call first):\n  File "<string>", line 6 in <module>'

----------------------------------------------------------------------
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5906/steps/test/logs/stdio
======================================================================
FAIL: test_return_result_with_error (test.test_capi.CAPITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_capi.py", line 206, in test_return_result_with_error
    self.assertIn(b'_Py_CheckFunctionResult: Assertion', err)
AssertionError: b'_Py_CheckFunctionResult: Assertion' not found in b'2015-03-21 10:28:29.114 defaults[68384:903] \nThe domain/default pair of (com.apple.CrashReporter, DialogType) does not exist\nAssertion failed: (!err_occurred), function _Py_CheckFunctionResult, file Objects/abstract.c, line 2088.\nFatal Python error: Aborted\n\nCurrent thread 0x00007fff71296cc0 (most recent call first):\n  File "<string>", line 6 in <module>'

----------------------------------------------------------------------

Why this assert is needed? Why not always raise SystemError?
msg238812 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-21 16:25
New changeset 970f33dff5ca by Victor Stinner in branch 'default':
Issue #23571: Fix test_capi
https://hg.python.org/cpython/rev/970f33dff5ca
msg238814 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-21 16:29
Serhiy Storchaka added the comment:
> Why this assert is needed? Why not always raise SystemError?

A SystemError exception may be ignored by a generic "except Exception:
pass" or logged at debug level at then ignored.

I consider that the bug is important, and that you must fix it. The
debug mode is to detect bugs earlier, right?
msg238816 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-21 17:05
This exception or assertion is triggered by the bug in CPython or in an extension. CPython developer uses release and debug builds of CPython and can get both exception or assertion. An extension developer usually uses release build of CPython with release and debug builds of an extension and can't get an assertion in CPython. Common Python user uses release builds of CPython and extensions and can only report about an exception. So this assertion can be used only in CPython developing and doesn't help to catch a bug in extensions. But an assertion itself provides less information than an exception. Debug build is less informative than release build.

May be add a runtime flag to control the reaction on system errors? If it set, all raised SystemError will be converted to fatal errors.
msg238827 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-21 19:24
Le 21 mars 2015 18:05, "Serhiy Storchaka" <report@bugs.python.org> a écrit :
> But an assertion itself provides less information than an exception.
Debug build is less informative than release build.

I like Antoine's idea to replace the assertion with Py_FatalError() in
debug mode.

I'm more concerned by bugs in Python itself. A fatal error/assertion should
be noticed quickly on buildbots which compile python in debug mode.

It's just fine if other people use the release mode and get an exception.

> May be add a runtime flag to control the reaction on system errors? If it
set, all raised SystemError will be converted to fatal errors.

It's already what I do when running Python test suite with pyfailmalloc. I
modify manually SystemError constructor in the C code. A flag may help, but
you have to know that many tests expect SystemError (I don't remember which
ones).

Victor
msg238861 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-03-22 01:19
Here's an exception in Django after the latest patch. The Django code block in the last exception catches ValueError, but this doesn't seem to work any longer since it's "wrapped" in SystemError. As Berker mentioned, some upgrade tips would be great as I'm not sure what adaptions in Django need to be made.

Traceback (most recent call last):
  File "/home/tim/code/django/django/contrib/contenttypes/views.py", line 17, in shortcut
    content_type = ContentType.objects.get(pk=content_type_id)
  File "/home/tim/code/django/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/tim/code/django/django/db/models/query.py", line 387, in get
    self.model._meta.object_name
django.contrib.contenttypes.models.DoesNotExist: ContentType matching query does not exist.

...

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
ValueError: could not convert string to float: request_path

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
....
  File "/home/tim/code/django/django/template/base.py", line 792, in __init__
    self.literal = float(var)
SystemError: <class 'ValueError'> returned a result with an error set
msg238880 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-22 06:55
Minimal reproducer:

>>> try:
...     1/0
... except:
...     int('spam')
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

ValueError: invalid literal for int() with base 10: 'spam'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
SystemError: <class 'ValueError'> returned a result with an error set
msg239110 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-24 11:10
New changeset efb2c9ac2f88 by Victor Stinner in branch '3.4':
Issue #23571: Enhance Py_FatalError()
https://hg.python.org/cpython/rev/efb2c9ac2f88

New changeset 6303795f035a by Victor Stinner in branch 'default':
(Merge 3.4) Issue #23571: Enhance Py_FatalError()
https://hg.python.org/cpython/rev/6303795f035a
msg239113 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-24 11:41
New changeset 2e14ca478a57 by Victor Stinner in branch 'default':
Issue #23571: PyErr_FormatV() and PyErr_SetObject() now always clear the
https://hg.python.org/cpython/rev/2e14ca478a57
msg239116 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-03-24 11:58
That last commit fixed compatibility with Django.
msg239118 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-24 13:07
New changeset 850b9dcd0534 by Victor Stinner in branch 'default':
Issue #23571: In debug mode, _Py_CheckFunctionResult() now calls
https://hg.python.org/cpython/rev/850b9dcd0534

New changeset da252f12352a by Victor Stinner in branch '3.4':
Issue #23571: Py_FatalError() now tries to flush sys.stdout and sys.stderr
https://hg.python.org/cpython/rev/da252f12352a

New changeset c9bcf669d807 by Victor Stinner in branch 'default':
(Merge 3.4) Issue #23571: Py_FatalError() now tries to flush sys.stdout and
https://hg.python.org/cpython/rev/c9bcf669d807

New changeset 57550e1f57d9 by Victor Stinner in branch 'default':
Issue #23571: Update test_capi
https://hg.python.org/cpython/rev/57550e1f57d9
msg239144 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-24 15:29
New changeset 2a18b958f1e1 by Victor Stinner in branch 'default':
Issue #23571: Enhance _Py_CheckFunctionResult()
https://hg.python.org/cpython/rev/2a18b958f1e1
msg239206 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-25 00:55
New changeset 1c2376825dd2 by Victor Stinner in branch '3.4':
Issue #23571: Fix reentrant call to Py_FatalError()
https://hg.python.org/cpython/rev/1c2376825dd2

New changeset e9ba95418af8 by Victor Stinner in branch 'default':
(Merge 3.4) Issue #23571: Fix reentrant call to Py_FatalError()
https://hg.python.org/cpython/rev/e9ba95418af8
msg239210 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-25 01:47
New changeset 004e3870d9e6 by Victor Stinner in branch '3.4':
Issue #23571: If io.TextIOWrapper constructor fails in _Py_DisplaySourceLine(),
https://hg.python.org/cpython/rev/004e3870d9e6
msg239551 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-30 01:30
> That last commit fixed compatibility with Django.

Good.

> Instead of an assert(), you could use Py_FatalError() at the end of _Py_CheckFunctionResult().

_Py_CheckFunctionResult() now calls Py_FatalError() in debug mode. By the way, I fixed various issues in Py_FatalError() to be able to display the exception and the traceback in more cases.

@Serhiy: Are you ok with that?

Can I close the issue?

FYI I write the PEP 490 as a "spin off" of this issue, to continue my work on the enhancement of exceptions in the C code: "PEP 490 - Chain exceptions at C level".
msg239567 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-30 05:43
The second (exception == NULL) check in _Py_PrintFatalError() looks suspicious. When it is possible? And if it is possible, can it cause leaks?
msg239581 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-30 08:38
Serhiy Storchaka added the comment:
> The second (exception == NULL) check in _Py_PrintFatalError() looks suspicious. When it is possible? And if it is possible, can it cause leaks?

Sorry, I have no idea. I didn't write this code myself. It comes from
PyErr_PrintEx():

    PyErr_Fetch(&exception, &v, &tb);
    if (exception == NULL)
        return;
    PyErr_NormalizeException(&exception, &v, &tb);
    if (tb == NULL) {
        tb = Py_None;
        Py_INCREF(tb);
    }
    PyException_SetTraceback(v, tb);
    if (exception == NULL)
        return;

I read again PyErr_NormalizeException(). I'm not sure that the case
can occur in practice. Maybe it can be replaced with an assertion?
Since Py_FatalError() is called in catastrophic cases, I chose to not
try to drop safety checks :-) You can modify PyErr_PrintEx() or even
_Py_PrintFatalError() if you feel more brave than me :-D
msg240185 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-04-06 21:30
I close the issue.
History
Date User Action Args
2015-11-04 23:34:05jceasetnosy: + jcea
2015-04-06 21:30:56vstinnersetstatus: open -> closed

messages: + msg240185
2015-03-30 08:38:51vstinnersetmessages: + msg239581
2015-03-30 05:43:22serhiy.storchakasetmessages: + msg239567
2015-03-30 01:30:55vstinnersetmessages: + msg239551
2015-03-25 01:47:37python-devsetmessages: + msg239210
2015-03-25 00:55:50python-devsetmessages: + msg239206
2015-03-24 15:29:09python-devsetmessages: + msg239144
2015-03-24 13:07:25python-devsetmessages: + msg239118
2015-03-24 11:58:08Tim.Grahamsetmessages: + msg239116
2015-03-24 11:41:35python-devsetmessages: + msg239113
2015-03-24 11:10:35python-devsetmessages: + msg239110
2015-03-22 06:55:20serhiy.storchakasetmessages: + msg238880
2015-03-22 01:19:59Tim.Grahamsetnosy: + Tim.Graham
messages: + msg238861
2015-03-21 19:24:05vstinnersetmessages: + msg238827
2015-03-21 17:05:49serhiy.storchakasetmessages: + msg238816
2015-03-21 16:29:57vstinnersetmessages: + msg238814
2015-03-21 16:25:10python-devsetmessages: + msg238812
2015-03-21 14:47:02serhiy.storchakasetmessages: + msg238810
2015-03-21 14:08:15vstinnersetmessages: + msg238806
2015-03-21 14:06:40python-devsetmessages: + msg238805
2015-03-21 11:32:39pitrousetnosy: + pitrou
messages: + msg238789
2015-03-21 11:00:07vstinnersetmessages: + msg238786
2015-03-21 02:07:17berker.peksagsetstatus: closed -> open
nosy: + berker.peksag
messages: + msg238749

2015-03-16 19:56:52berker.peksagsetstage: commit review -> resolved
2015-03-16 10:55:21python-devsetmessages: + msg238192
2015-03-10 12:25:32python-devsetmessages: + msg237755
2015-03-06 23:01:11vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg237396
2015-03-06 22:53:42python-devsetnosy: + python-dev
messages: + msg237394
2015-03-06 22:31:31serhiy.storchakasetassignee: vstinner
stage: commit review
2015-03-06 22:22:22vstinnersetmessages: + msg237387
2015-03-06 15:55:53serhiy.storchakasetmessages: + msg237364
2015-03-06 14:53:11vstinnersetfiles: + check_result-5.patch

messages: + msg237360
2015-03-06 14:22:26serhiy.storchakasetmessages: + msg237359
2015-03-06 14:16:06vstinnersetfiles: + check_result-4.patch

messages: + msg237357
2015-03-06 13:56:15serhiy.storchakasetmessages: + msg237355
2015-03-06 13:51:12vstinnersetfiles: + check_result-3.patch

messages: + msg237353
2015-03-06 13:28:22vstinnersetfiles: + check_result-2.patch

messages: + msg237351
2015-03-06 13:08:08vstinnersetmessages: + msg237350
2015-03-06 13:03:53vstinnersetmessages: + msg237349
2015-03-06 13:02:01vstinnersetmessages: + msg237348
2015-03-03 13:16:08vstinnersetmessages: + msg237132
2015-03-03 12:49:17serhiy.storchakasetmessages: + msg237128
2015-03-03 11:59:04vstinnersetfiles: + check_result.patch
keywords: + patch
2015-03-03 11:58:41vstinnercreate