classification
Title: Chain exceptions in C
Type: Stage: resolved
Components: Versions: Python 3.6
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-03-24 14:41 by vstinner, last changed 2017-06-28 01:23 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
pyerr_assert.patch vstinner, 2015-03-24 14:44 review
pyerr_match_clear.patch vstinner, 2015-03-24 14:46 review
pyerr_match_assertion.patch vstinner, 2015-03-24 14:48 review
pyerr_chain.patch vstinner, 2015-03-25 00:52
pyerr_match_clear-2.patch vstinner, 2015-03-25 00:56 review
Messages (14)
msg239130 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-24 14:41
In Python 3, it becomes possible to chain two exceptions. It's one of the killer feature of Python 3, it helps debugging.


In Python, exceptions are chained by default. Example:

    try:
        raise TypeError("old message")
    except TypeError:
        raise ValueError("new message")

Output:

    Traceback (most recent call last):
      File "x.py", line 2, in <module>
        raise TypeError("old message")
    TypeError: old message

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "x.py", line 4, in <module>
        raise ValueError("new message")
    ValueError: new message

In C, using the public PyErr_SetNone(), PyErr_Format(), PyErr_SetString(), ... functions, exceptions are *not* chained by default.

You have to call explicitly the new private _PyErr_ChainExceptions() function introduced in Python 3.4. It is not trivial to use it: you have to call PyErr_Fetch() and check if an exception was already raised.


In Python, the following examples are bad practice because they may hide important exceptions like MemoryError or KeyboardInterrupt:

    try:
        ....
    except:
        pass

or

    try:
        ....
    except:
        raise ValueError(...)

In C extensions, it's common to write such code, few functions check which exception was raised.


Last months, I enhanced Python to detect exceptions ignored by mistake: I added assert(!PyErr_Occurred()) in many functions which can clear the current exception (ex: call PyErr_Clear()) or raise a new exception (ex: call PyErr_SetString(...)). The last step is the issue #23571 which now implements in release mode.


For the next step, I propose to explicitly clear the current exception before raising a new exception.


I don't know yet if it would be a good idea to modify PyErr_*() functions to automatically chain exceptions.
msg239131 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-24 14:44
pyerr_assert.patch: modify PyErr_SetObject() and PyErr_Format() to fail with an assertion error if these functions are called with an exception set.

This patch detects where an exception is raised while another exception was already raised (like "try: ... except: raise ...").

I'm using this patch to explicitly not chain exceptions where chaining exceptions would be pointless (ex: replace a TypeError with a new TypeError with a better error message).
msg239132 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-24 14:46
> For the next step, I propose to explicitly clear the current exception before raising a new exception.

Attached pyerr_match_clear.patch implements this. It's only a work-in-progress. I prefer to get feedback on the patch before finishing it.

The patch checks also which exception was raised using PyErr_ExceptionMatches() to avoid hiding import exceptions.

Since my patch makes assumption on which exception is expected, it can change the behaviour of functions if I forgot a different exception which is also supposed to be replaced. Example: only catch ValueError and replace it with ValueError, whereas OverflowError must also be replaced with ValueError.
msg239133 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-24 14:48
pyerr_match_assertion.patch: Modify PyErr_ExceptionMatches() to raise an exception if it is called with no exception set.

This patch can be used to ensure that pyerr_match_clear.patch doesn't introduce regression.

Example:


-    PyErr_Format(PyExc_TypeError,
-                 "expected %s instance instead of %s",
-                 ((PyTypeObject *)type)->tp_name,
-                 Py_TYPE(value)->tp_name);
+
+    if (PyErr_ExceptionMatches(PyExc_AttributeError)) {
+        PyErr_Clear();
+        PyErr_Format(PyExc_TypeError,
+                     "expected %s instance instead of %s",
+                     ((PyTypeObject *)type)->tp_name,
+                     Py_TYPE(value)->tp_name);
+    }

This change is wrong is not exception is set, because PyErr_ExceptionMatches() returns 0 if no exception was raised.
msg239134 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-24 14:51
See also issue #21715: "Chaining exceptions at C level". The changeset 9af21752ea2a added the new _PyErr_ChainExceptions() function.
msg239137 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-24 15:04
While working on the PEP 475, I modified _Py_fopen_obj() to raise the OSError (instead of raising the exception from the call site). The zipimport uses _Py_fopen_obj() but it didn't raise OSError, only ZipImportError. I modified the zipimport module to raise a chained exception: OSError chained to ZipImportError: see issue #23696.

Other functions using _PyErr_ChainExceptions():

- open() (io.open): if open() failed and f.close() raised an exception, chain the two exceptions
- io.FileIO.close
- io.TextIOWrapper.close
- io.BufferedReader.close, io.BufferedWriter.close
- _Py_CheckFunctionResult(), new function introduced in the issue #23571
msg239200 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-25 00:32
pyerr_match_clear-2.patch: Updated patch, more complete (I also removed the assertions, I only added to debug).

> Since my patch makes assumption on which exception is expected, it can change the behaviour of functions if I forgot a different exception which is also supposed to be replaced.

To reduce risks, we can just remove the new PyErr_ExceptionMatches() checks from the patch in a first time, and add them later, while being very careful.

Calling PyErr_Clear() where exceptions must not be chained should be enough for a first step.
msg239201 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-25 00:40
New issue #23770: "Rework of exceptions are handled in the parser module (in validate_repeating_list())" to fix error handling in the parser module.
msg239203 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-25 00:46
Oh, I had a private local commit when I generated pyerr_match_clear-2.patch and so Rietveld failed to find the branch. I regenerated the patch.
msg239205 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-25 00:52
pyerr_chain.patch: The real patch to chain automatically exceptions when using
PyErr_*() functions.

pyerr_match_clear-2.patch hides the original exception when it is useless, too
low level like OverflowError when casting to C types, etc.

Examples without pyerr_match_clear-2.patch.

int, before:

| $ python3 -c 'int("123z")'
| Traceback (most recent call last):
|   File "<string>", line 1, in <module>
| ValueError: invalid literal for int() with base 10: '123z'

int, after:

| $ ./python -c 'int("123z")'
| ValueError: invalid literal for int() with base 10: '123z'
|
| During handling of the above exception, another exception occurred:
|
| Traceback (most recent call last):
|   File "<string>", line 1, in <module>
| ValueError: invalid literal for int() with base 10: '123z'

struct, before:

| $ python3 -c 'import struct; struct.pack("b", 2**100)'
| Traceback (most recent call last):
|   File "<string>", line 1, in <module>
| struct.error: argument out of range

struct, after:

| $ ./python -c 'import struct; struct.pack("b", 2**100)'
| OverflowError: Python int too large to convert to C long
|
| During handling of the above exception, another exception occurred:
|
| Traceback (most recent call last):
|   File "<string>", line 1, in <module>
| struct.error: argument out of range


More examples, after (still without pyerr_match_clear-2.patch):

int(str):

| TypeError: a bytes-like object is required, not 'type'
|
| During handling of the above exception, another exception occurred:
|
| Traceback (most recent call last):
|   File "<stdin>", line 1, in <module>
| TypeError: int() argument must be a string, a bytes-like object or a number, not 'type'

''.join(str):

| TypeError: 'type' object is not iterable
|
| During handling of the above exception, another exception occurred:
|
| Traceback (most recent call last):
|   File "<stdin>", line 1, in <module>
| TypeError: can only join an iterable


b"%f" % "abc":

| TypeError: a float is required
|
| During handling of the above exception, another exception occurred:
|
| Traceback (most recent call last):
|   File "<stdin>", line 1, in <module>
| TypeError: float argument required, not str
msg239207 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-25 00:56
Sorry, I had issues with my local repository. Third try to upload pyerr_match_clear-2.patch for Rietveld.
msg239227 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-25 06:49
I think it would be safer to defer automatically chaining exceptions to 3.6. After releasing 3.5 we can got reports about exceptions ignored by mistake. After fixing all bugs not covered by the testsuite, we could try automatically chain exceptions. May be this will require longer transient period, 3.6-3.7.
msg239233 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-25 07:53
Le mercredi 25 mars 2015, Serhiy Storchaka <report@bugs.python.org> a
écrit :
>
> I think it would be safer to defer automatically chaining exceptions to
> 3.6. After releasing 3.5 we can got reports about exceptions ignored by
> mistake.
>

Hum, what change can ignore exceptions by mistake?
msg297112 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-28 01:23
I lost track of this issue and my PEP 490 was more and less rejected, so I just close this issue.

I was decided that chaining or not exceptions should be done on a case by case basis.
History
Date User Action Args
2017-06-28 01:23:25vstinnersetstatus: open -> closed
resolution: out of date
messages: + msg297112

stage: resolved
2015-05-25 22:37:16vstinnersetversions: + Python 3.6
2015-03-25 07:53:41vstinnersetmessages: + msg239233
2015-03-25 06:49:07serhiy.storchakasetmessages: + msg239227
2015-03-25 00:56:57vstinnersetfiles: + pyerr_match_clear-2.patch

messages: + msg239207
2015-03-25 00:55:52vstinnersetfiles: - pyerr_match_clear-2.patch
2015-03-25 00:52:57vstinnersetfiles: + pyerr_chain.patch

messages: + msg239205
2015-03-25 00:46:33vstinnersetfiles: - pyerr_match_clear-2.patch
2015-03-25 00:46:21vstinnersetfiles: + pyerr_match_clear-2.patch

messages: + msg239203
2015-03-25 00:40:50vstinnersetmessages: + msg239201
2015-03-25 00:32:32vstinnersetfiles: + pyerr_match_clear-2.patch

messages: + msg239200
2015-03-24 15:04:52vstinnersetmessages: + msg239137
2015-03-24 14:51:12vstinnersetnosy: + serhiy.storchaka
messages: + msg239134
2015-03-24 14:48:44vstinnersetfiles: + pyerr_match_assertion.patch

messages: + msg239133
2015-03-24 14:46:38vstinnersetfiles: + pyerr_match_clear.patch

messages: + msg239132
2015-03-24 14:44:41vstinnersetfiles: + pyerr_assert.patch
keywords: + patch
messages: + msg239131
2015-03-24 14:41:37vstinnercreate