bpo-36829: Add sys.unraisablehook()#13187
Conversation
|
The original proposition was about conditionally aborting the current process. Raising an exception (in particularly by |
Sorry, something went wrong.
|
@vstinner PyErr_WriteUnraisable can get called during Or the hook could be called after the hook is removed by GC and then we're back to the same problem |
Sorry, something went wrong.
It works as expected on my tests: https://bugs.python.org/issue36829#msg341868 |
Sorry, something went wrong.
That sounds like a reasonable addition, expose C abort() as os.abort(). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
I modified my PR to log exception if custom sys.unraisablehook itself raises an exception: see the new dedicated unit test. |
Sorry, something went wrong.
|
I'm not sure that it's safe to run arbitrary Python code any time that |
Sorry, something went wrong.
|
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: Output: |
Sorry, something went wrong.
|
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>
.
|
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
See https://bugs.python.org/issue36829#msg342000 for PyErr_WriteUnraisable() called very late during Python shutdown. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
|
@serhiy-storchaka: Would you mind to review the updated PR? Do you like the new API? |
Sorry, something went wrong.
|
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 ;-) |
Sorry, something went wrong.
I don't know any concrete example, it was just a general comment.
I meant temporary inconsistency, when you are in the middle of updating some data structure. |
Sorry, something went wrong.
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:
Moreover, the GC is not disabled and can be triggered anytime. So it's important that everything remains consistent when PyErr_WriteUnraisable() is called. |
Sorry, something went wrong.
|
OK, I agree. Like I said, it was just a comment, not a complaint. |
Sorry, something went wrong.
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! |
Sorry, something went wrong.
|
New version of my API. sys.unraisablehook now gets a single argument which has 4 fields:
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. |
Sorry, something went wrong.
zooba
left a comment
There was a problem hiding this comment.
Minor comments here, I put my bigger thoughts on python-dev :)
Sorry, something went wrong.
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 |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
* 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. |
Sorry, something went wrong.
|
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):
|
Sorry, something went wrong.
|
Thanks for reviews, I merged my PR! |
Sorry, something went wrong.
|
Follow-up:
|
Sorry, something went wrong.
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