Raise SystemError if a function returns a result with an exception set #67759
Comments
|
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
#endifBe careful of __builtin_expect(), misused it can make the code slower! (benchmark, benchmark, benchmark!) |
Could you please open separate issue or even a topic on Python-Ideas for |
|
2015-03-03 13:49 GMT+01:00 Serhiy Storchaka <report@bugs.python.org>:
I'm not really interested by micro-benchmark here :-) I also propose Since __builtin_expect() may have a negative effect on performances, I |
|
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 ### ### fastunpickle ### The following not significant results are hidden, use -v to show them: I ran again etree_parse alone and the slowdown disappeared :-p Output when I ran fastunpickle alone: 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. -------------------------------------------------------------------------------
------------------------------------------------------------------------------- ------------------------------------------------------------------------------- Test minimum run-time average run-time (this=orig.pybench, other=patch.pybench) |
|
Even worse benchmark: timeit :-) python -m timeit -s 'def f(): pass' 'f()'
|
|
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:
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>:
Maybe it's time to play with micro-optimizations like __builtin_expect
My patch doesn't make more strict. Without my patch, if a function Without my patch, if a function returns a result and raise an In older Python versions, Python/ceval.c displayed the message "XXX Anyway, you must fix your code :-) |
|
New changeset 97ef38236dc1 by Victor Stinner in branch 'default': |
|
Thanks for the very useful reviews Serhiy! |
|
New changeset 52c7017fdcdd by Victor Stinner in branch 'default': |
|
New changeset 91fbe0fff882 by Victor Stinner in branch 'default': |
|
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. |
|
Serhiy was right: we should mention the name of the function in the Ok to mention the change in What's New in Python 3.5. |
|
Instead of an assert(), you could use Py_FatalError() at the end of _Py_CheckFunctionResult(). |
|
New changeset f30a5f6a665c by Victor Stinner in branch 'default': |
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.) |
|
Non-Linux buildbots failed. http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/2798/steps/test/logs/stdio 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 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? |
|
New changeset 970f33dff5ca by Victor Stinner in branch 'default': |
|
Serhiy Storchaka added the comment:
A SystemError exception may be ignored by a generic "except Exception: I consider that the bug is important, and that you must fix it. The |
|
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. |
|
Le 21 mars 2015 18:05, "Serhiy Storchaka" <report@bugs.python.org> a écrit :
I like Antoine's idea to replace the assertion with Py_FatalError() in I'm more concerned by bugs in Python itself. A fatal error/assertion should It's just fine if other people use the release mode and get an exception.
It's already what I do when running Python test suite with pyfailmalloc. I Victor |
|
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': New changeset 6303795f035a by Victor Stinner in branch 'default': |
|
New changeset 2e14ca478a57 by Victor Stinner in branch 'default': |
|
That last commit fixed compatibility with Django. |
|
New changeset 850b9dcd0534 by Victor Stinner in branch 'default': New changeset da252f12352a by Victor Stinner in branch '3.4': New changeset c9bcf669d807 by Victor Stinner in branch 'default': New changeset 57550e1f57d9 by Victor Stinner in branch 'default': |
|
New changeset 2a18b958f1e1 by Victor Stinner in branch 'default': |
|
New changeset 1c2376825dd2 by Victor Stinner in branch '3.4': New changeset e9ba95418af8 by Victor Stinner in branch 'default': |
|
New changeset 004e3870d9e6 by Victor Stinner in branch '3.4': |
Good.
_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:
Sorry, I have no idea. I didn't write this code myself. It comes from 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 |
|
I close the issue. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: