bpo-22898: Singleton RecursionError may hold a traceback in _PyExc_Fi… by xdegaye · Pull Request #1981 · 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

Conversation21 Commits1 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 }})

Did you verify the removed code here wasn't needed anymore? For the record, it was added in changeset 1e534b5 by @brettcannon.

Choose a reason for hiding this comment

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

This is part of the public API, so ripping it out is a breaking change.

Choose a reason for hiding this comment

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

I thought this PR was removing PyExc_RecursionErrorInst, ripping it out sounds scary though and I am not sure it should be done then :-)
Why is it part of the public API ?

Choose a reason for hiding this comment

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

For information, the PyExc_MemoryErrorInst singleton, also a part of the public API then, has been removed in 98e2b45 when fixing bpo-5437, and it was removed for the same reasons that this PR is removing the PyExc_RecursionErrorInst singleton.

Choose a reason for hiding this comment

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

Then it should at least be documented in What's New as having been removed since while it is an internal implementation detail, it's name wasn't given a leading _ to more significantly signal that it wasn't meant for use outside of Python itself.

Choose a reason for hiding this comment

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

I would have documented this removal in What's New just before the merge (to avoid merge conflicts) especially after you pointed out that it's part of the public API.

Choose a reason for hiding this comment

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

Doesn't removing this guard lead to stack overflow? See also #2035.

Choose a reason for hiding this comment

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

No because the failed invocations of the functions that are followed by a 'goto finally' (i.e. PyObject_IsSubclass() and _PyErr_CreateException()) are all either protected themselves by a Py_EnterRecursiveCall() or they set a built-in exception that won't cause PyErr_NormalizeException() to recurse indefinitely when being normalized.

#2035 does not fix bpo-22898 which is about the PyExc_RecursionErrorInst singleton keeping a reference to a traceback whose frames have locals that are finalized when interp->sysdict is NULL.

Choose a reason for hiding this comment

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

To be doubly sure one could add the following guard here:

    if (++tstate->recursion_depth > Py_GetRecursionLimit() + 50) {
        Py_FatalError("Cannot recover from stack overflow.");
    }

Choose a reason for hiding this comment

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

If this is a dead code this issue couldn't exist. Adding Py_FatalError() here will made the interpreter crashing earlier in the original reported example.

Choose a reason for hiding this comment

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

Your statement is not correct: test_recursion_normalizing_exception in this PR is a reproducer of the original reported example (check the gdb backtrace when run on the current code in master) and it does not abort or segfaults after adding the above Py_FatalError() guard to the PR and still raises the same RecursionError at the same recursion level and prints the ResourceWarning.

Are you aware that when tstate->overflowed is true a RecursionError is never raised ?

Did you verify the removed code here wasn't needed anymore?

Answering Antoine question on bpo-22898.

Choose a reason for hiding this comment

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

This line means PyExc_RecursionErrorInst is part of the stable ABI and shouldn't be removed at all. See PEP 384 for more information about the stable ABI.

Choose a reason for hiding this comment

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

Or at least I think so.

Choose a reason for hiding this comment

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

If the point is that using PyExc_RecursionErrorInst is an implementation oversight that may break Python, surely it is a case where its removal from the stable API is allowed no ?
IMO not removing it while not using it anymore in the CPython implementation looks worse.

Choose a reason for hiding this comment

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

@pitrou you can also look at my comments above about this exact topic. Apparently we have done this in the past when we removed exception singletons.

If we need final closure on this we can always ask on python-dev. And while you're right, @pitrou , that it might be part of the stable ABI (have to go through the header file to verify), apparently the stable ABI has been broken for some time.

Choose a reason for hiding this comment

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

Apparently we have done this in the past when we removed exception singletons.

Hmm, thank you, I stand corrected. I guess the dynamic linker doesn't mind a symbol isn't there if the symbol isn't actually used in the program linking with libpython.

apparently the stable ABI has been broken for some time.

This is unfortunate but you're probably right.

Choose a reason for hiding this comment

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

I have other concern. This singleton may be needed. Otherwise it wouldn't be added at first case.

Choose a reason for hiding this comment

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

I think the singleton was necessary on Python 2, which didn't have the two-level recursion threshold. It may not be necessary anymore on Python 3.

Choose a reason for hiding this comment

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

If I remember correctly, the reason the singleton was added was to allow for a deeper recursion limit so that constructing the instance itself didn't eat into the recursion depth (IOW it could fail to construct the instance). @pitrou may be right about it not being necessary in Python 3, but I'm personally not sure.

Labels

type-bug

An unexpected behavior, bug, or error