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

bpo-35711: When asserting !PyErr_Occurred, output error before crashing #11513

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@sfreilich
Copy link

sfreilich commented Jan 10, 2019

It's better to crash than to silently swallow an error state. Aid debugging by outputting the unexpectedly-pending error before crashing.

https://bugs.python.org/issue35711

When asserting !PyErr_Occurred, output error before crashing
It's better to crash than to silently swallow an error state. Aid
debugging by outputting the unexpectedly-pending error before crashing.
@the-knights-who-say-ni

This comment has been minimized.

Copy link

the-knights-who-say-ni commented Jan 10, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@lisroach
Copy link
Contributor

lisroach left a comment

LGTM, please sign the CLA :)

@sfreilich

This comment has been minimized.

Copy link

sfreilich commented Jan 10, 2019

Signed. But the confirmation link doesn't confirm that yet. My bpo profile says "Contributor Form Received" and has my github name set correctly, so I guess that just hasn't propagated yet?

@vstinner
Copy link
Member

vstinner left a comment

I concur with @serhiy-storchaka: if C extensions are written properly, this change only adds an overhead for no gain. I would prefer to restrict the check to debug mode.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 11, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@sfreilich

This comment has been minimized.

Copy link

sfreilich commented Jan 11, 2019

@vstinner: I'm confused by some of that. Not commenting on whether or not these asserts should be compiled in by default (outside of noting that it seems a little like the idea that you can avoid overhead by assuming that you're never in a situation where undefined behavior happens, a trade-off that is not without drawbacks), but these particular assertions seemed to be in by default. I think this change only adds overhead when the program is about to crash due to an assert failure.

If PyErr_Print is not the best choice for outputting some debugging information about the pending error before assert failing, what is the correct choice?

"If C extensions are written properly" is a big if. I'm writing this change for a reason, I spent a lot of time debugging a failure of this particular assert that would have been limited if it output more context.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 11, 2019

By default, Python is compiled in release mode and removes all C assertions (assert(...);).

@sfreilich

This comment has been minimized.

Copy link

sfreilich commented Jan 11, 2019

That's not necessarily true for C++ extensions linking in that code, but I see your point. My objective wasn't to add additional work in circumstances where the assert is disabled. My point was that "!PyErr_Occurred() was false" and then a stack-trace for the assert isn't terribly useful for debugging in situations where the thing passed to PyErr_SetString might help figure out what's going on.

There are quite a few instances of assert(!PyErr_Occurred()) like this. Some of them are conditioned on Py_DEBUG some are not. Should they all be? Is there something that could be used instead of (or in addition to) assert, since assert doesn't provide a way of adding additional error output?

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 12, 2019

@sfreilich

This comment has been minimized.

Copy link

sfreilich commented Jan 14, 2019

Would it be reasonable for the sigabrt handler to print information about a pending error, if there is one?

static PyObject *
faulthandler_sigabrt(PyObject *self, PyObject *args)
{
    faulthandler_suppress_crash_report();
    if (PyErr_Occurred()) {
        PyObject *exception, *v, *tb, *hook;
        PyErr_Fetch(&exception, &v, &tb);
        if (exception != NULL) {
            // Something minimal to print the exception (what exactly?)
        }
    }
    abort();
    Py_RETURN_NONE;
}

That would avoid losing useful debugging messages, while not doing additional work in normal situations.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 15, 2019

Would it be reasonable for the sigabrt handler to print information about a pending error, if there is one?

I modified _testcapi.raise_exception() to call this code:

    PyErr_SetString(PyExc_ValueError, "oops");
    Py_FatalError("something bad happened");

On the following example:

import _testcapi
def bug():
    _testcapi.raise_exception()
def func():
    bug()
func()

Python fails with exit code 134 and the following output (I tested on debug build, but release build should give the same output):

Fatal Python error: something bad happened
ValueError: oops

Current thread 0x00007f51a2c81740 (most recent call first):
  File "x.py", line 3 in bug
  File "x.py", line 5 in func
  File "x.py", line 6 in <module>
Aborted (core dumped)

_Py_FatalError_PrintExc() is responsible to dump the current exception. It uses the Python sys.stderr object, normalizes the exception which can allocate memory on the heap to instanciate the exception, and call PyErr_Display() which calls the complex function print_exception_recursive(), etc.

A signal handler must not allocate memory because it can be executed anytime, even during a current allocation on the heap and memory allocators are usually not reentrant. Writing an async-safe signal handler is very difficult.

You can easily write our own signal handler (in C) for SIGABRT which will call Py_FatalError() to dump the current exception with its traceback. But that can crash and we don't want to do that in Python.

I close the PR since we are far from the initial change and your request doesn't fit Python performance and stability requirements.

@vstinner vstinner closed this Jan 15, 2019

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 15, 2019

cc @serhiy-storchaka: read my last comment FYI ;-)

@sfreilich sfreilich deleted the sfreilich:print-pending-error-before-crashing branch Jan 15, 2019

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