◐ Shell
reader mode source ↗
Skip to content

bpo-36829: Add sys.unraisablehook()#13187

Merged
vstinner merged 3 commits into
python:masterfrom
vstinner:unraisablehook
May 22, 2019
Merged

bpo-36829: Add sys.unraisablehook()#13187
vstinner merged 3 commits into
python:masterfrom
vstinner:unraisablehook

Conversation

@vstinner

@vstinner vstinner commented May 8, 2019

Copy link
Copy Markdown
Member

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

@serhiy-storchaka

Copy link
Copy Markdown
Member

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?

@graingert

Copy link
Copy Markdown
Contributor

@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

@graingert

Copy link
Copy Markdown
Contributor

the other PR for reference: #13175

@vstinner

vstinner commented May 8, 2019

Copy link
Copy Markdown
Member Author

@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

@vstinner

vstinner commented May 8, 2019

Copy link
Copy Markdown
Member Author

Should not we add os.abort()?

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

@vstinner

vstinner commented May 8, 2019

Copy link
Copy Markdown
Member Author

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.

@vstinner

vstinner commented May 8, 2019

Copy link
Copy Markdown
Member Author

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.

@vstinner

vstinner commented May 9, 2019

Copy link
Copy Markdown
Member Author

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

@jdemeyer

jdemeyer commented May 9, 2019

Copy link
Copy Markdown
Contributor

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.

@vstinner

vstinner commented May 9, 2019

Copy link
Copy Markdown
Member Author

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

@graingert

graingert commented May 9, 2019 via email

Copy link
Copy Markdown
Contributor

@vstinner

vstinner commented May 9, 2019

Copy link
Copy Markdown
Member Author

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.

@vstinner

vstinner commented May 9, 2019

Copy link
Copy Markdown
Member Author

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

@vstinner

vstinner commented May 9, 2019

Copy link
Copy Markdown
Member Author

See https://bugs.python.org/issue36829#msg342000 for PyErr_WriteUnraisable() called very late during Python shutdown.

@vstinner

Copy link
Copy Markdown
Member Author

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.

@vstinner vstinner requested review from methane and pablogsal May 10, 2019 23:29
@vstinner

Copy link
Copy Markdown
Member Author

@vstinner

Copy link
Copy Markdown
Member Author

@serhiy-storchaka: Would you mind to review the updated PR? Do you like the new API?

@vstinner

Copy link
Copy Markdown
Member Author

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 ;-)

@jdemeyer

Copy link
Copy Markdown
Contributor

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.

@vstinner

Copy link
Copy Markdown
Member Author

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.

@jdemeyer

Copy link
Copy Markdown
Contributor

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

36 hidden items Load more…
@vstinner

Copy link
Copy Markdown
Member Author

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!

@vstinner

Copy link
Copy Markdown
Member Author

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.

@zooba zooba self-requested a review May 16, 2019 15:48

@zooba zooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

@vstinner

Copy link
Copy Markdown
Member Author

_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

@vstinner

Copy link
Copy Markdown
Member Author

@pablogsal: I addressed your comments.

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
@vstinner

Copy link
Copy Markdown
Member Author

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

vstinner added 2 commits May 20, 2019 01:15
* 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
@vstinner

Copy link
Copy Markdown
Member Author

@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.

@vstinner

Copy link
Copy Markdown
Member Author

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

@vstinner vstinner merged commit ef9d9b6 into python:master May 22, 2019
@vstinner vstinner deleted the unraisablehook branch May 22, 2019 09:28
@vstinner

Copy link
Copy Markdown
Member Author

Thanks for reviews, I merged my PR!

@vstinner

Copy link
Copy Markdown
Member Author

Follow-up:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants