Skip to content

gh-135552: Make the GC clear weakrefs later. #136189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

gh-135552: Make the GC clear weakrefs later. #136189

wants to merge 3 commits into from
+235 −103

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Jul 1, 2025 •
edited
Loading

@nascheme
Make the GC clear weakrefs later.
42abb05 
Clear the weakrefs to unreachable objects after finalizers are called.
@bedevere-app bedevere-app bot mentioned this pull request Jul 1, 2025
Open
@neonene
Copy link
Contributor

neonene commented Jul 1, 2025

I can confirm this PR fixes the gh-132413 issue as well.

@nascheme
Copy link
Member Author

nascheme commented Jul 1, 2025

I think this fixes (or mostly fixes) gh-91636 as well.

@nascheme nascheme mentioned this pull request Jul 1, 2025
Open
@nascheme nascheme requested a review from pablogsal July 1, 2025 23:33
@nascheme nascheme added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 1, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @nascheme for commit 12f0b5c 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136189%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 1, 2025
@nascheme
Copy link
Member Author

nascheme commented Jul 2, 2025

This introduces refleaks, it seems. One of the leaking tests:
test.test_concurrent_futures.test_shutdown.ProcessPoolSpawnProcessPoolShutdownTest.test_shutdown_gh_132969_case_1
My unconfirmed suspicion is that a finalizer is now resurrecting an object via a weakref. Previously that weakref would be cleared before the finalizer is run. The multiprocessing finalizer logic seems very complicated. :-/

@nascheme
Copy link
Member Author

nascheme commented Jul 2, 2025

This is the smallest leaking example I could make so far. Something with ProcessPoolExecutor leaking maybe.

class LeakTest(unittest.TestCase):
    @classmethod
    def _fail_task(cls, n):
        raise ValueError("failing task")

    def test_leak_case(self):
        # this leaks references
        executor = futures.ProcessPoolExecutor(
                max_workers=1,
                max_tasks_per_child=1,
                )
        f2 = executor.submit(LeakTest._fail_task, 0)
        try:
            f2.result()
        except ValueError:
            pass

        # Ensure that the executor cleans up after called
        # shutdown with wait=False
        executor_manager_thread = executor._executor_manager_thread
        executor.shutdown(wait=False)
        time.sleep(0.2)
        executor_manager_thread.join()

    def test_leak_case2(self):
        # this does not leak
        with futures.ProcessPoolExecutor(
                max_workers=1,
                max_tasks_per_child=1,
                ) as executor:
            f2 = executor.submit(LeakTest._fail_task, 0)
            try:
                f2.result()
            except ValueError:
                pass

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

@pablogsal pablogsal Awaiting requested review from pablogsal

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 will be requested when the pull request is marked ready for review kumaraditya303 is a code owner

@pganssle pganssle Awaiting requested review from pganssle pganssle will be requested when the pull request is marked ready for review pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin will be requested when the pull request is marked ready for review abalkin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nascheme @neonene @bedevere-bot