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-46070: _PyGC_Fini() untracks objects by vstinner · Pull Request #30577 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Jan 13, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Py_EndInterpreter() now explicitly untracks all objects currently
tracked by the GC. Previously, if an object was used later by another
interpreter, calling PyObject_GC_UnTrack() on the object crashed if
the previous or the next object of the PyGC_Head structure became a
dangling pointer.

https://bugs.python.org/issue46070

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently and erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 2 reactions
@vstinner
vstinner requested a review from pablogsal as a code owner January 13, 2022 17:04
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 13, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Jan 13, 2022
@vstinner vstinner added needs backport to 3.9 and removed awaiting core review labels Jan 13, 2022
@vstinner
bpo-46070: _PyGC_Fini() untracks objects
a59e2a4
Py_EndInterpreter() now explicitly untracks all objects currently
tracked by the GC. Previously, if an object was used later by another
interpreter, calling PyObject_GC_UnTrack() on the object crashed if
the previous or the next object of the PyGC_Head structure became a
dangling pointer.
@vstinner

vstinner commented Jan 13, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

cc @pablogsal @erlend-aasland @corona10 @ericsnowcurrently

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

I do not know the GC, so I'm not the right person to review this change. It sounds good to me, though, FWIW 😄

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner merged commit 1a4d1c1 into python:main Jan 13, 2022
@miss-islington

miss-islington commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner deleted the gc_fini_untrack branch January 13, 2022 18:28
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 13, 2022
@vstinner @miss-islington
bpo-46070: _PyGC_Fini() untracks objects (pythonGH-30577)
4bc978c
Py_EndInterpreter() now explicitly untracks all objects currently
tracked by the GC. Previously, if an object was used later by another
interpreter, calling PyObject_GC_UnTrack() on the object crashed if
the previous or the next object of the PyGC_Head structure became a
dangling pointer.
(cherry picked from commit 1a4d1c1)

Co-authored-by: Victor Stinner <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Jan 13, 2022
@bedevere-bot

bedevere-bot commented Jan 13, 2022

Copy link
Copy Markdown

GH-30578 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 13, 2022
@vstinner @miss-islington
bpo-46070: _PyGC_Fini() untracks objects (pythonGH-30577)
faf2783
Py_EndInterpreter() now explicitly untracks all objects currently
tracked by the GC. Previously, if an object was used later by another
interpreter, calling PyObject_GC_UnTrack() on the object crashed if
the previous or the next object of the PyGC_Head structure became a
dangling pointer.
(cherry picked from commit 1a4d1c1)

Co-authored-by: Victor Stinner <[email protected]>
@bedevere-bot

bedevere-bot commented Jan 13, 2022

Copy link
Copy Markdown

GH-30579 is a backport of this pull request to the 3.9 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Jan 13, 2022
This was referenced Jan 13, 2022
bpo-46070: Revert "bpo-36854: Move _PyRuntimeState.gc to PyInterpreterState (GH-17287)" #30564
Closed
[3.10] bpo-46070: Revert "bpo-36854: Move _PyRuntimeState.gc to PyInterpreterState (GH-17287)" #30565
Closed
[3.9] bpo-46070: Revert "bpo-36854: Move _PyRuntimeState.gc to PyInterpreterState (GH-17287)" #30566
Closed
ericsnowcurrently
ericsnowcurrently reviewed Jan 13, 2022
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.

The approach seems good to me, but I'd want to hear what @pablogsal, @pitrou, @iritkatriel, etc. have to say.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/gcmodule.c
Comment on lines +2184 to +2188
// bpo-46070: Explicitly untrack all objects currently tracked by the
// GC. Otherwise, if an object is used later by another interpreter,
// calling PyObject_GC_UnTrack() on the object crashs if the previous
// or the next object of the PyGC_Head structure became a dangling
// pointer.

@ericsnowcurrently ericsnowcurrently Jan 13, 2022

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.

FWIW, this change would be unnecessary no objects were ever shared. It may be worth adding such a note to this comment, so it's clear that this extra cleanup code could be removed at that point.

Sorry, something went wrong.

Uh oh!

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

All reactions

@vstinner vstinner Jan 13, 2022

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.

Once we will be sure that it is really impossible to share any object, we can just remove this code.

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/gcmodule.c
PyGC_Head *gc;
for (gc = GC_NEXT(list); gc != list; gc = GC_NEXT(list)) {
PyObject *op = FROM_GC(gc);
_PyObject_GC_UNTRACK(op);

@ericsnowcurrently ericsnowcurrently Jan 13, 2022

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.

So one or more objects "owned" by another interpreter (AKA OTHER) could still hold a reference to one of these untracked objects (AKA UNTRACKED). The bug we're fixing demonstrates that's a possibility.

Could it make it harder to break cycles? Could it lead to memory leaks?

How will the UNTRACKED object impact GC for any OTHER object holding a reference to it?

What happens if one of these UNTRACKED objects is involved in a cycle with one or more OTHER objects? Can we still clean them all up?

What happens if several of these UNTRACKED objects are involved in a cycle, but one or more OTHER objects holds a reference to one of them? Can we still clean them all up?

Sorry, something went wrong.

Uh oh!

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

All reactions

@vstinner vstinner Jan 13, 2022

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.

If an object is not tracked by the GC and is part of a ref cycle, the GC is unable to break the reference cycle, and so yes, memory leaks. Previously, Python was crashing. Well, the situation is less bad :-)

This change doesn't introduce the memory leak. An object cannot be tracked in two GC lists at the same time: _PyObject_GC_TRACK() has an assertion for that. The leak was already there.

If an object is created in interpreter 1, it's tracked by the GC of the interpreter 1. If it's copied to the interpreter 2 and the interpreter 1 is destroyed, the interpreter 2 GC is not going to automatically tracks the object. Moreover, the interpreter 1 cannot guess if another interpreter is using the object or not.

IMO untracking all objects is the least bad solution.

IMO the only way to ensure that no memory is leaked is to prevent sharing objects between interpreters, and rely on existing mechanisms (GC and finalizers) to release memory. So continue to convert static types to heap types, continue to update C extensions to the multi-phase initialization, continue moving globals into module state and per-interpreter structures, etc.

Sorry, something went wrong.

Uh oh!

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

erlend-aasland and ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@ericsnowcurrently ericsnowcurrently Jan 13, 2022

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.

so yes, memory leaks. Previously, Python was crashing. Well, the situation is less bad :-)

🙂

Sorry, something went wrong.

Uh oh!

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

All reactions

@vstinner vstinner Jan 14, 2022 •
edited
Loading

Uh oh!

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

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.

In Python 3.8, when the GC state was shared, it seems like any interpreter could run a GC collection. It couldn't happen in parallel, thanks to the "collecting" flag. The GC is not re-entrant and simply does nothing (exit) if it's already collecting.

I cannot say if Python 3.8 was able to break the reference cycles that Python 3.9 and newer can no longer break: when an object in created in an interpreter and then "migrates" to another interpreter.

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
miss-islington added a commit that referenced this pull request Jan 13, 2022
@miss-islington @vstinner
bpo-46070: _PyGC_Fini() untracks objects (GH-30577)
e6bb17f
Py_EndInterpreter() now explicitly untracks all objects currently
tracked by the GC. Previously, if an object was used later by another
interpreter, calling PyObject_GC_UnTrack() on the object crashed if
the previous or the next object of the PyGC_Head structure became a
dangling pointer.
(cherry picked from commit 1a4d1c1)

Co-authored-by: Victor Stinner <[email protected]>
vstinner added a commit that referenced this pull request Jan 13, 2022
@vstinner
bpo-46070: _PyGC_Fini() untracks objects (GH-30577) (GH-30580)
52937c2
Py_EndInterpreter() now explicitly untracks all objects currently
tracked by the GC. Previously, if an object was used later by another
interpreter, calling PyObject_GC_UnTrack() on the object crashed if
the previous or the next object of the PyGC_Head structure became a
dangling pointer.

(cherry picked from commit 1a4d1c1)
@pablogsal

pablogsal commented Jan 14, 2022

Copy link
Copy Markdown
Member

This approach looks sensible but another solution to consider (it may have problems) is to move all these objects into the main interpreter GC state. This way, more reference cycles can be broken there and the memory can be reclaimed when the main interpreter does a GC pass.

Thoughts on this approach?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Jan 15, 2022

Copy link
Copy Markdown
Member Author

This approach looks sensible but another solution to consider (it may have problems) is to move all these objects into the main interpreter GC state. This way, more reference cycles can be broken there and the memory can be reclaimed when the main interpreter does a GC pass. Thoughts on this approach?

For a simple object, it should be fine. But I don't understand how it would work for more complex structures with finalizers. Which interpreter is supposed to call it? Which interpreter "owns" the object? There is the "strong reference" ownership but the GC list ownership.

IMO we would avoid many not-fun-at-all problems by preventing objects to travel between interpreters ;-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@pitrou

pitrou commented Jan 15, 2022

Copy link
Copy Markdown
Member

I agree with @vstinner. Another related issue is: if objects can travel between interpreters, how would you make the object allocator per-interpreter (if you want to have a per-interpreter GIL)?

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Jan 15, 2022

Copy link
Copy Markdown
Member

I am convinced by these arguments 👍

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Jan 16, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I understand the motivation to "clean up everything" by breaking the reference cycles. But for me, the problem is way bigger than that. If you have complex objects with finalizers running complex code, the interpreter in which they are executed matters. For example, if it's a database connection. You may care in which interpreter it's run.

It reminds me a fork() problem when an application creates many objects before calling fork(). The consistency of a database connection, of a socket or a file becomes way harder to control if two processes can access it with "memory copy" of these objects. For example, a common problem is when a process opens a file and then forks: the file is not fully closed before all processes close the "shared" file descriptor (technically, there are more file descriptor copies all pointing to the same file in the kernel).

pitrou 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.

@vstinner

vstinner commented Jan 16, 2022

Copy link
Copy Markdown
Member Author

I am convinced by these arguments +1

I'm not 100% convinced, so it's good to have such discussion :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@prahal prahal mentioned this pull request Mar 22, 2022
[BUG] Kodi crashing when switching from an add-on to another LibreELEC/LibreELEC.tv#6266
Closed
@graysky graysky mannequin mentioned this pull request Apr 10, 2022
[subinterpreters] crash when importing _sre in subinterpreters in parallel (Python 3.9 regression) #90228
Closed
@vstinner vstinner mentioned this pull request Apr 29, 2022
gh-92036: Fix gc_fini_untrack() #92037
Merged
@nascheme nascheme mentioned this pull request Sep 25, 2024
[3.12] gh-116510: Fix crash during sub-interpreter shutdown #124536
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments
@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

8 participants

@vstinner @erlend-aasland @miss-islington @bedevere-bot @pablogsal @pitrou @ericsnowcurrently @the-knights-who-say-ni
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.