FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ cpython Public

gh-74929: Fix an extra DECREF for PEP 667 implementation - #118583

Merged
gvanrossum merged 2 commits into
python:mainfrom
gaogaotiantian:fix-framelocalsproxy-setitem
May 5, 2024
ConversationCommits (2)ChecksFiles changed
Merged

gh-74929: Fix an extra DECREF for PEP 667 implementation#118583
gvanrossum merged 2 commits into
python:mainfrom
gaogaotiantian:fix-framelocalsproxy-setitem

Conversation

gaogaotiantian commented May 5, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

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

bedevere-app Bot mentioned this pull request May 5, 2024
Closed
gaogaotiantian requested a review from gvanrossum May 5, 2024 00:05
gaogaotiantian mentioned this pull request May 5, 2024
Merged
gvanrossum approved these changes May 5, 2024
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Lib/test/test_frame.py Outdated
self.assertEqual(o, 'a.b.c')

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

gvanrossum May 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

gaogaotiantian May 5, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

gvanrossum May 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

gaogaotiantian May 5, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the quick review!

gvanrossum enabled auto-merge (squash) May 5, 2024 02:53
gvanrossum merged commit 5dd3673 into python:main May 5, 2024
gaogaotiantian deleted the fix-framelocalsproxy-setitem branch May 5, 2024 03:36
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
pythongh-74929: Remove undesirable DECREF in PEP 667 implementation (p…
b98b3de 
…ython#118583)

With tests.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gvanrossum gvanrossum approved these changes
markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL