GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-74929: Fix an extra DECREF for PEP 667 implementation by gaogaotiantian · Pull Request #118583 · python/cpython

Conversation

@gaogaotiantian

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

Uh oh!

There was an error while loading. Please reload this page.

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
  • Issue: Local variable assignment is broken when combined with threads + tracing + closures #74929

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gaogaotiantian
Fix an extra DECREF for pep 667 implementation
2bb4e2e
@gaogaotiantian
gaogaotiantian requested a review from markshannon as a code owner May 5, 2024 00:05
@bedevere-app bedevere-app Bot added the awaiting review label May 5, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 5, 2024
Local variable assignment is broken when combined with threads + tracing + closures #74929
Closed
@gaogaotiantian gaogaotiantian added the skip news label May 5, 2024
@gaogaotiantian
gaogaotiantian requested a review from gvanrossum May 5, 2024 00:05
@gaogaotiantian gaogaotiantian mentioned this pull request May 5, 2024
gh-83151: Make closure work on pdb #111094
Merged
gvanrossum
gvanrossum approved these changes May 5, 2024
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 gvanrossum May 5, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 :-).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gaogaotiantian gaogaotiantian May 5, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum May 5, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gaogaotiantian gaogaotiantian May 5, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 5, 2024
@gaogaotiantian
Delete comment
0bbfd4f
@gvanrossum
gvanrossum enabled auto-merge (squash) May 5, 2024 02:53
@gvanrossum
gvanrossum merged commit 5dd3673 into python:main May 5, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 5, 2024
@gaogaotiantian
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
@gaogaotiantian @SonicField
pythongh-74929: Remove undesirable DECREF in PEP 667 implementation (p…
b98b3de 
…ython#118583)

With tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@gaogaotiantian @gvanrossum
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.