bpo-38006: Clear weakrefs in garbage found by the GC

[nascheme]

Fix a bug due to the interaction of weakrefs and the cyclic garbage
collector. We must clear any weakrefs in garbage in order to prevent
their callbacks from executing and causing a crash.

https://bugs.python.org/issue38006

[tim-one]

Ship it! Thank you, Neil.

[bedevere-bot]

@ambv: Please replace # with GH- in the commit message next time. Thanks!

[miss-islington]

Thanks @nascheme for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-16499 is a backport of this pull request to the 3.8 branch.

[nascheme]

Should we backport to all maintained branches? I think it applies even to 2.7.

[tim-one]

Should we backport to all maintained branches?

Ya, these holes have been there since the day weakrefs were added to the language.

[pitrou]

Uh, I'm a bit anxious about this. This is added to workaround a crash that's caused by a container not implementing the GC protocol correctly.

But in turn it may silence the invocation of many legitimate weakref callbacks. Weakref callbacks are often used to release external resources (for example system resources). This is actually the original motivation for weakref.finalize, which was originally a multiprocessing-internal feature used to release OS handles and the like.

I may be misreading the patch, but if a weakref that's collected as part of a cycle doesn't invoke its callback, then this will inevitably produce silent, difficult-to-diagnose bugs in complex systems.

Edit: ok, I read through the "Headache time" part and it's clearer now :-) My bad.

[tim-one]

To be sure we're on the same page, it was always the case that we suppressed callbacks from trash weakrefs. If the user wants the callback to be invoked for sure, it's the user's responsibility to ensure the weakref outlives the referent. finalize is great for making that so. Nothing about that is changed here.

What did change is that a missing tp_traverse tricked us into running a callback from a trash weakref, because we didn't realize it was trash. This occurred during delete_garbage(), after the callback had already been tp_clear'ed. Segfault.

I bet we could run more callbacks now, since you added a pass to abort the gc run if trash got resurrected. But I never heard anyone ask for that, and don't want it myself 😉