bpo-36829: Add sys.unraisablehook() by vstinner · Pull Request #13187 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation68 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Add new sys.unraisablehook() function which can be overriden to
control how "unraisable exceptions" are handled. It is called when an
exception has occurred but there is no way for Python to handle it.
For example, when a destructor raises an exception or during garbage
collection (gc.collect()).

https://bugs.python.org/issue36829

The original proposition was about conditionally aborting the current process. Raising an exception (in particularly by os.exit()) does not work. Should not we add os.abort()? Or there are better alternatives for post-mortem investigation?

args[1] = (exc_value ? exc_value : Py_None);
args[2] = (exc_tb ? exc_tb : Py_None);
args[3] = (obj ? obj : Py_None);
res = _PyObject_FastCall(hook, args, Py_ARRAY_LENGTH(args));

Choose a reason for hiding this comment

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

Would not be better to call _PyErr_WriteUnraisable() if the hook set an exception instead of silently ignore it?

Choose a reason for hiding this comment

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

Hum, it's hard to know if the hook started to log something or not.

Choose a reason for hiding this comment

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

Maybe we could do something like logging: log a special message when the custom hook fails? We should just prevent recursive calls 😁

Choose a reason for hiding this comment

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

There is no recursion. I propose to call _PyErr_WriteUnraisable(), not PyErr_WriteUnraisable(). And pass hook as the obj argument.

Choose a reason for hiding this comment

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

PyErr_WriteUnraisable() now logs exception in custom unraisablehook using _PyErr_WriteUnraisable().

PyObject exc_value, PyObject exc_tb, PyObject *obj)
/[clinic end generated code: output=f83f725d576ea3d3 input=2aba571133d31a0f]/
{
return _PyErr_WriteUnraisable(exc_type, exc_value, exc_tb, obj);

Choose a reason for hiding this comment

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

Why not set sys.unraisablehook to None by default? Or not set it at all? What is the use case for exposing it at Python level?

Choose a reason for hiding this comment

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

I tried to use a design similar to sys.breakpointhook and sys.excepthook. It seems useful to be able to call sys.__unraisablehook__ from a custom hook for example.

Choose a reason for hiding this comment

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

There is a difference between sys.breakpointhook and sys.unraisablehook. The former is only called when you explicitly call breakpoint(). The latter can be called implicitly at arbitrary place and time. It is out of your control. sys.excepthook is also called implicitly, but at known circumstances -- when an uncaught exception achieves the top level. I afraid that calling sys.unraisablehook at arbitrary place may be unsafe.

Choose a reason for hiding this comment

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

sys.unraisablehook takes (exc_type, exc_value, exc_tb, obj) arguments, it doesn't rely on the current exception. Why would it be called to call it?

Choose a reason for hiding this comment

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

Sorry: Why would it be unsafe to call it?

Choose a reason for hiding this comment

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

For example because it can be called deep in the C stack.

@vstinner PyErr_WriteUnraisable can get called during __del__ or GC so most of the python environment could have already been deleted by the time the hook is called,

Or the hook could be called after the hook is removed by GC and then we're back to the same problem

the other PR for reference: #13175

@vstinner PyErr_WriteUnraisable can get called during del or GC so most of the python environment could have already been deleted by the time the hook is called, Or the hook could be called after the hook is removed by GC and then we're back to the same problem

It works as expected on my tests: https://bugs.python.org/issue36829#msg341868

Should not we add os.abort()?

That sounds like a reasonable addition, expose C abort() as os.abort().

Without os.abort(), you can call abort() using "import signal; signal.raise_signal(signal.SIGABRT)". But it only works if SIGABRT signal handler has not been overriden. Or maybe you can call "signal.signal(signal.SIGABRT, signal.SIG_DFL)" to restore the original signal handler.

Or the hook could be called after the hook is removed by GC and then we're back to the same problem

My implementations uses the default builtin hook if sys.unraisablehook doesn't exist or has been set to None.

I am not sure if we can trick PyImport_Cleanup to ensure that custom hook remains alive as long as possible.

PyObject exc_value, PyObject exc_tb, PyObject *obj)
/[clinic end generated code: output=f83f725d576ea3d3 input=2aba571133d31a0f]/
{
return _PyErr_WriteUnraisable(exc_type, exc_value, exc_tb, obj);

Choose a reason for hiding this comment

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

args[1] = (exc_value ? exc_value : Py_None);
args[2] = (exc_tb ? exc_tb : Py_None);
args[3] = (obj ? obj : Py_None);
res = _PyObject_FastCall(hook, args, Py_ARRAY_LENGTH(args));

Choose a reason for hiding this comment

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

There is no recursion. I propose to call _PyErr_WriteUnraisable(), not PyErr_WriteUnraisable(). And pass hook as the obj argument.

void
PyErr_WriteUnraisable(PyObject *obj)
PyObject*
_PyErr_WriteUnraisable(PyObject exc_type, PyObject exc_value,

Choose a reason for hiding this comment

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

Make it static.

Choose a reason for hiding this comment

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

It is called by sys_unraisablehook_impl() in sysmodule.c.

I modified my PR to log exception if custom sys.unraisablehook itself raises an exception: see the new dedicated unit test.

@@ -98,6 +98,12 @@ PyAPI_FUNC(_PyInitError) _Py_PreInitializeFromCoreConfig(
const _PyCoreConfig *coreconfig,
const _PyArgv *args);

PyAPI_FUNC(PyObject*) _PyErr_WriteUnraisable(

Choose a reason for hiding this comment

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

Why it is exposed in the header? Should not it be the static function?

Choose a reason for hiding this comment

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

sys.unraisablehook() calls _PyErr_WriteUnraisable().

goto done;
if (PyFile_WriteObject(obj, f, 0) < 0) {
if (hook_failed) {
if (PyFile_WriteString("sys.unraisablehook failed:", file) < 0) {

Choose a reason for hiding this comment

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

Why not show what exception was raised?

Choose a reason for hiding this comment

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

It is logged (see my comment below).

Choose a reason for hiding this comment

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

I often wish PyErr_WriteUnraisable() take an optional C string argument for describing the context. In many case there is no appropriate callable which can be passed as obj, sou you pass NULL and lost the context.

Choose a reason for hiding this comment

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

I added a new _PyErr_WriteUnraisableMsg() function exactly for that. I agree that PyErr_WriteUnraisable(NULL) is hard to debug. I only used it twice to show how it can be used. We can bikeshed later on converting existing PyErr_WriteUnraisable(NULL) to _PyErr_WriteUnraisableMsg() :-)

PyObject *res = _PyObject_FastCall(hook, args, Py_ARRAY_LENGTH(args));
Py_XDECREF(res);
if (res == NULL) {
unraisable_hook_failed();

Choose a reason for hiding this comment

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

I suggest to just fall back to the standard handler called below.

if (hook != NULL && hook != Py_None) { PyErr_Fetch(&exc_type, &exc_value, &exc_tb); ... // call the hook Py_XDECREF(exc_type); Py_XDECREF(exc_value); Py_XDECREF(exc_tb); if (res != NULL) { goto done; } obj = hook; } write_unraisable(...); done:

Choose a reason for hiding this comment

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

I inlined unraisable_hook_failed() to make the fallback more explicit.

Choose a reason for hiding this comment

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

Did you forget to push your changes?

Choose a reason for hiding this comment

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

I didn't write exactly what you proposed, but what I wrote is basically the same without goto. I dislike goto and "obj = hook" here. I prefer a more explicit fallback.

Choose a reason for hiding this comment

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

Well. I modified my PR to write an explicit fallback as you asked me for :-)

I'm not sure that it's safe to run arbitrary Python code any time that PyErr_WriteUnraisable is called. There might be occasions where PyErr_WriteUnraisable is called but where CPython is internally in an inconsistent state where calling Python code would crash the interpreter.

When a custom hook fails, the exception is logged. For example, test_custom_unraisablehook_fail() checks for "Exception: hook_func failed" in the output.

Another example:

import _testcapi
import sys

def hook_func(*args):
    raise Exception("hook_func failed")

sys.unraisablehook = hook_func
_testcapi.write_unraisable(ValueError(42), None)

Output:

sys.unraisablehook failed:
Traceback (most recent call last):
  File "x.py", line 5, in hook_func
    raise Exception("hook_func failed")
Exception: hook_func failed

If the hook fails, will it abort the process with a non-zero exit code?

…

On Thu, 9 May 2019, 21:19 Victor Stinner, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Python/errors.c <#13187 (comment)>: > + PyObject *exc_type, *exc_value, *exc_tb; + + PyErr_Fetch(&exc_type, &exc_value, &exc_tb); + + _Py_IDENTIFIER(unraisablehook); + PyObject *hook = _PySys_GetObjectId(&PyId_unraisablehook); + if (hook != NULL && hook != Py_None) { + PyObject *args[4]; + args[0] = (exc_type ? exc_type : Py_None); + args[1] = (exc_value ? exc_value : Py_None); + args[2] = (exc_tb ? exc_tb : Py_None); + args[3] = (obj ? obj : Py_None); + PyObject *res = _PyObject_FastCall(hook, args, Py_ARRAY_LENGTH(args)); + Py_XDECREF(res); + if (res == NULL) { + unraisable_hook_failed(); I inlined unraisable_hook_failed() to make the fallback more explicit. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13187 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADFATFUITFK6GHVTSB5MH3PUSBMFANCNFSM4HLQVLHQ> .

I'm not sure that it's safe to run arbitrary Python code any time that PyErr_WriteUnraisable is called. There might be occasions where PyErr_WriteUnraisable is called but where CPython is internally in an inconsistent state where calling Python code would crash the interpreter.

Do you have an example? I'm not aware of such issue.

Python must always remain consistent. Usually, Py_FatalError() is called when an inconstancy is detected.

If the hook fails, will it abort the process with a non-zero exit code?

No, the second new error is logged. Python should not be aborted when something goes wrong: it should be possible to continue the execution.

Another complete solution for https://bugs.python.org/issue36829 would be to add a new -X command line option to use a specific exit code if at least one "unraisable" exception is logged. So far, nobody showed an example where my hook cannot be used to fix https://bugs.python.org/issue36829

I added "msg" to sys.unraisablehook.

Ok, so I rebased my PR on top of master to reduce the risk of conflicts, I squashed my commits to be able to update the main commit message, and I added a new "msg" parameter to sys.unraisablehook.

The new "msg" parameter allows to pass an arbitrary error message rather than the hardcoded "Exception ignored in: ". I added _PyErr_WriteUnraisableMsg() to log an unraisable exception with an error message.

I only used _PyErr_WriteUnraisableMsg() twice where it was appropriate to show how it can be used. If the PR is accepted, a following PR can be written to add a more accurate error message (than the hardcoded error message). I prefer to keep this PR as small as possible.

I also cleaned up the implementation and added more comments.

I plan to merge this PR at the end of the week. I addressed all remarks/requests.

The only remaining complain about this PR is the fact that it doesn't allow to catch exceptions raised very late during Python shutdown: https://bugs.python.org/issue36829#msg342001

I confirm that it's a known limitation, but I am not comfortable with killing (SIGABRT) Python when PyErr_WriteUnraisable() is called late during Python finalization: there is no "easy" way to debug such issue. Only a low-level debugger like gdb can maybe provide some info about what happened. Or maybe not since Python finalization already destroyed too many things: modules, thread state, etc.

Python finalization code is very fragile and it's a work-in-progress for 5 years. I would love to see enhancements, but multiple attempts failed badly with adding new corner cases and triggering new crashes, and so had to be reverted.

My PR is a compromise for the most common cases. IMHO it's a way more generic solution than only allow to kill the process with SIGABRT. You are free to log "unraisable exceptions" into a file, raise a signal, send exceptions into a network socket, etc. Basically, whatever you want ;-)

Do you have an example? I'm not aware of such issue.

I don't know any concrete example, it was just a general comment.

Python must always remain consistent.

I meant temporary inconsistency, when you are in the middle of updating some data structure.

I meant temporary inconsistency, when you are in the middle of updating some data structure.

Ok, I see what you mean. If we have such code, it's a bug that should be fixed.

The default hook implementation, current PyErr_WriteUnraisable() implementation, already executes "arbitrary" Python code:

call repr(exception_value) which can be an arbitrary Python function
get exception_type.module attribute
call sys.stderr.write() whereas sys.stderr can be a custom object
displaying the traceback calls io.open(filename, "rb"), parse the encoding cookie, create a io.TextIOWrapper, call fp.readline() and then fp.close()
etc.

Moreover, the GC is not disabled and can be triggered anytime. So it's important that everything remains consistent when PyErr_WriteUnraisable() is called.

OK, I agree. Like I said, it was just a comment, not a complaint.

OK, I agree. Like I said, it was just a comment, not a complaint.

Thanks for sharing your concerns. This issue is very tricky, I wouldn't be surprised if we discover a corner case tomorrow ;-) So having more people looking into the code and how it's used is very helpful!

New version of my API. sys.unraisablehook now gets a single argument which has 4 fields:

exc_type
exc_value
exc_tb
obj

It becomes possible to extend this object later to add new attribute without breaking the backward compatibility (existing hooks used in the wild).

I also reverted unrelated changes.

I removed _PyErr_WriteUnraisableMsg() and the "err_msg" parameter / field. I tried to write a PR as small as possible. Once this PR will be merged, I will work on a second PR to add a new err_msg field to the hook and add back _PyErr_WriteUnraisableMsg() function.

Choose a reason for hiding this comment

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

Minor comments here, I put my bigger thoughts on python-dev :)

_PySys_GetObjectId() returns a borrowed reference.

To be clear, I trust no one, especially myself: since I started to work on this PR, I'm running frequently "./python -m test test_sys -R 3:3" to make sure that nothing leaks :-) I added multiple tests in test_sys, and it happened multiple times that I introduced a giant leak because of an obvious bug in my code :-D

Add new sys.unraisablehook() function which can be overriden to control how "unraisable exceptions" are handled. It is called when an exception has occurred but there is no way for Python to handle it. For example, when a destructor raises an exception or during garbage collection (gc.collect()).

Changes:

The default hook now ignores exception on writing the traceback.
Add an internal UnraisableHookArgs type used to pass arguments to sys.unraisablehook.
Add _PyErr_WriteUnraisableDefaultHook().
test_sys now uses unittest.main() to automatically discover tests: remove test_main().
Add _PyErr_Init().
Fix PyErr_WriteUnraisable(): hold a strong reference to sys.stderr while using it

I squashed my commits, rebased them on master and fix a merge conflict.

Rename 'exc_tb' field to 'exc_traceback'
Rename 'obj' field to 'object'
Fix PyErr_WriteUnraisable(): don't call sys.unraisablehook if exc_type is NULL (this case should not happen)
Documentation: add links between sys.excepthook and sys.unraisablehook

Fix typo in the doc
Update PyErr_WriteUnraisable() documentation
Regenerate importlib.h

@serhiy-storchaka @pablogsal @njsmith: Would you mind to review my PR adding sys.unraisablehook?

I renamed UnraisableHookArgs fields to (exc_type, exc_value, exc_traceback, object). I also fixed a few more bugs in the implementation to fix a few more corner cases like exc_type == NULL.

I also completed the documentation.

I plan to merge this change next Wednesday (in 2 days). I didn't see any strong opinion to the API, and I'm not convinced by other proposed APIs.

Once this PR will be merged, I'm interested to work on follow-up (I plan to write 1 PR to bullet):

Add an optional error message to UnraisableHookArgs to override default "Exception ignored in:"
Try to normalize the exception
Try to create a traceback object and attack it to the exception
Add a helper function to test.support to catch unraisable exception and modify unit tests generating such unraisable exception
Maybe also experiment to modify regrtest to log these unraisable exceptions to display them again in the summary at the end

Thanks for reviews, I merged my PR!

Follow-up:

PR [3.7] bpo-36829: Enhance PyErr_WriteUnraisable() #13487 backports enhancement and bugfix to Python 3.7
PR bpo-36829: Add _PyErr_WriteUnraisableMsg() #13488 adds _PyErr_WriteUnraisableMsg() and adds 'err_msg' field to sys.unraisablehook
PR bpo-36829: Add test.support.catch_unraisable_exception() #13490 adds test.support.catch_unraisable_exception()

This was referenced

May 22, 2019

Reviewers

zooba zooba left review comments

serhiy-storchaka serhiy-storchaka left review comments

pablogsal pablogsal left review comments

auvipy auvipy approved these changes

methane Awaiting requested review from methane