gh-74929: Fix an extra DECREF for PEP 667 implementation by gaogaotiantian · Pull Request #118583 · python/cpython

gh-74929: Fix an extra DECREF for PEP 667 implementation by gaogaotiantian · Pull Request #118583 · python/cpython

Conversation

In the PEP 667 implementation, when I copied the code from _PyFrame_LocalsToFast, I did not realize the value is a new reference so it was decrefed in the original code. This resulted in an extra DECREF in __setitem__. Unfortunately I did not catch it because the test cases used were all immortals.

Two test cases were added:

local object variable - this will trigger an assertion failure in debug mode
f_locals updating itself multiple times - this will crash the interpreter without the fix

Issue: Local variable assignment is broken when combined with threads + tracing + closures #74929

Choose a reason for hiding this comment

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

Maybe you want to delete or update the comment in the test?

		self.assertEqual(o, 'a.b.c')

		def test_update_with_self(self):
		# Make sure reference is not leaking here

Choose a reason for hiding this comment

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

The problem was not a leak though -- it was a free-after-use (the opposite of a leak, really :-).

Choose a reason for hiding this comment

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

Right. I was imaging a pool of reference and a hole that's leaking the reference without people knowing it so the pool is drained, but yeah leak is not a good word choice here. I just deleted it, the test itself makes sense without any comments.

Choose a reason for hiding this comment

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

Ooh, that's actually a nice image. :-) I will merge now.

Choose a reason for hiding this comment

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

Thanks for the quick review!

SonicField pushed a commit to SonicField/cpython that referenced this pull request