8000 gh-134170: Add colorization to unraisable exceptions by ZeroIntensity · Pull Request #134183 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

gh-134170: Add colorization to unraisable exceptions #134183

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 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
3 changes: 3 additions & 0 deletions Doc/whatsnew/3.15.rst
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ Other language changes
* Several error messages incorrectly using the term "argument" have been corrected.
(Contributed by Stan Ulbrych in :gh:`133382`.)

* Unraisable exceptions are now highlighted with color by default. This can be
controlled by :ref:`environment variables <using-on-controlling-color>`.
(Contributed by Peter Bierma in :gh:`134170`.)


New modules
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_capi/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import textwrap

from test import support
from test.support import import_helper
from test.support import import_helper, force_not_colorized
from test.support.os_helper import TESTFN, TESTFN_UNDECODABLE
from test.support.script_helper import assert_python_failure, assert_python_ok
from test.support.testcase import ExceptionIsLikeMixin
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_cmd_line.py
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ def test_unmached_quote(self):
self.assertRegex(err.decode('ascii', 'ignore'), 'SyntaxError')
self.assertEqual(b'', out)

@force_not_colorized
def test_stdout_flush_at_shutdown(self):
# Issue #5319: if stdout.flush() fails at shutdown, an error should
# be printed out.
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_concurrent_futures/test_shutdown.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def test_interpreter_shutdown(self):
self.assertFalse(err)
self.assertEqual(out.strip(), b"apple")

@support.force_not_colorized
def test_submit_after_interpreter_shutdown(self):
# Test the atexit hook for shutdown of worker threads and processes
rc, out, err = assert_python_ok('-c', """if 1:
Expand Down
3 changes: 2 additions & 1 deletion Lib/test/test_signal.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import unittest
from test import support
from test.support import (
is_apple, is_apple_mobile, os_helper, threading_helper
force_not_colorized, is_apple, is_apple_mobile, os_helper, threading_helper
)
from test.support.script_helper import assert_python_ok, spawn_python
try:
Expand Down Expand Up @@ -353,6 +353,7 @@ def check_signum(signals):

@unittest.skipIf(_testcapi is None, 'need _testcapi')
@unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()")
@force_not_colorized
def test_wakeup_write_error(self):
# Issue #16105: write() errors in the C signal handler should not
# pass silently.
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_sys.py
Original file line number Diff line number Diff line change
Expand Up @@ -1347,6 +1347,7 @@ def test_disable_gil_abi(self):


@test.support.cpython_only
@force_not_colorized
class UnraisableHookTest(unittest.TestCase):
def test_original_unraisablehook(self):
_testcapi = import_helper.import_module('_testcapi')
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -2397,6 +2397,7 @@ def test_atexit_called_once(self):

self.assertFalse(err)

@force_not_colorized
def test_atexit_after_shutdown(self):
# The only way to do this is by registering an atexit within
# an atexit, which is intended to raise an exception.
Expand Down
4 changes: 2 additions & 2 deletions Lib/traceback.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ def print_exception(exc, /, value=_sentinel, tb=_sentinel, limit=None, \
BUILTIN_EXCEPTION_LIMIT = object()


def _print_exception_bltin(exc, /):
file = sys.stderr if sys.stderr is not None else sys.__stderr__
def _print_exception_bltin(exc, file=None, /):
file = file or sys.stderr if sys.stderr is not None else sys.__stderr__
colorize = _colorize.can_colorize(file=file)
return print_exception(exc, limit=BUILTIN_EXCEPTION_LIMIT, file=file, colorize=colorize)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add colorization to unraisable exceptions by default
(:func:`sys.unraisablehook`).
16 changes: 16 additions & 0 deletions Python/errors.c
Original file line number Diff line number Diff line change
Expand Up @@ -1485,6 +1485,22 @@ write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,
}
}

// Try printing the exception with color
PyObject *print_exception_fn = PyImport_ImportModuleAttrString("traceback",
Copy link
Member

Choose a reason for hiding this comment

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

This would print the exception with color on stderr but write_unraisable_exc_file accepts arbitrary file objects. So:

  • Let's use a separate function to print the exception with color
  • Only import if file is not None.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just modified _print_exception_bltin to take a file, I don't think we need a new function. Is that alright?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the comment for this function is:

Do nothing if sys.stderr attribute doesn't exist or is set to None

So I don't really know which one is the correct implementation. There are cases when sys.__stderr__ may be None (as said in the docs). OTOH, the Python function doesn't check for the existence of sys.stderr itself. So I think the comment is a bit misleading. Can you check which contract is the expected one? Thanks.+

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, what am I checking for? If sys.stderr doesn't work, we'll raise an exception and fall back to the C implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Hide comment

But if sys.stderr does not exist, the function says "Do nothing if sys.stderr attribute doesn't exist or is set to None" which in this case isn't right as we're not doing "nothing" (we're still calling something). I'm not against your implementation, I'm just trying to understand the comment actually

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, yeah, I think that comment is wrong. Should I change it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes (wrong comments are misleading!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Did that and added an assertion. The caller always validates sys.stdout.

"_print_exception_bltin");
if (print_exception_fn != NULL && PyCallable_Check(print_exception_fn)) {
PyObject *args[2] = {exc_value, file};
PyObject *result = PyObject_Vectorcall(print_exception_fn, args, 2, NULL);
Py_DECREF(print_exception_fn);
Py_XDECREF(result);
if (result != NULL) {
return 0;
}
}
// traceback module failed, fall back to pure C
_PyErr_Clear(tstate);
Py_XDECREF(print_exception_fn);

if (exc_tb != NULL && exc_tb != Py_None) {
if (PyTraceBack_Print(exc_tb, file) < 0) {
/* continue even if writing the traceback failed */
Expand Down
Loading
0