Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3.11+: Add __notes__ to error_already_set::what() output. #4678

Merged
merged 6 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 62 additions & 8 deletions include/pybind11/pytypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -471,13 +471,24 @@ inline const char *obj_class_name(PyObject *obj) {

std::string error_string();

// The code in this struct is very unusual, to minimize the chances of
// masking bugs (elsewhere) by errors during the error handling (here).
// This is meant to be a lifeline for troubleshooting long-running processes
// that crash under conditions that are virtually impossible to reproduce.
// Low-level implementation alternatives are preferred to higher-level ones
// that might raise cascading exceptions. Last-ditch-kind-of attempts are made
// to report as much of the original error as possible, even if there are
// secondary issues obtaining some of the details.
struct error_fetch_and_normalize {
// Immediate normalization is long-established behavior (starting with
// https://github.com/pybind/pybind11/commit/135ba8deafb8bf64a15b24d1513899eb600e2011
// from Sep 2016) and safest. Normalization could be deferred, but this could mask
// errors elsewhere, the performance gain is very minor in typical situations
// (usually the dominant bottleneck is EH unwinding), and the implementation here
// would be more complex.
// This comment only applies to Python <= 3.11:
// Immediate normalization is long-established behavior (starting with
// https://github.com/pybind/pybind11/commit/135ba8deafb8bf64a15b24d1513899eb600e2011
// from Sep 2016) and safest. Normalization could be deferred, but this could mask
// errors elsewhere, the performance gain is very minor in typical situations
// (usually the dominant bottleneck is EH unwinding), and the implementation here
// would be more complex.
// Starting with Python 3.12, PyErr_Fetch() normalizes exceptions immediately.
// Any errors during normalization are tracked under __notes__.
explicit error_fetch_and_normalize(const char *called) {
PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
if (!m_type) {
Expand All @@ -492,6 +503,14 @@ struct error_fetch_and_normalize {
"of the original active exception type.");
}
m_lazy_error_string = exc_type_name_orig;
#if PY_VERSION_HEX >= 0x030C0000
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this version check necessary? I know it should only be true for python 3.11 and above, but the hasattrstring is implicitly a version check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of it as a very easy runtime optimization, eliding the runtime overhead for the attribute lookup. — A while ago I looked at the implementation and it's actually quite involved. It basically does a full getattr() equivalent with PyErr_Clear().

// The presence of __notes__ is likely due to exception normalization
// errors, although that is not necessarily true, therefore insert a
// hint only:
if (PyObject_HasAttrString(m_value.ptr(), "__notes__")) {
m_lazy_error_string += "[WITH __notes__]";
}
#else
// PyErr_NormalizeException() may change the exception type if there are cascading
// failures. This can potentially be extremely confusing.
PyErr_NormalizeException(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
Expand All @@ -506,12 +525,12 @@ struct error_fetch_and_normalize {
+ " failed to obtain the name "
"of the normalized active exception type.");
}
#if defined(PYPY_VERSION_NUM) && PYPY_VERSION_NUM < 0x07030a00
# if defined(PYPY_VERSION_NUM) && PYPY_VERSION_NUM < 0x07030a00
// This behavior runs the risk of masking errors in the error handling, but avoids a
// conflict with PyPy, which relies on the normalization here to change OSError to
// FileNotFoundError (https://github.com/pybind/pybind11/issues/4075).
m_lazy_error_string = exc_type_name_norm;
#else
# else
if (exc_type_name_norm != m_lazy_error_string) {
std::string msg = std::string(called)
+ ": MISMATCH of original and normalized "
Expand All @@ -523,6 +542,7 @@ struct error_fetch_and_normalize {
msg += ": " + format_value_and_trace();
pybind11_fail(msg);
}
# endif
#endif
}

Expand Down Expand Up @@ -558,6 +578,40 @@ struct error_fetch_and_normalize {
}
}
}
#if PY_VERSION_HEX >= 0x030B0000
auto notes
= reinterpret_steal<object>(PyObject_GetAttrString(m_value.ptr(), "__notes__"));
if (!notes) {
PyErr_Clear(); // No notes is good news.
} else {
auto len_notes = PyList_Size(notes.ptr());
if (len_notes < 0) {
result += "\nFAILURE obtaining len(__notes__): " + detail::error_string();
} else {
result += "\n__notes__ (len=" + std::to_string(len_notes) + "):";
for (ssize_t i = 0; i < len_notes; i++) {
PyObject *note = PyList_GET_ITEM(notes.ptr(), i);
auto note_bytes = reinterpret_steal<object>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be reinterpret_borrow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copy-pasted from the existing code further up (btw I didn't see enough meat to factor out).

https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_AsEncodedString

returns a new reference, we have to take ownership.

(Unfortuantely IMO) it's called "steal" instead of "take_ownership", I think this is correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, nvm. Misread this and didn't notice the PyUnicode_AsEncodedString. You are 100% right.

PyUnicode_AsEncodedString(note, "utf-8", "backslashreplace"));
if (!note_bytes) {
result += "\nFAILURE obtaining __notes__[" + std::to_string(i)
+ "]: " + detail::error_string();
} else {
char *buffer = nullptr;
Py_ssize_t length = 0;
if (PyBytes_AsStringAndSize(note_bytes.ptr(), &buffer, &length)
== -1) {
result += "\nFAILURE formatting __notes__[" + std::to_string(i)
+ "]: " + detail::error_string();
} else {
result += '\n';
result += std::string(buffer, static_cast<std::size_t>(length));
}
}
}
}
}
#endif
} else {
result = "<MESSAGE UNAVAILABLE>";
}
Expand Down
36 changes: 28 additions & 8 deletions tests/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,13 +317,7 @@ def test_error_already_set_what_with_happy_exceptions(
assert what == expected_what


@pytest.mark.skipif(
# Intentionally very specific:
"sys.version_info == (3, 12, 0, 'alpha', 7)",
reason="WIP: https://github.com/python/cpython/issues/102594",
)
@pytest.mark.skipif("env.PYPY", reason="PyErr_NormalizeException Segmentation fault")
def test_flaky_exception_failure_point_init():
def _test_flaky_exception_failure_point_init_before_py_3_12():
with pytest.raises(RuntimeError) as excinfo:
m.error_already_set_what(FlakyException, ("failure_point_init",))
lines = str(excinfo.value).splitlines()
Expand All @@ -337,7 +331,33 @@ def test_flaky_exception_failure_point_init():
# Checking the first two lines of the traceback as formatted in error_string():
assert "test_exceptions.py(" in lines[3]
assert lines[3].endswith("): __init__")
assert lines[4].endswith("): test_flaky_exception_failure_point_init")
assert lines[4].endswith(
"): _test_flaky_exception_failure_point_init_before_py_3_12"
)


def _test_flaky_exception_failure_point_init_py_3_12():
# Behavior change in Python 3.12: https://github.com/python/cpython/issues/102594
what, py_err_set_after_what = m.error_already_set_what(
FlakyException, ("failure_point_init",)
)
assert not py_err_set_after_what
lines = what.splitlines()
assert lines[0].endswith("ValueError[WITH __notes__]: triggered_failure_point_init")
assert lines[1] == "__notes__ (len=1):"
assert "Normalization failed:" in lines[2]
assert "FlakyException" in lines[2]


@pytest.mark.skipif(
"env.PYPY and sys.version_info[:2] < (3, 12)",
reason="PyErr_NormalizeException Segmentation fault",
)
def test_flaky_exception_failure_point_init():
if sys.version_info[:2] < (3, 12):
_test_flaky_exception_failure_point_init_before_py_3_12()
else:
_test_flaky_exception_failure_point_init_py_3_12()


def test_flaky_exception_failure_point_str():
Expand Down