bpo-33622: Add checks for exceptions leaks in the garbage collector. by serhiy-storchaka · Pull Request #7126 · python/cpython

bpo-33622: Add checks for exceptions leaks in the garbage collector. by serhiy-storchaka · Pull Request #7126 · python/cpython

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

Choose a reason for hiding this comment

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

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

		Py_INCREF(op);
		clear(op);
		(void) clear(op);
		assert(!PyErr_Occurred());

Choose a reason for hiding this comment

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

You might put the assertion into clear().

Choose a reason for hiding this comment

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

clear is tp_clear. It is defined in user code.

Choose a reason for hiding this comment

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

Oh I see. In that case, the assertion is correct.

		n = 0; /* already collecting, don't do anything */
		else {
		_PyRuntime.gc.collecting = 1;
		assert(!PyErr_Occurred());

Choose a reason for hiding this comment

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

Would it make sense to put the assertion inside collect_with_callback()? At the entry.

Choose a reason for hiding this comment

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

LGTM.

My comments are just minor suggestions. The current change is good, if you want to apply it as it is.

Choose a reason for hiding this comment

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

Thank you Victor! I hoped on your review.

		Py_INCREF(op);
		clear(op);
		(void) clear(op);
		assert(!PyErr_Occurred());

Choose a reason for hiding this comment

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

clear is tp_clear. It is defined in user code.

Choose a reason for hiding this comment

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

The PR is now perfect :-D

Exception handling at the C level is hard :-( It's so easy to get it wrong (clear or replace the current exception by mistake). These assertions should help to detect such bugs earlier.

Choose a reason for hiding this comment

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

Still LGTM even with the latest change ;-)

I have replaced the assertion with this check because I have found few cases in the stdlib (very unlikely) in which tp_clear() sets an exception. Currently this can lead to a crash. Cases in the stdlib should be fixed in separate issues, but there may be third-party code.

I have replaced the assertion with this check because I have found few cases in the stdlib (very unlikely) in which tp_clear() sets an exception. Currently this can lead to a crash.

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