Raise SystemError if a function returns a result with an exception set

@jcea

BPO	23571
Nosy	@jcea, @pitrou, @vstinner, @berkerpeksag, @serhiy-storchaka, @timgraham
Files	check_result.patch check_result-2.patch check_result-3.patch check_result-4.patch check_result-5.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/vstinner'
closed_at = <Date 2015-04-06.21:30:56.045>
created_at = <Date 2015-03-03.11:58:41.396>
labels = ['interpreter-core']
title = 'Raise SystemError if a function returns a result with an exception set'
updated_at = <Date 2015-11-04.23:34:05.885>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2015-11-04.23:34:05.885>
actor = 'jcea'
assignee = 'vstinner'
closed = True
closed_date = <Date 2015-04-06.21:30:56.045>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2015-03-03.11:58:41.396>
creator = 'vstinner'
dependencies = []
files = ['38312', '38356', '38357', '38358', '38359']
hgrepos = []
issue_num = 23571
keywords = ['patch']
message_count = 41.0
messages = ['237123', '237128', '237132', '237348', '237349', '237350', '237351', '237353', '237355', '237357', '237359', '237360', '237364', '237387', '237394', '237396', '237755', '238192', '238749', '238786', '238789', '238805', '238806', '238810', '238812', '238814', '238816', '238827', '238861', '238880', '239110', '239113', '239116', '239118', '239144', '239206', '239210', '239551', '239567', '239581', '240185']
nosy_count = 7.0
nosy_names = ['jcea', 'pitrou', 'vstinner', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'Tim.Graham']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue23571'
versions = ['Python 3.5']

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!)

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.

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 :-)

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?

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)

Even worse benchmark: timeit :-)

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

original: 67.5 nanoseconds
patched: 64.1 nanoseconds (faster!?)

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.

New patch to take in account Serhiy's comments. I also fixed call_function(), I forgot to call _Py_CheckFunctionResult() in two cases.

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.

Updated patch to take in account more Serhiy's comments.

Could you please run microbenchmarks from bpo-23507?

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 bpo-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%)

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.

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 :-)

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

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

New changeset efb2c9ac2f88 by Victor Stinner in branch '3.4':
Issue bpo-23571: Enhance Py_FatalError()
https://hg.python.org/cpython/rev/efb2c9ac2f88

New changeset 6303795f035a by Victor Stinner in branch 'default':
(Merge 3.4) Issue bpo-23571: Enhance Py_FatalError()
https://hg.python.org/cpython/rev/6303795f035a

New changeset 2e14ca478a57 by Victor Stinner in branch 'default':
Issue bpo-23571: PyErr_FormatV() and PyErr_SetObject() now always clear the
https://hg.python.org/cpython/rev/2e14ca478a57

That last commit fixed compatibility with Django.

New changeset 850b9dcd0534 by Victor Stinner in branch 'default':
Issue bpo-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 bpo-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 bpo-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 bpo-23571: Update test_capi
https://hg.python.org/cpython/rev/57550e1f57d9

New changeset 2a18b958f1e1 by Victor Stinner in branch 'default':
Issue bpo-23571: Enhance _Py_CheckFunctionResult()
https://hg.python.org/cpython/rev/2a18b958f1e1

New changeset 1c2376825dd2 by Victor Stinner in branch '3.4':
Issue bpo-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 bpo-23571: Fix reentrant call to Py_FatalError()
https://hg.python.org/cpython/rev/e9ba95418af8

New changeset 004e3870d9e6 by Victor Stinner in branch '3.4':
Issue bpo-23571: If io.TextIOWrapper constructor fails in _Py_DisplaySourceLine(),
https://hg.python.org/cpython/rev/004e3870d9e6

@serhiy

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".

The second (exception == NULL) check in _Py_PrintFatalError() looks suspicious. When it is possible? And if it is possible, can it cause leaks?

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

I close the issue.

Navigation Menu

Raise SystemError if a function returns a result with an exception set #67759

17 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Raise SystemError if a function returns a result with an exception set #67759

Description

Activity

vstinner commented on Mar 3, 2015

serhiy-storchaka commented on Mar 3, 2015

vstinner commented on Mar 3, 2015

vstinner commented on Mar 6, 2015

vstinner commented on Mar 6, 2015

vstinner commented on Mar 6, 2015

vstinner commented on Mar 6, 2015

vstinner commented on Mar 6, 2015

serhiy-storchaka commented on Mar 6, 2015

vstinner commented on Mar 6, 2015

serhiy-storchaka commented on Mar 6, 2015

vstinner commented on Mar 6, 2015

serhiy-storchaka commented on Mar 6, 2015

vstinner commented on Mar 6, 2015

17 remaining items

timgraham commented on Mar 22, 2015

serhiy-storchaka commented on Mar 22, 2015

python-dev commented on Mar 24, 2015

python-dev commented on Mar 24, 2015

timgraham commented on Mar 24, 2015

python-dev commented on Mar 24, 2015

python-dev commented on Mar 24, 2015

python-dev commented on Mar 25, 2015

python-dev commented on Mar 25, 2015

vstinner commented on Mar 30, 2015

serhiy-storchaka commented on Mar 30, 2015

vstinner commented on Mar 30, 2015

vstinner commented on Apr 6, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions