bpo-33622: Add checks for exceptions leaks in the garbage collector. #7126
Conversation
Failure in adding to gc.garbage is no longer fatal.
| @@ -663,8 +663,10 @@ handle_legacy_finalizers(PyGC_Head *finalizers, PyGC_Head *old) | |||
| PyObject *op = FROM_GC(gc); | |||
There was a problem hiding this comment.
Usually, when I start to add assert(!PyErr_Occurred()), I like to add the assertion at the function entry and exit. Here it would avoid to remove an exception, since you add PyErr_Clear(). Currently, it's non obvious that the function must not be called with an exception set. An assertion would make it obvious ;-)
Modules/gcmodule.c
Outdated
| } | ||
| else { | ||
| if ((clear = Py_TYPE(op)->tp_clear) != NULL) { | ||
| Py_INCREF(op); | ||
| clear(op); | ||
| (void) clear(op); | ||
| assert(!PyErr_Occurred()); |
There was a problem hiding this comment.
You might put the assertion into clear().
There was a problem hiding this comment.
clear is tp_clear. It is defined in user code.
There was a problem hiding this comment.
Oh I see. In that case, the assertion is correct.
Modules/gcmodule.c
Outdated
| @@ -1127,6 +1137,7 @@ gc_collect_impl(PyObject *module, int generation) | |||
| n = 0; /* already collecting, don't do anything */ | |||
| else { | |||
| _PyRuntime.gc.collecting = 1; | |||
| assert(!PyErr_Occurred()); | |||
There was a problem hiding this comment.
Would it make sense to put the assertion inside collect_with_callback()? At the entry.
Modules/gcmodule.c
Outdated
| } | ||
| else { | ||
| if ((clear = Py_TYPE(op)->tp_clear) != NULL) { | ||
| Py_INCREF(op); | ||
| clear(op); | ||
| (void) clear(op); | ||
| assert(!PyErr_Occurred()); |
There was a problem hiding this comment.
clear is tp_clear. It is defined in user code.
|
I have replaced the assertion with this check because I have found few cases in the stdlib (very unlikely) in which |
Oh. It's a bug, right? Do you plan to open a new issue or write a fix for these bugs? If the bug is "very unlikely" and the case is now handled properly (error logged into stderr), maybe it's fine. |
|
I have opened three new issues for three possible cases of exceptions in |
Failure in adding to gc.garbage is no longer fatal.
https://bugs.python.org/issue33622