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 — bpo-44881: Integrate GC untrack into trashcan begin. by nascheme · Pull Request #27718 · python/cpython

Conversation

@nascheme

@nascheme nascheme commented Aug 10, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

For subtle reasons, PyObject_GC_UnTrack() function must be called before
Py_TRASHCAN_BEGIN(). There have been a number of bugs over the years
related to not doing this particular dance just right. Integrating
the PyObject_GC_UnTrack() call makes it harder to do things incorrectly.
That avoids some hard to find bugs (e.g. only triggered when object
nesting gets deep enough).

Extensions that still call PyObject_GC_UnTrack() explictly will
work correctly but the call is unneeded after this change. It would
still be needed for the extension to work correctly with older versions
of Python.

https://bugs.python.org/issue44881

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nascheme
Integrate GC untrack into trashcan begin.
d045bb6 
For subtle reasons, PyObject_GC_UnTrack() function must be called before
Py_TRASHCAN_BEGIN().  There have been a number of bugs over the years
related to not doing this particular dance just right.  Integrating
the PyObject_GC_UnTrack() call makes it harder to do things incorrectly.
That avoids some hard to find bugs (e.g. only triggered when object
nesting gets deep enough).

Extensions that still call PyObject_GC_UnTrack() explictly will still
work correctly but the call is unneeded after this change.  It would
still be needed for the extension to work correctly with older versions
of Python.
@nascheme
nascheme requested review from 1st1, markshannon, methane and rhettinger as code owners August 10, 2021 21:49
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 10, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Aug 10, 2021
@nascheme
nascheme requested a review from tim-one August 10, 2021 21:50
pablogsal
pablogsal approved these changes Aug 10, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 10, 2021
@pablogsal

pablogsal commented Aug 10, 2021

Copy link
Copy Markdown
Member

Let's land #27678 first to include that change in this PR
nascheme reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

nascheme commented Aug 10, 2021

Copy link
Copy Markdown
Member Author

Just to be clear, I don't consider this PR a bugfix and it would not be appropriate to include in 3.10.
ambv reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

pablogsal commented Aug 10, 2021

Copy link
Copy Markdown
Member

Just to be clear, I don't consider this PR a bugfix and it would not be appropriate to include in 3.10.

Yep, we are on the same page :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nascheme
small cleanup, move declarations after
42a10b4 
It seems slightly cleaner to have the BEGIN/END macros at the start and
end of the dealloc function body.
vstinner
vstinner reviewed Aug 12, 2021
View reviewed changes
Comment thread Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ambv
Add check for GC protocol per @vstinner request
7539849
@ambv ambv added the DO-NOT-MERGE label Aug 16, 2021
@ambv

ambv commented Aug 16, 2021

Copy link
Copy Markdown
Contributor

We won't be merging this as of yet because an alternative solution is evaluated in GH-27738. I added the assert Victor wanted so that the benchmarks are fairer between the two approaches.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Sep 16, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Sep 16, 2021
@rhettinger
rhettinger removed their request for review May 3, 2022 06:19
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 11, 2022
@nascheme nascheme mentioned this pull request Nov 28, 2023
Consider integration of PyObject_GC_UnTrack() with the trashcan C API #89044
Open
@nascheme

nascheme commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

This PR is out of date and will not be required if GH-132280 gets merged. That PR integrates the trashcan mechanism into Py_Dealloc and also stops using the GC next/prev pointers to store the queue of trash. So, it removes the need to call PyObject_GC_UnTrack() at the exact right time.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nascheme nascheme closed this Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@pablogsal pablogsal pablogsal approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@methane methane Awaiting requested review from methane methane is a code owner
@tim-one tim-one Awaiting requested review from tim-one

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge DO-NOT-MERGE

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.

7 participants

@nascheme @pablogsal @ambv @vstinner @the-knights-who-say-ni @ezio-melotti @bedevere-bot
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 01402520 — 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.